[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-devel] [PATCH] QEMU "drive_init()" Disk Format Security Bypass
Thanks to everyone for their comments. It turns out that the file:/path/to/image syntax doesn't work properly at the moment unless the image is a raw image: although qemu-dm will treat it as a cow image, if it is in a format it understands, blktap will not. This means that the guest would see the processed cow version via the emulated IDE controller but the raw cow differences file, header and all, via the PV block interface. So it is fine for us to make file:/path/to/image always expect raw images. Non-raw images are done with tap:qcow:/path/to/image and that currently works. Below is a patch, with suggested commit message. The resulting code in xenstore.c is sadly not very simple. This is because it is untangling a mess made (entirely pointlessly) by xend, as well as translating between the different naming schemes used by xenstore and qemu for image formats. I hope to remove this block driver special case machinery from qemu as part of or shortly after I complete my merge with qemu upstream. ioemu: fix disk format security vulnerability * make the xenstore reader in qemu-dm's startup determine which of qemu's block drivers to use according to the xenstore backend `type' field. This `type' field typically comes from the front of the drive mapping string in ioemu. The supported cases are: xm config file string `type' image format qemu driver phy:[/dev/]<device> phy raw image bdrv_raw file:<filename> file raw image bdrv_raw tap:aio:<filename> tap raw image bdrv_raw tap:qcow:<image> tap not raw autoprobe tap:<cow-fmt>:<image> tap named format bdrv_<cow-fmt> It is still necessary to autoprobe when the image is specified as `tap:qcow:<image>', because qemu distinguishes `qcow' and `qcow2' whereas blktap doesn't; `qcow' in xenstore typically means what qemu calls qcow2. This is OK because qemu can safely distinguish the different cow formats provided we know it's not a raw image. * Make the format autoprobing machinery never return `raw'. This has two purposes: firstly, it arranges that the `tap:qcow:...' case above can be handled without accidentally falling back to raw format. Secondly it prevents accidents in case the code changes in future: autoprobing will now always fail on supposed cow files which actually contain junk, rather than giving the guest access to the underlying file. Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx> Ian. diff -r c99a88623eda tools/ioemu/block.c --- a/tools/ioemu/block.c Thu May 08 14:33:31 2008 +0100 +++ b/tools/ioemu/block.c Fri May 09 15:31:46 2008 +0100 @@ -254,7 +254,7 @@ static BlockDriver *find_protocol(const #endif p = strchr(filename, ':'); if (!p) - return &bdrv_raw; + return NULL; /* do not ever guess raw, it is a security problem! */ len = p - filename; if (len > sizeof(protocol) - 1) len = sizeof(protocol) - 1; diff -r c99a88623eda tools/ioemu/xenstore.c --- a/tools/ioemu/xenstore.c Thu May 08 14:33:31 2008 +0100 +++ b/tools/ioemu/xenstore.c Fri May 09 16:32:43 2008 +0100 @@ -90,6 +90,7 @@ void xenstore_parse_domain_config(int hv int i, is_scsi, is_hdN = 0; unsigned int len, num, hd_index, pci_devid = 0; BlockDriverState *bs; + BlockDriver *format; for(i = 0; i < MAX_DISKS + MAX_SCSI_DISKS; i++) media_filename[i] = NULL; @@ -135,6 +136,8 @@ void xenstore_parse_domain_config(int hv } for (i = 0; i < num; i++) { + format = NULL; /* don't know what the format is yet */ + /* read the backend path */ if (pasprintf(&buf, "%s/device/vbd/%s/backend", path, e[i]) == -1) continue; @@ -181,13 +184,20 @@ void xenstore_parse_domain_config(int hv drv = xs_read(xsh, XBT_NULL, buf, &len); if (drv == NULL) continue; - /* Strip off blktap sub-type prefix aio: - QEMU can autodetect this */ + /* Obtain blktap sub-type prefix */ if (!strcmp(drv, "tap") && params[0]) { char *offset = strchr(params, ':'); if (!offset) continue ; + free(drv); + drv = malloc(offset - params + 1); + memcpy(drv, params, offset - params); + drv[offset - params] = '\0'; + if (!strcmp(drv, "aio")) + /* qemu does aio anyway if it can */ + format = &bdrv_raw; memmove(params, offset+1, strlen(offset+1)+1 ); - fprintf(logfile, "Strip off blktap sub-type prefix to %s\n", params); + fprintf(logfile, "Strip off blktap sub-type prefix to %s (drv '%s')\n", params, drv); } /* Prefix with /dev/ if needed */ if (!strcmp(drv, "phy") && params[0] != '/') { @@ -195,6 +205,7 @@ void xenstore_parse_domain_config(int hv sprintf(newparams, "/dev/%s", params); free(params); params = newparams; + format = &bdrv_raw; } /* @@ -240,8 +251,25 @@ void xenstore_parse_domain_config(int hv #endif if (params[0]) { - if (bdrv_open(bs, params, 0 /* snapshot */) < 0) - fprintf(stderr, "qemu: could not open vbd '%s' or hard disk image '%s'\n", buf, params); + if (!format) { + if (!drv) { + fprintf(stderr, "qemu: type (image format) not specified for vbd '%s' or image '%s'\n", buf, params); + continue; + } + if (!strcmp(drv,"qcow")) { + /* autoguess qcow vs qcow2 */ + } else if (!strcmp(drv,"file")) { + format = &bdrv_raw; + } else { + format = bdrv_find_format(drv); + if (!format) { + fprintf(stderr, "qemu: type (image format) '%s' unknown for vbd '%s' or image '%s'\n", drv, buf, params); + continue; + } + } + } + if (bdrv_open2(bs, params, 0 /* snapshot */, format) < 0) + fprintf(stderr, "qemu: could not open vbd '%s' or hard disk image '%s' (drv '%s')\n", buf, params, drv ? drv : "?"); } } _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxx http://lists.xensource.com/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |