https://bugzilla.redhat.com/show_bug.cgi?id=1471225 Commit id '99a2d6af2' was a bit too aggressive with determining whether the provided path was a "physical" cd-rom in order to generate a taint message due to the possibility of some guest and host trying to control the tray. For cd-rom guest devices backed to some VIR_STORAGE_TYPE_FILE storage, this wouldn't be a problem and as such it shouldn't be a problem for guest devices using some sort of block device on the host such as iSCSI, LVM, or a Disk pool would present. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 2 +- src/util/virfile.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 4 ++++ 4 files changed, 54 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f30a04b..0354568 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1680,6 +1680,7 @@ virFileGetMountSubtree; virFileHasSuffix; virFileInData; virFileIsAbsPath; +virFileIsCDROM; virFileIsDir; virFileIsExecutable; virFileIsLink; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9cff501..426c577 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4807,7 +4807,7 @@ void qemuDomainObjCheckDiskTaint(virQEMUDriverPtr driver, if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM && virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_BLOCK && - disk->src->path) + disk->src->path && virFileIsCDROM(disk->src->path)) qemuDomainObjTaint(driver, obj, VIR_DOMAIN_TAINT_CDROM_PASSTHROUGH, logCtxt); diff --git a/src/util/virfile.c b/src/util/virfile.c index 2f28e83..4c31949 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -4166,3 +4166,51 @@ virFileReadValueString(char **value, const char *format, ...) VIR_FREE(str); return ret; } + + +#if defined(__linux__) + +/* virFileIsCDROM + * @path: Supplied path. + * + * Determine if the path is a CD-ROM path. Typically on Linux systems this + * is either /dev/cdrom or /dev/sr0, so those are easy checks. Still if + * someone is trying to be tricky, we can resolve the link to /dev/cdrom + * and compare it to the resolved link of the supplied @path to compare + * if they're the same. + * + * Returns true if the path is a CDROM, false otherwise. + */ +bool +virFileIsCDROM(const char *path) +{ + bool ret = false; + char *linkpath = NULL; + char *cdrompath = NULL; + + if (STREQ(path, "/dev/cdrom") || STREQ(path, "/dev/sr0")) + return true;

+ + if (virFileResolveLink(path, &linkpath) < 0 || + virFileResolveLink("/dev/cdrom", &cdrompath) < 0) + goto cleanup;

On 09/06/2017 08:17 AM, Michal Privoznik wrote: > On 09/05/2017 10:51 PM, John Ferlan wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1471225 >> >> Commit id '99a2d6af2' was a bit too aggressive with determining whether >> the provided path was a "physical" cd-rom in order to generate a taint >> message due to the possibility of some guest and host trying to control >> the tray. For cd-rom guest devices backed to some VIR_STORAGE_TYPE_FILE >> storage, this wouldn't be a problem and as such it shouldn't be a problem >> for guest devices using some sort of block device on the host such as >> iSCSI, LVM, or a Disk pool would present. >> >> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >> --- >> src/libvirt_private.syms | 1 + >> src/qemu/qemu_domain.c | 2 +- >> src/util/virfile.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ >> src/util/virfile.h | 4 ++++ >> 4 files changed, 54 insertions(+), 1 deletion(-) >> >> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms >> index f30a04b..0354568 100644 >> --- a/src/libvirt_private.syms >> +++ b/src/libvirt_private.syms >> @@ -1680,6 +1680,7 @@ virFileGetMountSubtree; >> virFileHasSuffix; >> virFileInData; >> virFileIsAbsPath; >> +virFileIsCDROM; >> virFileIsDir; >> virFileIsExecutable; >> virFileIsLink; >> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c >> index 9cff501..426c577 100644 >> --- a/src/qemu/qemu_domain.c >> +++ b/src/qemu/qemu_domain.c >> @@ -4807,7 +4807,7 @@ void qemuDomainObjCheckDiskTaint(virQEMUDriverPtr driver, >> >> if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM && >> virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_BLOCK && >> - disk->src->path) >> + disk->src->path && virFileIsCDROM(disk->src->path)) >> qemuDomainObjTaint(driver, obj, VIR_DOMAIN_TAINT_CDROM_PASSTHROUGH, >> logCtxt); >> >> diff --git a/src/util/virfile.c b/src/util/virfile.c >> index 2f28e83..4c31949 100644 >> --- a/src/util/virfile.c >> +++ b/src/util/virfile.c >> @@ -4166,3 +4166,51 @@ virFileReadValueString(char **value, const char *format, ...) >> VIR_FREE(str); >> return ret; >> } >> + >> + >> +#if defined(__linux__) >> + >> +/* virFileIsCDROM >> + * @path: Supplied path. >> + * >> + * Determine if the path is a CD-ROM path. Typically on Linux systems this >> + * is either /dev/cdrom or /dev/sr0, so those are easy checks. Still if >> + * someone is trying to be tricky, we can resolve the link to /dev/cdrom >> + * and compare it to the resolved link of the supplied @path to compare >> + * if they're the same. >> + * >> + * Returns true if the path is a CDROM, false otherwise. >> + */ >> +bool >> +virFileIsCDROM(const char *path) >> +{ >> + bool ret = false; >> + char *linkpath = NULL; >> + char *cdrompath = NULL; >> + >> + if (STREQ(path, "/dev/cdrom") || STREQ(path, "/dev/sr0")) >> + return true; > > What if I have two CDROMs? /dev/sr0 and /dev/sr1? I'm worried that name > match is not sufficient and we need to lstat() and check if the major > number (st.st_rdev) is 11 (0xb) (according to > Documentation/admin-guide/devices.txt from kernel sources). And even > that might be not enough :( > Ironically I had "STRPREFIX(path, "/dev/sr") originally, but at the last moment switched to /dev/sr0. I also considered the stat option for the major number. Could do the same STRPREFIX for /dev/cdrom too. In a way, I was hoping to get more ideas from the community on this. Are we always "sure" that CD-ROM's will start with /dev/sr? That was something else I couldn't tell. And there's so much "history" out there in google land that one could spend way too much time researching for all the crazy ways to name a cdrom (the path stuff below was mainly a reaction to something I read, but I forget where). I did check QEMU and ironically sources there are included to use /dev/cdrom. Other options I considered, but felt were a bit over the top/excessive: 1. Could defer to nodedev - it would be able to "know" which devices are cdrom... stores that in <drive_type>... could add a capability 'cdrom' that would also returning a list of cdrom devices, then compare the resolved path against that list. 2. Could use something like 'processLU', but only for SCSI devices... Essentially ends up searching for a 0x5 in the "/sys/bus/scsi/devices/*/type", then again comparing resolved paths against the list returned. Neither of the options was palatable for the "benefit" gained of being able to further filter and decide which which paths wouldn't get the tainted message. >> + >> + if (virFileResolveLink(path, &linkpath) < 0 || >> + virFileResolveLink("/dev/cdrom", &cdrompath) < 0) >> + goto cleanup; > > This will only work for cases where /dev/cdrom points to the device in > @path. However, if /dev/cdrom is pointing to /dev/sr0 and I'm calling > this function over /dev/sr1 (because I have a machine with dozens of > CDROMs) this function returns false which is obviously wrong. > > Michal > True, sigh. I'm not sure how "easily" solve-able this will be for us to determine that the provided $path is some block device (iSCSI, LVM, Disk, etc.) or the "real" CD-ROM.