[libvirt] [PATCH] qemu: Be more selective when determining cdrom for taint messaging
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@redhat.com> --- 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; + + ret = STREQ(linkpath, cdrompath); + + cleanup: + VIR_FREE(linkpath); + VIR_FREE(cdrompath); + return ret; +} + +#else /* __linux__ */ + +bool +virFileIsCDROM(const char *path) +{ + /* XXX implement me :-) */ + virReportUnsupportedError(); + return false; +} + +#endif /* __linux__ */ diff --git a/src/util/virfile.h b/src/util/virfile.h index 57ceb80..21caabf 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -352,4 +352,8 @@ int virFileInData(int fd, int *inData, long long *length); +bool +virFileIsCDROM(const char *path) + ATTRIBUTE_NONNULL(1); + #endif /* __VIR_FILE_H */ -- 2.9.5
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@redhat.com> --- 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 :(
+ + 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
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@redhat.com> --- 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. John
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@redhat.com> --- 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.
participants (2)
-
John Ferlan -
Michal Privoznik