[libvirt] [PATCH 1/4] virt-aa-helper: fix paths for usb hostdevs

[Jamie Strandboge]

On Tue, 2017-10-17 at 09:04 +0200, Christian Ehrhardt wrote:
> On Fri, Sep 29, 2017 at 4:58 PM, Michal Privoznik <mprivozn at 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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20171025/a422684c/attachment-0001.sig>