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

[Michal Privoznik]

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

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.