[libvirt] [PATCH 0/4] misc virt-aa-helper fixes
Hi, this was mostly created by clearing old libvirt bugs in Ubuntu. USB passthrough so far often used workarounds but can be fixed in virt-aa-helper. I have some more changes planned but these seem to become longer term activities so I didn't want to postpone those easier ones due to that and submit them today. Christian Ehrhardt (4): virt-aa-helper: fix paths for usb hostdevs virt-aa-helper: fix libusb access to udev usb data virt-aa-helper: allow spaces in vm names virt-aa-helper: put static rules in quotes examples/apparmor/libvirt-qemu | 3 +++ src/security/virt-aa-helper.c | 12 ++++++++---- 2 files changed, 11 insertions(+), 4 deletions(-) -- 2.7.4
If users only specified vendor&product (the common case) then parsing the xml via virDomainHostdevSubsysUSBDefParseXML would only set these. Bus and Device would much later be added when the devices are prepared to be added. Due to that a hot-add of a usb hostdev works as the device is prepared and virt-aa-helper processes the new internal xml. But on an initial guest start at the time virt-aa-helper renders the apparmor rules the bus/device id's are not set yet: p ctl->def->hostdevs[0]->source.subsys.u.usb $12 = {autoAddress = false, bus = 0, device = 0, vendor = 1921, product = 21888} That causes rules to be wrong: "/dev/bus/usb/000/000" rw, The fix calls virHostdevFindUSBDevice after reading the XML from irt-aa-helper to only add apparmor rules for devices that could be found and now are fully known to be able to write the rule correctly. It uncondtionally sets virHostdevFindUSBDevice mandatory attribute as adding an apparmor rule for a device not found makes no sense no matter what startup policy it has set. Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/virt-aa-helper.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 7944dc1..d1518ea 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -55,6 +55,7 @@ #include "virrandom.h" #include "virstring.h" #include "virgettext.h" +#include "virhostdev.h" #include "storage/storage_source.h" @@ -1069,6 +1070,9 @@ get_files(vahControl * ctl) if (usb == NULL) continue; + if (virHostdevFindUSBDevice(dev, true, &usb) < 0) + continue; + rc = virUSBDeviceFileIterate(usb, file_iterate_hostdev_cb, &buf); virUSBDeviceFree(usb); if (rc != 0) -- 2.7.4
libusb as used by qemu needs to read data from /run/udev/data/ about usb devices. That is read once on the first initialization of libusb_init by qemu. Therefore generating just the device we need would not be sufficient as another hotplug later can need another device which would fail as the data is no more re-read at this point. But we can restrict the paths very much to just the major number of potential usb devices which will make it match approximately the detail that e.g. an lsusb -v would reveal - that is much safer than the "/run/udev/data/* r" blanket many users are using now as a workaround. Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- examples/apparmor/libvirt-qemu | 3 +++ 1 file changed, 3 insertions(+) diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu index dcfb1a5..b341e31 100644 --- a/examples/apparmor/libvirt-qemu +++ b/examples/apparmor/libvirt-qemu @@ -29,6 +29,9 @@ # For hostdev access. The actual devices will be added dynamically /sys/bus/usb/devices/ r, /sys/devices/**/usb[0-9]*/** r, + # libusb needs udev data about usb devices (~equal to content of lsusb -v) + /run/udev/data/c16[6,7]* r, + /run/udev/data/c18[0,8,9]* r, # WARNING: this gives the guest direct access to host hardware and specific # portions of shared memory. This is required for sound using ALSA with kvm, -- 2.7.4
libvirt allows spaces in vm names, there were issues in the past but it seems not removed so the assumption has to be that spaces are continuing to be allowed. Therefore virt-aa-helper should not reject spaces in vm names anymore if it is goign to be refused causing issues then the parser or xml schema should do so. Apparmor rules are in quotes, so a space in a path based on the name works. Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/virt-aa-helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index d1518ea..5f4519d 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -449,7 +449,7 @@ valid_name(const char *name) { /* just try to filter out any dangerous characters in the name that can be * used to subvert the profile */ - const char *bad = " /[]*"; + const char *bad = "/[]*"; if (strlen(name) == 0) return -1; -- 2.7.4
To avoid any issues later on if paths ever change (unlikely but possible) and to match the style of other generated rules the paths of the static rules have to be quoted as well. Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/virt-aa-helper.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 5f4519d..95906e6 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1149,11 +1149,11 @@ get_files(vahControl * ctl) } } if (needsvhost) - virBufferAddLit(&buf, " /dev/vhost-net rw,\n"); + virBufferAddLit(&buf, " \"/dev/vhost-net\" rw,\n"); if (needsVfio) { - virBufferAddLit(&buf, " /dev/vfio/vfio rw,\n"); - virBufferAddLit(&buf, " /dev/vfio/[0-9]* rw,\n"); + virBufferAddLit(&buf, " \"/dev/vfio/vfio\" rw,\n"); + virBufferAddLit(&buf, " \"/dev/vfio/[0-9]*\" rw,\n"); } if (ctl->newfile) -- 2.7.4
Hi, just a ping to ask if anybody could take a look to review this set of smaller changes? On Wed, Sep 20, 2017 at 4:59 PM, Christian Ehrhardt < christian.ehrhardt@canonical.com> wrote:
Hi, this was mostly created by clearing old libvirt bugs in Ubuntu. USB passthrough so far often used workarounds but can be fixed in virt-aa-helper. I have some more changes planned but these seem to become longer term activities so I didn't want to postpone those easier ones due to that and submit them today.
Christian Ehrhardt (4): virt-aa-helper: fix paths for usb hostdevs virt-aa-helper: fix libusb access to udev usb data virt-aa-helper: allow spaces in vm names virt-aa-helper: put static rules in quotes
examples/apparmor/libvirt-qemu | 3 +++ src/security/virt-aa-helper.c | 12 ++++++++---- 2 files changed, 11 insertions(+), 4 deletions(-)
-- 2.7.4
-- Christian Ehrhardt Software Engineer, Ubuntu Server Canonical Ltd
On 09/20/2017 04:59 PM, Christian Ehrhardt wrote:
If users only specified vendor&product (the common case) then parsing the xml via virDomainHostdevSubsysUSBDefParseXML would only set these. Bus and Device would much later be added when the devices are prepared to be added.
Due to that a hot-add of a usb hostdev works as the device is prepared and virt-aa-helper processes the new internal xml. But on an initial guest start at the time virt-aa-helper renders the apparmor rules the bus/device id's are not set yet:
p ctl->def->hostdevs[0]->source.subsys.u.usb $12 = {autoAddress = false, bus = 0, device = 0, vendor = 1921, product = 21888}
That causes rules to be wrong: "/dev/bus/usb/000/000" rw,
The fix calls virHostdevFindUSBDevice after reading the XML from irt-aa-helper to only add apparmor rules for devices that could be found and now are fully known to be able to write the rule correctly.
It uncondtionally sets virHostdevFindUSBDevice mandatory attribute as adding an apparmor rule for a device not found makes no sense no matter what startup policy it has set.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/virt-aa-helper.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 7944dc1..d1518ea 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -55,6 +55,7 @@ #include "virrandom.h" #include "virstring.h" #include "virgettext.h" +#include "virhostdev.h"
#include "storage/storage_source.h"
@@ -1069,6 +1070,9 @@ get_files(vahControl * ctl) if (usb == NULL) continue;
+ if (virHostdevFindUSBDevice(dev, true, &usb) < 0) + continue; +
Shouldn't we rather fail in this case? Or, what happens if startupPolicy of the device is set to 'optional'? I think we need to error out here (although, we've probably errored out earlier in the process). ACK to the rest of the patches (after some typo clean up, esp. in the commit messages). Michal
On Fri, Sep 29, 2017 at 4:58 PM, Michal Privoznik <mprivozn@redhat.com> wrote:
On 09/20/2017 04:59 PM, Christian Ehrhardt wrote:
If users only specified vendor&product (the common case) then parsing the xml via virDomainHostdevSubsysUSBDefParseXML would only set these. Bus and Device would much later be added when the devices are prepared to be added.
Due to that a hot-add of a usb hostdev works as the device is prepared and virt-aa-helper processes the new internal xml. But on an initial guest start at the time virt-aa-helper renders the apparmor rules the bus/device id's are not set yet:
p ctl->def->hostdevs[0]->source.subsys.u.usb $12 = {autoAddress = false, bus = 0, device = 0, vendor = 1921, product = 21888}
That causes rules to be wrong: "/dev/bus/usb/000/000" rw,
The fix calls virHostdevFindUSBDevice after reading the XML from irt-aa-helper to only add apparmor rules for devices that could be found and now are fully known to be able to write the rule correctly.
It uncondtionally sets virHostdevFindUSBDevice mandatory attribute as adding an apparmor rule for a device not found makes no sense no matter what startup policy it has set.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/virt-aa-helper.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 7944dc1..d1518ea 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -55,6 +55,7 @@ #include "virrandom.h" #include "virstring.h" #include "virgettext.h" +#include "virhostdev.h"
#include "storage/storage_source.h"
@@ -1069,6 +1070,9 @@ get_files(vahControl * ctl) if (usb == NULL) continue;
+ if (virHostdevFindUSBDevice(dev, true, &usb) < 0) + continue; +
Shouldn't we rather fail in this case? Or, what happens if startupPolicy of the device is set to 'optional'? I think we need to error out here (although, we've probably errored out earlier in the process).
Hi, sorry for the late reply, but I was finally getting some time off for a few days. I intentionally decided not to error out to avoid a new "source" of issues. Compare the two options we have: 1. continue if not finding the device 1.1 likely case we found it, rule will be correct - good 1.2 we don't find it (for whatever reason) - we are "as bad" as before this fix, but not worse 2. error out if not finding the device 2.1 likely case we found it, rule will be correct - good 2.2 we don't find it (for whatever reason) - we are now failing completely What I don't like about 2.2 is that there might be cases things would have been kind of ok, depsite whatever dark usb magic hit some special setup. In those cases if we error out we add a new chance to fail. And as there are often too many unknowns, so I chose the safer option.
ACK to the rest of the patches (after some typo clean up, esp. in the commit messages).
Thanks, do you want me to clean up commit messages or will somebody do (to his preferred style) on accepting the commit?
Michal
-- Christian Ehrhardt Software Engineer, Ubuntu Server Canonical Ltd
On Wed, 2017-09-20 at 16:59 +0200, Christian Ehrhardt wrote:
+ # libusb needs udev data about usb devices (~equal to content of lsusb -v) + /run/udev/data/c16[6,7]* r, + /run/udev/data/c18[0,8,9]* r,
This read-only access looks fine to me. +1 -- Jamie Strandboge | http://www.canonical.com
On Wed, 2017-09-20 at 16:59 +0200, Christian Ehrhardt wrote:
To avoid any issues later on if paths ever change (unlikely but possible) and to match the style of other generated rules the paths of the static rules have to be quoted as well.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/virt-aa-helper.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa- helper.c index 5f4519d..95906e6 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1149,11 +1149,11 @@ get_files(vahControl * ctl) } } if (needsvhost) - virBufferAddLit(&buf, " /dev/vhost-net rw,\n"); + virBufferAddLit(&buf, " \"/dev/vhost-net\" rw,\n");
if (needsVfio) { - virBufferAddLit(&buf, " /dev/vfio/vfio rw,\n"); - virBufferAddLit(&buf, " /dev/vfio/[0-9]* rw,\n"); + virBufferAddLit(&buf, " \"/dev/vfio/vfio\" rw,\n"); + virBufferAddLit(&buf, " \"/dev/vfio/[0-9]*\" rw,\n"); }
if (ctl->newfile)
LGTM +1 -- Jamie Strandboge | http://www.canonical.com
On Wed, 2017-09-20 at 16:59 +0200, Christian Ehrhardt wrote:
libvirt allows spaces in vm names, there were issues in the past but it seems not removed so the assumption has to be that spaces are continuing to be allowed.
Therefore virt-aa-helper should not reject spaces in vm names anymore if it is goign to be refused causing issues then the parser or xml schema should do so. Apparmor rules are in quotes, so a space in a path based on the name works.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/virt-aa-helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa- helper.c index d1518ea..5f4519d 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -449,7 +449,7 @@ valid_name(const char *name) { /* just try to filter out any dangerous characters in the name that can be * used to subvert the profile */ - const char *bad = " /[]*"; + const char *bad = "/[]*";
if (strlen(name) == 0) return -1;
Your justification seems reasonable. It does mean that we'll need always quote rules that use def->name and looking at virt-aa-helper.c, that seems to be the case. All that said, I was surprised that tests/virt-aa-helper-test didn't need to be updated, but, indeed, this is a testing gap. +1 as is, but perhaps in a follow-up patch you could expand bad to be: const char *bad = "/[]{}?^,\"*"; '{', '}', '?', '^', ',' and '"' are characters used in AARE (see 'Globbing' in 'man apparmor.d') and add tests to tests/virt-aa-helper- test for this. Thanks! -- Jamie Strandboge | http://www.canonical.com
On Tue, 2017-10-17 at 09:04 +0200, Christian Ehrhardt wrote:
On Fri, Sep 29, 2017 at 4:58 PM, Michal Privoznik <mprivozn@redhat.co m> wrote:
On 09/20/2017 04:59 PM, Christian Ehrhardt wrote:
If users only specified vendor&product (the common case) then parsing the xml via virDomainHostdevSubsysUSBDefParseXML would only set these. Bus and Device would much later be added when the devices are prepared to be added.
Due to that a hot-add of a usb hostdev works as the device is prepared and virt-aa-helper processes the new internal xml. But on an initial guest start at the time virt-aa-helper renders the apparmor rules the bus/device id's are not set yet:
p ctl->def->hostdevs[0]->source.subsys.u.usb $12 = {autoAddress = false, bus = 0, device = 0, vendor = 1921, product = 21888}
That causes rules to be wrong: "/dev/bus/usb/000/000" rw,
The fix calls virHostdevFindUSBDevice after reading the XML from irt-aa-helper to only add apparmor rules for devices that could be found and now are fully known to be able to write the rule correctly.
It uncondtionally sets virHostdevFindUSBDevice mandatory attribute as adding an apparmor rule for a device not found makes no sense no matter what startup policy it has set.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.c om> --- src/security/virt-aa-helper.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/security/virt-aa-helper.c
b/src/security/virt-aa-helper.c
index 7944dc1..d1518ea 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -55,6 +55,7 @@ #include "virrandom.h" #include "virstring.h" #include "virgettext.h" +#include "virhostdev.h"
#include "storage/storage_source.h"
@@ -1069,6 +1070,9 @@ get_files(vahControl * ctl) if (usb == NULL) continue;
+ if (virHostdevFindUSBDevice(dev, true, &usb) < 0) + continue; +
Shouldn't we rather fail in this case? Or, what happens if startupPolicy of the device is set to 'optional'? I think we need to error out here (although, we've probably errored out earlier in the process).
Hi, sorry for the late reply, but I was finally getting some time off for a few days. I intentionally decided not to error out to avoid a new "source" of issues. Compare the two options we have: 1. continue if not finding the device 1.1 likely case we found it, rule will be correct - good 1.2 we don't find it (for whatever reason) - we are "as bad" as before this fix, but not worse 2. error out if not finding the device 2.1 likely case we found it, rule will be correct - good 2.2 we don't find it (for whatever reason) - we are now failing completely
What I don't like about 2.2 is that there might be cases things would have been kind of ok, depsite whatever dark usb magic hit some special setup. In those cases if we error out we add a new chance to fail. And as there are often too many unknowns, so I chose the safer option.
I agree with your assessment and simply continuing if not found rather than erroring. Patch LGTM. +1 Thanks for chasing this one down. -- Jamie Strandboge | http://www.canonical.com
On Wed, Oct 25, 2017 at 8:48 PM, Jamie Strandboge <jamie@canonical.com> wrote:
On Wed, 2017-09-20 at 16:59 +0200, Christian Ehrhardt wrote:
libvirt allows spaces in vm names, there were issues in the past but it seems not removed so the assumption has to be that spaces are continuing to be allowed.
Therefore virt-aa-helper should not reject spaces in vm names anymore if it is goign to be refused causing issues then the parser or xml schema should do so. Apparmor rules are in quotes, so a space in a path based on the name works.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/virt-aa-helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa- helper.c index d1518ea..5f4519d 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -449,7 +449,7 @@ valid_name(const char *name) { /* just try to filter out any dangerous characters in the name that can be * used to subvert the profile */ - const char *bad = " /[]*"; + const char *bad = "/[]*";
if (strlen(name) == 0) return -1;
Your justification seems reasonable. It does mean that we'll need always quote rules that use def->name and looking at virt-aa-helper.c, that seems to be the case.
All that said, I was surprised that tests/virt-aa-helper-test didn't need to be updated, but, indeed, this is a testing gap.
+1 as is, but perhaps in a follow-up patch you could expand bad to be:
const char *bad = "/[]{}?^,\"*";
Yep, makes sense. Found a (minor) cleanup along the way - patches will follow shortly as new thread (no need to splice it into this thread I think)
'{', '}', '?', '^', ',' and '"' are characters used in AARE (see 'Globbing' in 'man apparmor.d') and add tests to tests/virt-aa-helper- test for this.
Thanks!
-- Jamie Strandboge | http://www.canonical.com
-- Christian Ehrhardt Software Engineer, Ubuntu Server Canonical Ltd
participants (3)
-
Christian Ehrhardt -
Jamie Strandboge -
Michal Privoznik