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(-)
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.