From 47ce9ef7f89032c4079bf5132a12d1bfd4d5bca5 Mon Sep 17 00:00:00 2001 From: Stefan Weil Date: Tue, 22 May 2012 23:23:32 +0200 Subject: [PATCH 1/5] virtio: Fix compiler warning for non Linux hosts The local variables ret, i are only used if __linux__ is defined. Signed-off-by: Stefan Weil Signed-off-by: Kevin Wolf --- hw/virtio-blk.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c index f9e1896ea9..60750cb2ef 100644 --- a/hw/virtio-blk.c +++ b/hw/virtio-blk.c @@ -147,9 +147,11 @@ static VirtIOBlockReq *virtio_blk_get_request(VirtIOBlock *s) static void virtio_blk_handle_scsi(VirtIOBlockReq *req) { +#ifdef __linux__ int ret; - int status = VIRTIO_BLK_S_OK; int i; +#endif + int status = VIRTIO_BLK_S_OK; /* * We require at least one output segment each for the virtio_blk_outhdr From 6f3c714eb7730630241fd0b33b799352d7feb876 Mon Sep 17 00:00:00 2001 From: MORITA Kazutaka Date: Wed, 30 May 2012 01:05:15 +0900 Subject: [PATCH 2/5] sheepdog: fix return value of do_load_save_vm_state bdrv_save_vmstate and bdrv_load_vmstate should return the vmstate size on success, and -errno on error. Signed-off-by: MORITA Kazutaka Signed-off-by: Kevin Wolf --- block/sheepdog.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/block/sheepdog.c b/block/sheepdog.c index 6d52277a89..f46ca8fb69 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -1957,7 +1957,7 @@ static int do_load_save_vmstate(BDRVSheepdogState *s, uint8_t *data, int64_t pos, int size, int load) { int fd, create; - int ret = 0; + int ret = 0, remaining = size; unsigned int data_len; uint64_t vmstate_oid; uint32_t vdi_index; @@ -1968,11 +1968,11 @@ static int do_load_save_vmstate(BDRVSheepdogState *s, uint8_t *data, return fd; } - while (size) { + while (remaining) { vdi_index = pos / SD_DATA_OBJ_SIZE; offset = pos % SD_DATA_OBJ_SIZE; - data_len = MIN(size, SD_DATA_OBJ_SIZE); + data_len = MIN(remaining, SD_DATA_OBJ_SIZE); vmstate_oid = vid_to_vmstate_oid(s->inode.vdi_id, vdi_index); @@ -1993,9 +1993,9 @@ static int do_load_save_vmstate(BDRVSheepdogState *s, uint8_t *data, } pos += data_len; - size -= data_len; - ret += data_len; + remaining -= data_len; } + ret = size; cleanup: closesocket(fd); return ret; From c2d76497b6eafcaedc806e07804e7bed55a98a0b Mon Sep 17 00:00:00 2001 From: Jim Meyering Date: Mon, 28 May 2012 09:27:54 +0200 Subject: [PATCH 3/5] block: prevent snapshot mode $TMPDIR symlink attack In snapshot mode, bdrv_open creates an empty temporary file without checking for mkstemp or close failure, and ignoring the possibility of a buffer overrun given a surprisingly long $TMPDIR. Change the get_tmp_filename function to return int (not void), so that it can inform its two callers of those failures. Also avoid the risk of buffer overrun and do not ignore mkstemp or close failure. Update both callers (in block.c and vvfat.c) to propagate temp-file-creation failure to their callers. get_tmp_filename creates and closes an empty file, while its callers later open that presumed-existing file with O_CREAT. The problem was that a malicious user could provoke mkstemp failure and race to create a symlink with the selected temporary file name, thus causing the qemu process (usually root owned) to open through the symlink, overwriting an attacker-chosen file. This addresses CVE-2012-2652. http://bugzilla.redhat.com/CVE-2012-2652 Signed-off-by: Jim Meyering Reviewed-by: Stefan Hajnoczi Signed-off-by: Kevin Wolf --- block.c | 37 ++++++++++++++++++++++++------------- block/vvfat.c | 7 ++++++- block_int.h | 2 +- 3 files changed, 31 insertions(+), 15 deletions(-) diff --git a/block.c b/block.c index af2ab4f3ea..7547051ec2 100644 --- a/block.c +++ b/block.c @@ -409,28 +409,36 @@ int bdrv_create_file(const char* filename, QEMUOptionParameter *options) return bdrv_create(drv, filename, options); } +/* + * Create a uniquely-named empty temporary file. + * Return 0 upon success, otherwise a negative errno value. + */ +int get_tmp_filename(char *filename, int size) +{ #ifdef _WIN32 -void get_tmp_filename(char *filename, int size) -{ char temp_dir[MAX_PATH]; - - GetTempPath(MAX_PATH, temp_dir); - GetTempFileName(temp_dir, "qem", 0, filename); -} + /* GetTempFileName requires that its output buffer (4th param) + have length MAX_PATH or greater. */ + assert(size >= MAX_PATH); + return (GetTempPath(MAX_PATH, temp_dir) + && GetTempFileName(temp_dir, "qem", 0, filename) + ? 0 : -GetLastError()); #else -void get_tmp_filename(char *filename, int size) -{ int fd; const char *tmpdir; - /* XXX: race condition possible */ tmpdir = getenv("TMPDIR"); if (!tmpdir) tmpdir = "/tmp"; - snprintf(filename, size, "%s/vl.XXXXXX", tmpdir); + if (snprintf(filename, size, "%s/vl.XXXXXX", tmpdir) >= size) { + return -EOVERFLOW; + } fd = mkstemp(filename); - close(fd); -} + if (fd < 0 || close(fd)) { + return -errno; + } + return 0; #endif +} /* * Detect host devices. By convention, /dev/cdrom[N] is always @@ -753,7 +761,10 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags, bdrv_delete(bs1); - get_tmp_filename(tmp_filename, sizeof(tmp_filename)); + ret = get_tmp_filename(tmp_filename, sizeof(tmp_filename)); + if (ret < 0) { + return ret; + } /* Real path is meaningless for protocols */ if (is_protocol) diff --git a/block/vvfat.c b/block/vvfat.c index 2dc9d50888..0fd3367d82 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -2808,7 +2808,12 @@ static int enable_write_target(BDRVVVFATState *s) array_init(&(s->commits), sizeof(commit_t)); s->qcow_filename = g_malloc(1024); - get_tmp_filename(s->qcow_filename, 1024); + ret = get_tmp_filename(s->qcow_filename, 1024); + if (ret < 0) { + g_free(s->qcow_filename); + s->qcow_filename = NULL; + return ret; + } bdrv_qcow = bdrv_find_format("qcow"); options = parse_option_parameters("", bdrv_qcow->create_options, NULL); diff --git a/block_int.h b/block_int.h index b80e66db6e..3d4abc6575 100644 --- a/block_int.h +++ b/block_int.h @@ -335,7 +335,7 @@ struct BlockDriverState { BlockJob *job; }; -void get_tmp_filename(char *filename, int size); +int get_tmp_filename(char *filename, int size); void bdrv_set_io_limits(BlockDriverState *bs, BlockIOLimit *io_limits); From 136be99e6e2130d3cd960b6b7d0ca86b6f011e5f Mon Sep 17 00:00:00 2001 From: Christian Borntraeger Date: Thu, 24 May 2012 13:22:55 +0200 Subject: [PATCH 4/5] virtio-blk: Fix geometry sector calculation Currently the sector value for the geometry is masked, even if the user usesa command line parameter that explicitely gives a number. This breaks dasd devices on s390. A dasd device can have a physical block size of 4096 (== same for logical block size) and a typcial geometry of 15 heads and 12 sectors per cyl. The ibm partition detection relies on a correct geometry reported by the device. Unfortunately the current code changes 12 to 8. This would be necessary if the total size is not a multiple of logical sector size, but for dasd this is not the case. This patch checks the device size and only applies sector mask if necessary. Signed-off-by: Christian Borntraeger CC: Christoph Hellwig Reviewed-by: Paolo Bonzini Signed-off-by: Kevin Wolf --- hw/virtio-blk.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c index 60750cb2ef..fe0774617b 100644 --- a/hw/virtio-blk.c +++ b/hw/virtio-blk.c @@ -491,7 +491,22 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config) stw_raw(&blkcfg.min_io_size, s->conf->min_io_size / blk_size); stw_raw(&blkcfg.opt_io_size, s->conf->opt_io_size / blk_size); blkcfg.heads = heads; - blkcfg.sectors = secs & ~s->sector_mask; + /* + * We must ensure that the block device capacity is a multiple of + * the logical block size. If that is not the case, lets use + * sector_mask to adopt the geometry to have a correct picture. + * For those devices where the capacity is ok for the given geometry + * we dont touch the sector value of the geometry, since some devices + * (like s390 dasd) need a specific value. Here the capacity is already + * cyls*heads*secs*blk_size and the sector value is not block size + * divided by 512 - instead it is the amount of blk_size blocks + * per track (cylinder). + */ + if (bdrv_getlength(s->bs) / heads / secs % blk_size) { + blkcfg.sectors = secs & ~s->sector_mask; + } else { + blkcfg.sectors = secs; + } blkcfg.size_max = 0; blkcfg.physical_block_exp = get_physical_block_exp(s->conf); blkcfg.alignment_offset = 0; From 4bb9c939a57103898f5a51aa6a7336eb3320d923 Mon Sep 17 00:00:00 2001 From: Daniel Verkamp Date: Tue, 22 May 2012 16:26:42 -0700 Subject: [PATCH 5/5] ahci: SATA FIS is 20 bytes, not 0x20 As in the SATA and AHCI specifications, a FIS is 5 Dwords of 4 bytes each, which comes to 20 bytes (decimal), not 0x20. Signed-off-by: Daniel Verkamp Signed-off-by: Kevin Wolf --- hw/ide/ahci.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index a883a920be..2d7d03d772 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -462,7 +462,7 @@ static void ahci_check_cmd_bh(void *opaque) static void ahci_init_d2h(AHCIDevice *ad) { - uint8_t init_fis[0x20]; + uint8_t init_fis[20]; IDEState *ide_state = &ad->port.ifs[0]; memset(init_fis, 0, sizeof(init_fis)); @@ -619,7 +619,7 @@ static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis) d2h_fis[11] = cmd_fis[11]; d2h_fis[12] = cmd_fis[12]; d2h_fis[13] = cmd_fis[13]; - for (i = 14; i < 0x20; i++) { + for (i = 14; i < 20; i++) { d2h_fis[i] = 0; }