On 11/17/22 09:42, christian.ehrhardt(a)canonical.com wrote: > From: Christian Ehrhardt <christian.ehrhardt(a)canonical.com&gt; > > 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(a)canonical.com&gt; > --- > src/security/apparmor/libvirt-qemu | 1 + > 1 file changed, 1 insertion(+) > Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com&gt;

Michal

On 11/22/22 09:47, Christian Ehrhardt wrote: > On Mon, Nov 21, 2022 at 4:51 PM Michal Prívozník <mprivozn(a)redhat.com&gt; wrote: >> >> On 11/17/22 09:42, christian.ehrhardt(a)canonical.com wrote: >>> From: Christian Ehrhardt <christian.ehrhardt(a)canonical.com&gt; >>> >>> 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(a)canonical.com&gt; >>> --- >>> src/security/apparmor/libvirt-qemu | 1 + >>> 1 file changed, 1 insertion(+) >>> >> >> Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > > 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