On 09/06/2017 06:42 PM, John Ferlan wrote: > > > 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@xxxxxxxxxx> >>> --- >>> 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. Well, I guess since this function is needed only for tainting (which we have but not use really [1]), I'm okay with STRPREFIX("/dev/cdrom") || STRPREFIX("/dev/sr"). Worst case scenario we will trigger tainting even if the device is not a CD-ROM. So what. Michal 1: I mean, we debug problem regardless of tainted domain. -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
