On Tue, 2017-10-17 at 09:04 +0200, Christian Ehrhardt wrote:
> On Fri, Sep 29, 2017 at 4:58 PM, Michal Privoznik <mprivozn@xxxxxxxxx
> 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.comAttachment:
signature.asc
Description: This is a digitally signed message part
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
