From: Christian Ehrhardt <christian.ehrhardt@canonical.com>

For the handling of usb we already allow plenty of read access,
but so far /sys/bus/usb/devices only needed read access to the directory
to enumerate the symlinks in there that point to the actual entries via
relative links to ../../../devices/.

But in more recent systemd with updated libraries a program might do
getattr calls on those symlinks. And while symlinks in apparmor usually
do not matter, as it is the effective target of an access that has to be
allowed, here the getattr calls are on the links themselves.

On USB hostdev usage that causes a set of denials like:
 apparmor="DENIED" operation="getattr" class="file"
 name="/sys/bus/usb/devices/usb1" comm="qemu-system-x86"
 requested_mask="r" denied_mask="r" ...

It is safe to read the links, therefore add a rule to allow it to
the block of rules that covers the usb related access.

Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
---
 src/security/apparmor/libvirt-qemu | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/security/apparmor/libvirt-qemu b/src/security/apparmor/libvirt-qemu
index 02ee273e7e..d0289b8943 100644
--- a/src/security/apparmor/libvirt-qemu
+++ b/src/security/apparmor/libvirt-qemu
@@ -42,6 +42,7 @@
 
   # For hostdev access. The actual devices will be added dynamically
   /sys/bus/usb/devices/ r,
+  /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/+usb* r,
-- 
2.38.1

On 11/17/22 09:42, christian.ehrhardt@canonical.com wrote:
> From: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> 
> For the handling of usb we already allow plenty of read access,
> but so far /sys/bus/usb/devices only needed read access to the directory
> to enumerate the symlinks in there that point to the actual entries via
> relative links to ../../../devices/.
> 
> But in more recent systemd with updated libraries a program might do
> getattr calls on those symlinks. And while symlinks in apparmor usually
> do not matter, as it is the effective target of an access that has to be
> allowed, here the getattr calls are on the links themselves.
> 
> On USB hostdev usage that causes a set of denials like:
>  apparmor="DENIED" operation="getattr" class="file"
>  name="/sys/bus/usb/devices/usb1" comm="qemu-system-x86"
>  requested_mask="r" denied_mask="r" ...
> 
> It is safe to read the links, therefore add a rule to allow it to
> the block of rules that covers the usb related access.
> 
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> ---
>  src/security/apparmor/libvirt-qemu | 1 +
>  1 file changed, 1 insertion(+)
> 

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal

On Mon, Nov 21, 2022 at 4:51 PM Michal Prívozník <mprivozn@redhat.com> wrote:
>
> On 11/17/22 09:42, christian.ehrhardt@canonical.com wrote:
> > From: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> >
> > For the handling of usb we already allow plenty of read access,
> > but so far /sys/bus/usb/devices only needed read access to the directory
> > to enumerate the symlinks in there that point to the actual entries via
> > relative links to ../../../devices/.
> >
> > But in more recent systemd with updated libraries a program might do
> > getattr calls on those symlinks. And while symlinks in apparmor usually
> > do not matter, as it is the effective target of an access that has to be
> > allowed, here the getattr calls are on the links themselves.
> >
> > On USB hostdev usage that causes a set of denials like:
> >  apparmor="DENIED" operation="getattr" class="file"
> >  name="/sys/bus/usb/devices/usb1" comm="qemu-system-x86"
> >  requested_mask="r" denied_mask="r" ...
> >
> > It is safe to read the links, therefore add a rule to allow it to
> > the block of rules that covers the usb related access.
> >
> > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> > ---
> >  src/security/apparmor/libvirt-qemu | 1 +
> >  1 file changed, 1 insertion(+)
> >
>
> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Thank you for having a look, we are not yet in the 8.10 freeze and the
case is rather straightforward, therefore I have pushed it now.

> Michal
>


-- 
Christian Ehrhardt
Senior Staff Engineer, Ubuntu Server
Canonical Ltd

On 11/22/22 09:47, Christian Ehrhardt wrote:
> On Mon, Nov 21, 2022 at 4:51 PM Michal Prívozník <mprivozn@redhat.com> wrote:
>>
>> On 11/17/22 09:42, christian.ehrhardt@canonical.com wrote:
>>> From: Christian Ehrhardt <christian.ehrhardt@canonical.com>
>>>
>>> For the handling of usb we already allow plenty of read access,
>>> but so far /sys/bus/usb/devices only needed read access to the directory
>>> to enumerate the symlinks in there that point to the actual entries via
>>> relative links to ../../../devices/.
>>>
>>> But in more recent systemd with updated libraries a program might do
>>> getattr calls on those symlinks. And while symlinks in apparmor usually
>>> do not matter, as it is the effective target of an access that has to be
>>> allowed, here the getattr calls are on the links themselves.
>>>
>>> On USB hostdev usage that causes a set of denials like:
>>>  apparmor="DENIED" operation="getattr" class="file"
>>>  name="/sys/bus/usb/devices/usb1" comm="qemu-system-x86"
>>>  requested_mask="r" denied_mask="r" ...
>>>
>>> It is safe to read the links, therefore add a rule to allow it to
>>> the block of rules that covers the usb related access.
>>>
>>> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
>>> ---
>>>  src/security/apparmor/libvirt-qemu | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>
>> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
> 
> Thank you for having a look, we are not yet in the 8.10 freeze and the
> case is rather straightforward, therefore I have pushed it now.

Perfect. But just to clarify what freeze means: it's a period where we
try to stabilize for the release. E.g. I run various (hand) tests, try
more exotic operations that I don't do daily. Now, should we find a bug,
merging its fix is very much desired. But merging a feature is less so,
because that usually comes with introducing a bug. Therefore, merging
patches that fix a bug (like this case) is perfectly okay.

Now, there are of course subtle nuances: e.g. merging an aggressive, say
hundred lines of code long bugfix in -rc2 for a theoretical bug is less
desirable. Similarly, a small feature (say a completer to a virsh
command) that's very well understood could be merged if reviewer states
that explicitly "reviewed by and safe for freeze". But of course,
there's no harm merging the feature after the release. It's gotten way
better since we switched to time based releases. Freeze is short and
thus the worst that can happen is the patch is merged after a week.

#morningCoffeeThoughts

Michal

On Tue, Nov 22, 2022 at 9:55 AM Michal Prívozník <mprivozn@redhat.com> wrote:
>
> On 11/22/22 09:47, Christian Ehrhardt wrote:
> > On Mon, Nov 21, 2022 at 4:51 PM Michal Prívozník <mprivozn@redhat.com> wrote:
> >>
> >> On 11/17/22 09:42, christian.ehrhardt@canonical.com wrote:
> >>> From: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> >>>
> >>> For the handling of usb we already allow plenty of read access,
> >>> but so far /sys/bus/usb/devices only needed read access to the directory
> >>> to enumerate the symlinks in there that point to the actual entries via
> >>> relative links to ../../../devices/.
> >>>
> >>> But in more recent systemd with updated libraries a program might do
> >>> getattr calls on those symlinks. And while symlinks in apparmor usually
> >>> do not matter, as it is the effective target of an access that has to be
> >>> allowed, here the getattr calls are on the links themselves.
> >>>
> >>> On USB hostdev usage that causes a set of denials like:
> >>>  apparmor="DENIED" operation="getattr" class="file"
> >>>  name="/sys/bus/usb/devices/usb1" comm="qemu-system-x86"
> >>>  requested_mask="r" denied_mask="r" ...
> >>>
> >>> It is safe to read the links, therefore add a rule to allow it to
> >>> the block of rules that covers the usb related access.
> >>>
> >>> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> >>> ---
> >>>  src/security/apparmor/libvirt-qemu | 1 +
> >>>  1 file changed, 1 insertion(+)
> >>>
> >>
> >> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
> >
> > Thank you for having a look, we are not yet in the 8.10 freeze and the
> > case is rather straightforward, therefore I have pushed it now.
>
> Perfect. But just to clarify what freeze means: it's a period where we
> try to stabilize for the release. E.g. I run various (hand) tests, try
> more exotic operations that I don't do daily. Now, should we find a bug,
> merging its fix is very much desired. But merging a feature is less so,
> because that usually comes with introducing a bug. Therefore, merging
> patches that fix a bug (like this case) is perfectly okay.

Thanks for letting me know, I'm active in too many projects with
different definitions in each that I usually try to stay on the safe
side :-)
But it is good to know and I realize that my usual contributions
(which are mostly fixes) would be fine even at that time.

Thanks Michal!

> Now, there are of course subtle nuances: e.g. merging an aggressive, say
> hundred lines of code long bugfix in -rc2 for a theoretical bug is less
> desirable. Similarly, a small feature (say a completer to a virsh
> command) that's very well understood could be merged if reviewer states
> that explicitly "reviewed by and safe for freeze". But of course,
> there's no harm merging the feature after the release. It's gotten way
> better since we switched to time based releases. Freeze is short and
> thus the worst that can happen is the patch is merged after a week.
>
> #morningCoffeeThoughts
>
> Michal
>


-- 
Christian Ehrhardt
Senior Staff Engineer, Ubuntu Server
Canonical Ltd