With libpmem support compiled into qemu it will trigger the following
denials on every startup.
  apparmor="DENIED" operation="open" name="/"
  apparmor="DENIED" operation="open" name="/sys/bus/nd/devices/"

This is due to [1] that tries to auto-detect if the platform supports
auto flush for all region.

Once we know all the paths that are potentially needed if this feature
is really used we can add them conditionally in virt-aa-helper and labelling
calls in case </pmem> is enabled.

But until then the change here silences the denial warnings seen above.

[1]: https://github.com/pmem/pmdk/blob/master/src/libpmem2/auto_flush_linux.c#L131

Bug: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1871354

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

diff --git a/src/security/apparmor/libvirt-qemu b/src/security/apparmor/libvirt-qemu
index 80986aec61..602f5eb587 100644
--- a/src/security/apparmor/libvirt-qemu
+++ b/src/security/apparmor/libvirt-qemu
@@ -227,3 +227,8 @@
   # required for sasl GSSAPI plugin
   /etc/gss/mech.d/ r,
   /etc/gss/mech.d/* r,
+
+  # scanned on libpmem init, but harmless on any lsb compliant system
+  / r,
+  /sys/bus/nd/devices/ r,
+  /sys/bus/nd/devices/* r,
-- 
2.26.0

On Wed, 08 Apr 2020, Christian Ehrhardt wrote:

> With libpmem support compiled into qemu it will trigger the following
> denials on every startup.
>   apparmor="DENIED" operation="open" name="/"
>   apparmor="DENIED" operation="open" name="/sys/bus/nd/devices/"
> 
> This is due to [1] that tries to auto-detect if the platform supports
> auto flush for all region.
> 
> Once we know all the paths that are potentially needed if this feature
> is really used we can add them conditionally in virt-aa-helper and labelling
> calls in case </pmem> is enabled.
> 
> But until then the change here silences the denial warnings seen above.
> 
> [1]: https://github.com/pmem/pmdk/blob/master/src/libpmem2/auto_flush_linux.c#L131
> 
> Bug: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1871354
> 
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> ---
>  src/security/apparmor/libvirt-qemu | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/src/security/apparmor/libvirt-qemu b/src/security/apparmor/libvirt-qemu
> index 80986aec61..602f5eb587 100644
> --- a/src/security/apparmor/libvirt-qemu
> +++ b/src/security/apparmor/libvirt-qemu
> @@ -227,3 +227,8 @@
>    # required for sasl GSSAPI plugin
>    /etc/gss/mech.d/ r,
>    /etc/gss/mech.d/* r,
> +
> +  # scanned on libpmem init, but harmless on any lsb compliant system
> +  / r,

I suggest adjusting the comment for clarity. Eg:

  # required by libpmem init
  / r, # harmless on any lsb compliant system
  /sys/bus/nd/devices/ r,
  ...

The '/' read is indeed fine.

> +  /sys/bus/nd/devices/ r,

This also is fine.

> +  /sys/bus/nd/devices/* r,

Can you list what files libpem init is looking at? I'm a bit
uncomfortable with the glob here and would rather not guess that today's
and all future files in /sys/bus/nd/devices are safe for all qemu
processes to read.

-- 
Jamie Strandboge             | http://www.canonical.com

On Wed, 08 Apr 2020, Jamie Strandboge wrote:

> On Wed, 08 Apr 2020, Christian Ehrhardt wrote:
> 
> > With libpmem support compiled into qemu it will trigger the following
> > denials on every startup.
> >   apparmor="DENIED" operation="open" name="/"
> >   apparmor="DENIED" operation="open" name="/sys/bus/nd/devices/"
> > 
> > This is due to [1] that tries to auto-detect if the platform supports
> > auto flush for all region.
> > 
> > Once we know all the paths that are potentially needed if this feature
> > is really used we can add them conditionally in virt-aa-helper and labelling
> > calls in case </pmem> is enabled.
> > 
> > But until then the change here silences the denial warnings seen above.
> > 
> > [1]: https://github.com/pmem/pmdk/blob/master/src/libpmem2/auto_flush_linux.c#L131
> > 
> > Bug: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1871354
> > 
> > +  /sys/bus/nd/devices/* r,
> 
> Can you list what files libpem init is looking at? I'm a bit
> uncomfortable with the glob here and would rather not guess that today's
> and all future files in /sys/bus/nd/devices are safe for all qemu
> processes to read.

Answering myself after looking at the pmdk source code, fs_new() is
calling fts_open() without FTS_NOCHDIR (and based on your '/' rule,
starting in '/'), then calls fs_read() which calls fts_read() on the
files it finds in /sys/bus/nd/devices (it makes sure to only look at
symlinks, but that doesn't impact our rules). Writing some test code to
simulate this and testing on /sys/bus/usb/devices (since I have usb
devices here, but not nd and this dir is populated with symlinks as the
libpmem code expects), I think the full rules you want are:

# required by libpmem init to fts_open()/fts_read() the symlinks in
# /sys/bus/nd/devices
/ r,
/sys/bus/nd/devices/{,**/} r,

Ideally this access would only be needed if using NFIT-ND devices, but
as you mentioned, that is not possible at the point of the denial. I
think these rules are fine to apply to the default VM policy since they
are only a collection of directory reads (note the trailing '/'s in the
second rule), which should have no impact guest isolation or host
protections.

-- 
Jamie Strandboge             | http://www.canonical.com

On Wed, Apr 8, 2020 at 9:50 PM Jamie Strandboge <jamie@canonical.com> wrote:
>
> On Wed, 08 Apr 2020, Jamie Strandboge wrote:
>
> > On Wed, 08 Apr 2020, Christian Ehrhardt wrote:
> >
> > > With libpmem support compiled into qemu it will trigger the following
> > > denials on every startup.
> > >   apparmor="DENIED" operation="open" name="/"
> > >   apparmor="DENIED" operation="open" name="/sys/bus/nd/devices/"
> > >
> > > This is due to [1] that tries to auto-detect if the platform supports
> > > auto flush for all region.
> > >
> > > Once we know all the paths that are potentially needed if this feature
> > > is really used we can add them conditionally in virt-aa-helper and labelling
> > > calls in case </pmem> is enabled.
> > >
> > > But until then the change here silences the denial warnings seen above.
> > >
> > > [1]: https://github.com/pmem/pmdk/blob/master/src/libpmem2/auto_flush_linux.c#L131
> > >
> > > Bug: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1871354
> > >
> > > +  /sys/bus/nd/devices/* r,
> >
> > Can you list what files libpem init is looking at? I'm a bit
> > uncomfortable with the glob here and would rather not guess that today's
> > and all future files in /sys/bus/nd/devices are safe for all qemu
> > processes to read.
>
> Answering myself after looking at the pmdk source code, fs_new() is
> calling fts_open() without FTS_NOCHDIR (and based on your '/' rule,
> starting in '/'), then calls fs_read() which calls fts_read() on the
> files it finds in /sys/bus/nd/devices (it makes sure to only look at
> symlinks, but that doesn't impact our rules). Writing some test code to
> simulate this and testing on /sys/bus/usb/devices (since I have usb
> devices here, but not nd and this dir is populated with symlinks as the
> libpmem code expects), I think the full rules you want are:
>
> # required by libpmem init to fts_open()/fts_read() the symlinks in
> # /sys/bus/nd/devices
> / r,
> /sys/bus/nd/devices/{,**/} r,

Thanks for your help to make this less of a wildcard.

Since this is qemu startup (without any special config) I tested it and it
works with these improved rules as well, triggering no Denial.

I'll submit a V2 with the adapted rules and comments

If anyone is interested to fake some nvdimms you can take a look at [1],
but as I outlined in the referred bug for the real conditional rules we will
need to check how systems with real devices will look like.

[1]: https://git.launchpad.net/qa-regression-testing/tree/notes_testing/ndctl/README.txt

> Ideally this access would only be needed if using NFIT-ND devices, but
> as you mentioned, that is not possible at the point of the denial. I
> think these rules are fine to apply to the default VM policy since they
> are only a collection of directory reads (note the trailing '/'s in the
> second rule), which should have no impact guest isolation or host
> protections.
>
> --
> Jamie Strandboge             | http://www.canonical.com



-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd