<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Sep 29, 2017 at 4:58 PM, Michal Privoznik <span dir="ltr"><<a href="mailto:mprivozn@redhat.com" target="_blank">mprivozn@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-HOEnZb"><div class="gmail-h5">On 09/20/2017 04:59 PM, Christian Ehrhardt wrote:<br>
> If users only specified vendor&product (the common case) then parsing<br>
> the xml via virDomainHostdevSubsysUSBDefPa<wbr>rseXML would only set these.<br>
> Bus and Device would much later be added when the devices are prepared<br>
> to be added.<br>
><br>
> Due to that a hot-add of a usb hostdev works as the device is prepared<br>
> and virt-aa-helper processes the new internal xml. But on an initial<br>
> guest start at the time virt-aa-helper renders the apparmor rules the<br>
> bus/device id's are not set yet:<br>
><br>
> p ctl->def->hostdevs[0]->source.<wbr>subsys.u.usb<br>
> $12 = {autoAddress = false, bus = 0, device = 0, vendor = 1921, product<br>
> = 21888}<br>
><br>
> That causes rules to be wrong:<br>
> "/dev/bus/usb/000/000" rw,<br>
><br>
> The fix calls virHostdevFindUSBDevice after reading the XML from<br>
> irt-aa-helper to only add apparmor rules for devices that could be found<br>
> and now are fully known to be able to write the rule correctly.<br>
><br>
> It uncondtionally sets virHostdevFindUSBDevice mandatory attribute as<br>
> adding an apparmor rule for a device not found makes no sense no matter<br>
> what startup policy it has set.<br>
><br>
> Signed-off-by: Christian Ehrhardt <<a href="mailto:christian.ehrhardt@canonical.com">christian.ehrhardt@canonical.<wbr>com</a>><br>
> ---<br>
> src/security/virt-aa-helper.c | 4 ++++<br>
> 1 file changed, 4 insertions(+)<br>
><br>
> diff --git a/src/security/virt-aa-helper.<wbr>c b/src/security/virt-aa-helper.<wbr>c<br>
> index 7944dc1..d1518ea 100644<br>
> --- a/src/security/virt-aa-helper.<wbr>c<br>
> +++ b/src/security/virt-aa-helper.<wbr>c<br>
> @@ -55,6 +55,7 @@<br>
> #include "virrandom.h"<br>
> #include "virstring.h"<br>
> #include "virgettext.h"<br>
> +#include "virhostdev.h"<br>
><br>
> #include "storage/storage_source.h"<br>
><br>
> @@ -1069,6 +1070,9 @@ get_files(vahControl * ctl)<br>
> if (usb == NULL)<br>
> continue;<br>
><br>
> + if (virHostdevFindUSBDevice(dev, true, &usb) < 0)<br>
> + continue;<br>
> +<br>
<br>
</div></div>Shouldn't we rather fail in this case? Or, what happens if startupPolicy<br>
of the device is set to 'optional'? I think we need to error out here<br>
(although, we've probably errored out earlier in the process).<br></blockquote><div><br></div><div>Hi,</div><div>sorry for the late reply, but I was finally getting some time off for a few days.</div><div>I intentionally decided not to error out to avoid a new "source" of issues.</div><div>Compare the two options we have:</div><div>1. continue if not finding the device</div><div> 1.1 likely case we found it, rule will be correct - good</div><div> 1.2 we don't find it (for whatever reason) - we are "as bad" as before this fix, but not worse</div><div>2. error out if not finding the device</div><div> 2.1 likely case we found it, rule will be correct - good</div><div> 2.2 we don't find it (for whatever reason) - we are now failing completely</div><div><br></div><div>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.</div><div>In those cases if we error out we add a new chance to fail.</div><div>And as there are often too many unknowns, so I chose the safer option.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
ACK to the rest of the patches (after some typo clean up, esp. in the<br>
commit messages).<span class="gmail-HOEnZb"><font color="#888888"><br></font></span></blockquote><div><br></div><div>Thanks,</div><div>do you want me to clean up commit messages or will somebody do (to his preferred style) on accepting the commit?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-HOEnZb"><font color="#888888">
Michal<br>
</font></span></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature"><div dir="ltr"><div><div dir="ltr"><span style="color:rgb(136,136,136);font-size:12.8px">Christian Ehrhardt</span><div style="color:rgb(136,136,136);font-size:12.8px">Software Engineer, Ubuntu Server</div><div style="color:rgb(136,136,136);font-size:12.8px">Canonical Ltd</div></div></div></div></div>
</div></div>