[libvirt] [PATCH] qemu: Be more selective when determining cdrom for taint messaging

[John Ferlan]

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 at 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