Re: [PATCH] apparmor: ceph config file names
On Thu, 07 Oct 2021, christian.ehrha...@canonical.com wrote: > From: Christian Ehrhardt > > If running multiple [1] clusters (uncommon) the ceph config file will be > derived from the cluster name. Therefore the rule to allow to read ceph > config files need to be opened up slightly to allow for that condition. > > [1]: > https://docs.ceph.com/en/mimic/rados/configuration/common/#running-multiple-clusters > > Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1588576 > > Signed-off-by: Christian Ehrhardt > --- > src/security/apparmor/libvirt-qemu | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/security/apparmor/libvirt-qemu > b/src/security/apparmor/libvirt-qemu > index 4156428163..8cd76d48ec 100644 > --- a/src/security/apparmor/libvirt-qemu > +++ b/src/security/apparmor/libvirt-qemu > @@ -199,7 +199,7 @@ >/sys/class/ r, > ># for rbd > - /etc/ceph/ceph.conf r, > + /etc/ceph/*.conf r, > ># Various functions will need to enumerate /tmp (e.g. ceph), allow the base ># dir and a few known functions like samba support. LGTM, thanks! -- Email: ja...@strandboge.com IRC: jdstrand signature.asc Description: PGP signature
Re: [PATCH] apparmor: allow kvm-spice compat wrapper
On Tue, 17 Nov 2020, Neal Gompa wrote: > On Tue, Nov 17, 2020 at 11:49 AM Christian Ehrhardt > wrote: > > > > On Mon, Nov 16, 2020 at 3:28 PM Michal Privoznik > > wrote: > > > > > > On 11/16/20 1:26 PM, Christian Ehrhardt wrote: > > > > 'kvm-spice' is a binary name used to call 'kvm' which actually is a > > > > wrapper > > > > around qemu-system-x86_64 enabling kvm acceleration. This isn't in use > > > > for quite a while anymore, but required to work for compatibility e.g. > > > > when migrating in old guests. > > > > > > > > For years this was a symlink kvm-spice->kvm and therefore covered > > > > apparmor-wise by the existing entry: > > > > /usr/bin/kvm rmix, > > > > But due to a recent change [1] in qemu packaging this now is no symlink, > > > > but a wrapper on its own and therefore needs an own entry that allows it > > > > to be executed. > > > > > > > > [1]: https://salsa.debian.org/qemu-team/qemu/-/commit/9944836d3 > > > > > > > > Signed-off-by: Christian Ehrhardt > > > > --- > > > > src/security/apparmor/libvirt-qemu | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > Reviewed-by: Michal Privoznik > > > > Thank you Michal, > > it also passed fine through my tests (as backport to 6.8 and 6.9). > > We are not in any freeze, review has happened, tests LGTM - pushed to git. > > > > Hold up, why was this merged? Did anyone validate whether this would > break the other AppArmor user (SUSE)? > > Unlike SELinux, AppArmor functionality is quite fragmented between > Ubuntu and SUSE distributions (the two major users of AppArmor), and > there did not seem to be any indication that this AppArmor patch was > validated with openSUSE before merging. My personal experience with > AppArmor across the two distribution families is that it's really easy > to make profiles that work for Ubuntu but fail on SUSE because of the > disparity of functionality. I also don't see Jim Fehlig stepping in to > indicate that this worked for him. > > I haven't had a chance to test this myself, but I am immediately > suspicious of a change that references a commit based on Debian > packaging of QEMU. Others have referred to how this list handles SUSE policies, but I'll point out that the request was for a simple file rule that only adds additional access. This should be no problem at all on SUSE. Outside of this rule, the apparmor userspace understands kernel differences and various rules and any modern SUSE would have a new enough parser to handle the various rules syntax we use in the current libvirt policy and be parseable without issues. The typical distro pattern for new rule syntax would be that when a distro pulled in a new libvirt with new policy syntax that the distro doesn't support, then it would be abundantly clear to the distro maintainer when the parser failed and the distro would either choose to upgrade apparmor or patch out the problematic rules. Hope this helps -- Jamie Strandboge | http://www.canonical.com
Re: [PATCH v2] apparmor: allow libvirtd to call virtiofsd
ibvirt/virt-aa-helper w,
audit deny /usr/{lib,lib64,lib/qemu,libexec}/qemu-bridge-helper w,
(I consider this a hardening measure and not a security bug since the
libvirtd profile is intentionally lenient).
> > ---
> >
> > Changes in v2:
> > - Wrap log and dmesg messages, as requested by Christian Ehrhardt.
> >
> > src/security/apparmor/usr.sbin.libvirtd.in | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/src/security/apparmor/usr.sbin.libvirtd.in
> > b/src/security/apparmor/usr.sbin.libvirtd.in
> > index 4518e8f865..f2030764cd 100644
> > --- a/src/security/apparmor/usr.sbin.libvirtd.in
> > +++ b/src/security/apparmor/usr.sbin.libvirtd.in
> > @@ -89,6 +89,7 @@ profile libvirtd @sbindir@/libvirtd
> > flags=(attach_disconnected) {
> >/usr/lib/xen-*/bin/libxl-save-helper PUx,
> >/usr/lib/xen-*/bin/pygrub PUx,
> >/usr/{lib,lib64,lib/qemu,libexec}/vhost-user-gpu PUx,
> > + /usr/{lib,lib64,lib/qemu,libexec}/virtiofsd PUx,
> >
> ># Required by nwfilter_ebiptables_driver.c:ebiptablesWriteToTempFile() to
> ># read and run an ebtables script.
> > --
> > 2.28.0
> >
>
> --
> Christian Ehrhardt
> Staff Engineer, Ubuntu Server
> Canonical Ltd
>
--
Jamie Strandboge | http://www.canonical.com
Re: [PATCH 8/8] apparmor: let qemu load old shared objects after upgrades
On Mon, 03 Aug 2020, Christian Ehrhardt wrote:
> Since [1] qemu can after upgrade fall back to pre-upgrade modules
> to still be able to dynamically load qmeu-module based features.
>
> The paths for these modules are pre-defined by the code and should
> be allowed to be mapped and loaded from which will allow packagers
> avoiding the inability of late feature load [2] after package upgrades.
>
> [1]: https://github.com/qemu/qemu/commit/bd83c861
> [2]: https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1847361
>
> Signed-off-by: Christian Ehrhardt
> ---
> 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 25eff20b82..c6f7149799 100644
> --- a/src/security/apparmor/libvirt-qemu
> +++ b/src/security/apparmor/libvirt-qemu
> @@ -168,6 +168,11 @@
>/usr/{lib,lib64}/qemu/*.so mr,
>/usr/lib/@{multiarch}/qemu/*.so mr,
>
> + # let qemu load old shared objects after upgrades (LP: #1847361)
> + /{var/,}run/qemu/*/*.so mr,
> + # but explicitly deny writing to these files
> + audit deny /{var/,}run/qemu/*/*.so w,
> +
+1 to apply
--
Jamie Strandboge | http://www.canonical.com
Re: [PATCH 7/8] apparmor: allow virt-aa-helper to read openvswitch sockets
On Mon, 03 Aug 2020, Christian Ehrhardt wrote:
> From: Serge Hallyn
>
> Chardevs/sockets configured for openvswitch-dpdk use cases
> might be probed by virt-aa-helper. Allow that access to enable
> virt-aa-helper rendering per-guest rules for the actual qemu
> guest accessing these sockets eventually.
>
> Signed-off-by: Christian Ehrhardt
> Signed-off-by: Stefan Bader
> Signed-off-by: Serge Hallyn
> ---
> src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
> b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
> index 3f204799a6..877cb04b1e 100644
> --- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
> +++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
> @@ -46,6 +46,9 @@ profile virt-aa-helper @libexecdir@/virt-aa-helper {
>@sysconfdir@/apparmor.d/libvirt/* r,
>
> @sysconfdir@/apparmor.d/libvirt/libvirt-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*
> rw,
>
> + # for openvswitch sockets
> + /{,var/}run/openvswitch/** rw,
A bit unfortunate and unexpected. What kind of probing does
virt-aa-helper do on these?
--
Jamie Strandboge | http://www.canonical.com
Re: [PATCH 5/8] apparmor: qemu access to @{PROC}/*/auxv for hw_cap
On Mon, 03 Aug 2020, Christian Ehrhardt wrote:
> From: Stefan Bader
>
> On some architectures (ppc, s390x, sparc, arm) qemu will read auxv
> to detect hardware capabilities via qemu_getauxval.
>
> Allow that access read-only for the entry owned by the current
> qemu process.
>
> Signed-off-by: Christian Ehrhardt
> Signed-off-by: Stefan Bader
> ---
> 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 b132cf0226..25eff20b82 100644
> --- a/src/security/apparmor/libvirt-qemu
> +++ b/src/security/apparmor/libvirt-qemu
> @@ -33,6 +33,7 @@
>owner @{PROC}/@{pid}/task/@{tid}/comm rw,
>@{PROC}/sys/kernel/cap_last_cap r,
>@{PROC}/sys/vm/overcommit_memory r,
> + owner @{PROC}/*/auxv r,
+1 to apply. A code comment that is simply the first sentence of
Stefan's commit message might be a nice touch, but that is not a
blocker.
--
Jamie Strandboge | http://www.canonical.com
Re: [PATCH 1/8] apparmor: allow default pki path
On Mon, 03 Aug 2020, Christian Ehrhardt wrote:
> From: Sam Hartman
>
> /etc/pki/qemu is a pki path recommended by qemu tls docs [1]
> and one that can cause issues with spice connections when missing.
>
> Add the path to the allowed list of pki paths to fix the issue.
>
> Note: this is active in Debian/Ubuntu [1] for quite a while already.
>
> [1]: https://www.qemu.org/docs/master/system/tls.html
> [2]: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=930100
>
> Signed-off-by: Christian Ehrhardt
> ---
> src/security/apparmor/libvirt-qemu | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/src/security/apparmor/libvirt-qemu
> b/src/security/apparmor/libvirt-qemu
> index 1a4b226612..2d08d6f7ad 100644
> --- a/src/security/apparmor/libvirt-qemu
> +++ b/src/security/apparmor/libvirt-qemu
> @@ -94,6 +94,8 @@
>/etc/pki/CA/* r,
>/etc/pki/libvirt{,-spice,-vnc}/ r,
>/etc/pki/libvirt{,-spice,-vnc}/** r,
> + /etc/pki/qemu/ r,
> + /etc/pki/qemu/** r,
+1 to apply
--
Jamie Strandboge | http://www.canonical.com
Re: [PATCH 2/8] apparmor: allow libvirtd to call pygrub
On Mon, 03 Aug 2020, Christian Ehrhardt wrote:
> From: Stefan Bader
>
> When using xen through libxl in Debian/Ubuntu it needs to be able to
> call pygrub.
>
> This is placed in a versioned path like /usr/lib/xen-4.11/bin.
> In theory the rule could be more strict by rendering the libexec_dir
> setting pkg-config can derive from libbxen-dev. But that would make
> particular libvirt/xen packages version-depend on each other. It seems
> more reasonable to avoid these versioned dependencies and use a wildcard
> rule instead as it is already in place for libxl-save-helper.
>
> Note: This change was in Debian [1] and Ubuntu [2] for quite some time
> already.
>
> [1]: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=931768
> [2]: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1326003
>
> Signed-off-by: Christian Ehrhardt
> ---
> src/security/apparmor/usr.sbin.libvirtd.in | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/src/security/apparmor/usr.sbin.libvirtd.in
> b/src/security/apparmor/usr.sbin.libvirtd.in
> index 1e137039e9..312fa4b6d1 100644
> --- a/src/security/apparmor/usr.sbin.libvirtd.in
> +++ b/src/security/apparmor/usr.sbin.libvirtd.in
> @@ -86,6 +86,7 @@ profile libvirtd @sbindir@/libvirtd
> flags=(attach_disconnected) {
>/usr/{lib,lib64}/xen-common/bin/xen-toolstack PUx,
>/usr/{lib,lib64}/xen/bin/* Ux,
>/usr/lib/xen-*/bin/libxl-save-helper PUx,
> + /usr/lib/xen-*/bin/pygrub PUx,
LGTM. +1 to apply
--
Jamie Strandboge | http://www.canonical.com
Re: [PATCH 6/8] apparmor: allow virt-aa-helper to read from tmp
On Mon, 03 Aug 2020, Christian Ehrhardt wrote:
> From: Stefan Bader
>
> temporary directories are a common place images are placed by users
> for any sort of quick evaluation. Allow virt-aa-helper access to tmp
> via the existing user-tmp apparmor abstraction.
>
> That way if a guest definition has paths in temporary directories
> virt-aa-helper can properly probe them e.g. for further backing files in
> the case of qcow2.
>
> Signed-off-by: Christian Ehrhardt
> ---
> src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
> b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
> index dfc61e8de4..3f204799a6 100644
> --- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
> +++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
> @@ -3,6 +3,7 @@
> profile virt-aa-helper @libexecdir@/virt-aa-helper {
>#include
>#include
> + #include
user-tmp allows write and all other accesses for disks are read. We have
these rules:
/**.img r,
/**.raw r,
/**.qcow{,2} r,
/**.qed r,
/**.vmdk r,
/**.vhd r,
/**.[iI][sS][oO] r,
/**/disk{,.*} r,
Why are these not sufficient? What was the denial that triggered the
issue?
--
Jamie Strandboge | http://www.canonical.com
Re: [PATCH 4/8] apparmor: read only access to overcommit_memory
On Mon, 03 Aug 2020, Christian Ehrhardt wrote:
> From: Jamie Strandboge
>
> Allow qemu to read @{PROC}/sys/vm/overcommit_memory.
> This is read on guest start-up and (as read-only) not a
> critical secret that has to stay hidden.
>
> Signed-off-by: Christian Ehrhardt
> Signed-off-by: Stefan Bader
> Signed-off-by: Jamie Strandboge
> ---
> 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 2d08d6f7ad..b132cf0226 100644
> --- a/src/security/apparmor/libvirt-qemu
> +++ b/src/security/apparmor/libvirt-qemu
> @@ -32,6 +32,7 @@
># only modify its comm value or those in its thread group.
>owner @{PROC}/@{pid}/task/@{tid}/comm rw,
>@{PROC}/sys/kernel/cap_last_cap r,
> + @{PROC}/sys/vm/overcommit_memory r,
+1 to apply
--
Jamie Strandboge | http://www.canonical.com
Re: [PATCH 3/8] apparmor: allow virt-aa-helper nameservices
On Mon, 03 Aug 2020, Christian Ehrhardt wrote:
> Since quite a while libvirt-aa-helper triggers nss related apparmor
> denials like:
> operation="open" profile="virt-aa-helper" name="/etc/nsswitch.conf"
> operation="open" profile="virt-aa-helper" name="/etc/host.conf"
> operation="open" profile="virt-aa-helper" name="/etc/resolv.conf"
> operation="open" profile="virt-aa-helper" name="/etc/hosts"
>
> Rules to allow these are in Debian [1] / Ubuntu [2] since quite a
> while but do not seem to be specific to those distributions.
>
> There can be much more reasons than one would think to inadvertently
> use/trigger nameservices as can be seen in the comments in
> profiles/apparmor.d/abstractions/nameservice at [3].
> The nameservices abstraction provides a nice and upgrade safe
> way to cover all of them.
>
> [1]: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=882979
> [2]: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1546674
> [3]: https://gitlab.com/apparmor/apparmor
>
> Signed-off-by: Christian Ehrhardt
> ---
> src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
> b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
> index dd18c8ab89..dfc61e8de4 100644
> --- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
> +++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
> @@ -2,6 +2,7 @@
>
> profile virt-aa-helper @libexecdir@/virt-aa-helper {
>#include
> + #include
nameservice brings in network rules so this is actually a lot of access.
Why is it reaching out to nss? Is it just cause some library happens to
look at /etc/nsswitch.conf and pull in other things or does it actually
need networking? I suspect the former. If my suspicion is true, perhaps
instead:
# virt-aa-helper dependent libraries read (and if successful, other
# files) but virt-aa-helper itself doesn't require the access, so
# silence the denial.
deny /etc/nsswitch.conf r,
--
Jamie Strandboge | http://www.canonical.com
Re: [PATCH v2] apparmor: avoid denials on libpmem initialization
On Thu, 09 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 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
> ---
> 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..1a4b226612 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,
> +
> + # required by libpmem init to fts_open()/fts_read() the symlinks in
> + # /sys/bus/nd/devices
> + / r, # harmless on any lsb compliant system
> + /sys/bus/nd/devices/{,**/} r,
LGTM. Thanks!
--
Jamie Strandboge | http://www.canonical.com
Re: [PATCH] apparmor: avoid denials on libpmem initialization
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 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
Re: [PATCH] apparmor: avoid denials on libpmem initialization
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 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 > --- > 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
Re: [PATCH 1/6] apparmor: Fix parthelper, iohelper and virt-aa-helper paths in profiles
On Wed, 29 Jan 2020, Michal Privoznik wrote:
> On 1/27/20 5:30 PM, Jamie Strandboge wrote:
> > On Sat, 25 Jan 2020, Michal Privoznik wrote:
> >
> > > These helper binaries are installed under libexec dir not lib
> > > dir.
> > >
> > > Signed-off-by: Michal Privoznik
> > > ---
> > > src/security/apparmor/usr.lib.libvirt.virt-aa-helper | 2 +-
> > > src/security/apparmor/usr.sbin.libvirtd | 4 ++--
> > > 2 files changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper
> > > b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper
> > > index 11e9c039ca..504c70e0ce 100644
> > > --- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper
> > > +++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper
> > > @@ -39,7 +39,7 @@ profile virt-aa-helper
> > > /usr/{lib,lib64}/libvirt/virt-aa-helper {
> > > deny /dev/mapper/ r,
> > > deny /dev/mapper/* r,
> > > - /usr/{lib,lib64}/libvirt/virt-aa-helper mr,
> > > + /usr/libexec/virt-aa-helper mr,
> >
> > I suggest you use this instead here and in the rest of the patch series:
> >
> > /usr/{lib,lib64,libexec}/libvirt/virt-aa-helper mr,
> >
> > since it will let existing installs to continue to work.
>
> You mean some downstream has installed virt-aa-helper into /usr/lib or
> /usr/lib64? Because the upstream install rule says /usr/libexec/.
Well, I was thinking the rule was what it was for a reason, so with my
distro hat on, changing it to just libexec sounded like a potential
pain point for upgraders. I also understand that the policy is intended
as example policy that distros can adjust as needed, so perhaps it is ok
to cut straight to libexec in this patchset... I don't have objections
if you prefer to keep it as is.
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: PGP signature
Re: [PATCH 4/6] apparmor: Rename virt-aa-helper profile
On Sat, 25 Jan 2020, Michal Privoznik wrote:
> The profile name should reflect the path under which the binary
> it describes is installed.
>
> Signed-off-by: Michal Privoznik
> ---
> src/security/Makefile.inc.am | 10 +-
> ...bvirt.virt-aa-helper => usr.libexec.virt-aa-helper} | 4 ++--
> 2 files changed, 7 insertions(+), 7 deletions(-)
> rename src/security/apparmor/{usr.lib.libvirt.virt-aa-helper =>
> usr.libexec.virt-aa-helper} (93%)
>
> diff --git a/src/security/Makefile.inc.am b/src/security/Makefile.inc.am
> index 6fe9d50f29..02efefd6d6 100644
> --- a/src/security/Makefile.inc.am
> +++ b/src/security/Makefile.inc.am
> @@ -38,7 +38,7 @@ EXTRA_DIST += \
> security/apparmor/TEMPLATE.lxc \
> security/apparmor/libvirt-qemu \
> security/apparmor/libvirt-lxc \
> - security/apparmor/usr.lib.libvirt.virt-aa-helper \
> + security/apparmor/usr.libexec.virt-aa-helper \
> security/apparmor/usr.sbin.libvirtd \
> $(NULL)
>
> @@ -91,7 +91,7 @@ endif WITH_SECDRIVER_APPARMOR
> if WITH_APPARMOR_PROFILES
> apparmordir = $(sysconfdir)/apparmor.d/
> apparmor_DATA = \
> - security/apparmor/usr.lib.libvirt.virt-aa-helper \
> + security/apparmor/usr.libexec.virt-aa-helper \
> security/apparmor/usr.sbin.libvirtd \
> $(NULL)
>
> @@ -111,11 +111,11 @@ APPARMOR_LOCAL_DIR = "$(DESTDIR)$(apparmordir)/local"
> install-apparmor-local:
> $(MKDIR_P) "$(APPARMOR_LOCAL_DIR)"
> echo "# Site-specific additions and overrides for \
> - 'usr.lib.libvirt.virt-aa-helper'" \
> - >"$(APPARMOR_LOCAL_DIR)/usr.lib.libvirt.virt-aa-helper"
> + 'usr.libexec.virt-aa-helper'" \
> + >"$(APPARMOR_LOCAL_DIR)/usr.libexec.virt-aa-helper"
>
> uninstall-apparmor-local:
> - rm -f "$(APPARMOR_LOCAL_DIR)/usr.lib.libvirt.virt-aa-helper"
> + rm -f "$(APPARMOR_LOCAL_DIR)/usr.libexec.virt-aa-helper"
> rmdir "$(APPARMOR_LOCAL_DIR)" || :
>
> INSTALL_DATA_LOCAL += install-apparmor-local
> diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper
> b/src/security/apparmor/usr.libexec.virt-aa-helper
> similarity index 93%
> rename from src/security/apparmor/usr.lib.libvirt.virt-aa-helper
> rename to src/security/apparmor/usr.libexec.virt-aa-helper
> index 504c70e0ce..25754037e1 100644
> --- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper
> +++ b/src/security/apparmor/usr.libexec.virt-aa-helper
> @@ -1,7 +1,7 @@
> # Last Modified: Mon Apr 5 15:10:27 2010
> #include
>
> -profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper {
> +profile virt-aa-helper /usr/libexec/virt-aa-helper {
I suggest using this for the previous reasons:
profile virt-aa-helper /usr/{lib,lib64,libexec}/libvirt/virt-aa-helper {
The filename rename is fine though (the filename doesn't have to match
the profile name or binary attachment, so picking what we expect to be
the normal use case is fine).
>#include
>
># needed for searching directories
> @@ -70,5 +70,5 @@ profile virt-aa-helper
> /usr/{lib,lib64}/libvirt/virt-aa-helper {
>/**.[iI][sS][oO] r,
>/**/disk{,.*} r,
>
> - #include
> + #include
> }
> --
> 2.24.1
>
--
Jamie Strandboge | http://www.canonical.com
Re: [PATCH 6/6] apparmor: Allow some more BIOS/UEFI paths
On Sat, 25 Jan 2020, Michal Privoznik wrote: > There are two more paths that we are missing in the default > domain profile: /usr/share/edk2-ovmf/ and /usr/share/sgabios/. > These exist on my Gentoo box and contain UEFI and BIOS images > respectively. > > Signed-off-by: Michal Privoznik > --- > src/security/apparmor/libvirt-qemu | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/src/security/apparmor/libvirt-qemu > b/src/security/apparmor/libvirt-qemu > index 2291829270..6942b83969 100644 > --- a/src/security/apparmor/libvirt-qemu > +++ b/src/security/apparmor/libvirt-qemu > @@ -75,6 +75,7 @@ ># access to firmware's etc >/usr/share/AAVMF/** r, >/usr/share/bochs/** r, > + /usr/share/edk2-ovmf/** r, >/usr/share/kvm/** r, >/usr/share/misc/sgabios.bin r, >/usr/share/openbios/** r, > @@ -86,6 +87,7 @@ >/usr/share/qemu-kvm/** r, >/usr/share/qemu/** r, >/usr/share/seabios/** r, > + /usr/share/sgabios/** r, > /usr/share/slof/** r, >/usr/share/vgabios/** r, +1 to apply -- Jamie Strandboge | http://www.canonical.com
Re: [PATCH 3/6] docs: Fix virt-aa-helper location
On Sat, 25 Jan 2020, Michal Privoznik wrote: > The location of virt-aa-helper shown in the docs is incorrect. > The helper binary is installed under libexec dir. > > Signed-off-by: Michal Privoznik > --- > docs/drvqemu.html.in | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/docs/drvqemu.html.in b/docs/drvqemu.html.in > index 8beb28655c..60d4352556 100644 > --- a/docs/drvqemu.html.in > +++ b/docs/drvqemu.html.in > @@ -340,7 +340,7 @@ chmod o+x /path/to/directory > >While users can define their own AppArmor profile scheme, a typical >configuration will include a profile for > /usr/sbin/libvirtd, > - /usr/lib/libvirt/virt-aa-helper (a helper program which > the > + /usr/libexec/virt-aa-helper (a helper program which the >libvirtd daemon uses instead of manipulating AppArmor directly), and >an abstraction to be included by > /etc/apparmor.d/libvirt/TEMPLATE >(typically /etc/apparmor.d/abstractions/libvirt-qemu). +1 to apply -- Jamie Strandboge | http://www.canonical.com
Re: [PATCH 5/6] apparmor: Sort paths in blocks in libvirt-qemu profile
in/qemu-mipsn32el rmix, > - /usr/bin/qemu-or32 rmix, > - /usr/bin/qemu-ppc rmix, > - /usr/bin/qemu-ppc64 rmix, > - /usr/bin/qemu-ppc64abi32 rmix, > - /usr/bin/qemu-ppc64le rmix, > - /usr/bin/qemu-s390x rmix, > - /usr/bin/qemu-sh4 rmix, > - /usr/bin/qemu-sh4eb rmix, > - /usr/bin/qemu-sparc rmix, > - /usr/bin/qemu-sparc32plus rmix, > - /usr/bin/qemu-sparc64 rmix, >/usr/bin/qemu-unicore32 rmix, >/usr/bin/qemu-x86_64 rmix, ># for Debian/Ubuntu qemu-block-extra / RPMs qemu-block-* (LP: #1554761) +1 to apply -- Jamie Strandboge | http://www.canonical.com
Re: [PATCH 1/6] apparmor: Fix parthelper, iohelper and virt-aa-helper paths in profiles
On Sat, 25 Jan 2020, Michal Privoznik wrote:
> These helper binaries are installed under libexec dir not lib
> dir.
>
> Signed-off-by: Michal Privoznik
> ---
> src/security/apparmor/usr.lib.libvirt.virt-aa-helper | 2 +-
> src/security/apparmor/usr.sbin.libvirtd | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper
> b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper
> index 11e9c039ca..504c70e0ce 100644
> --- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper
> +++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper
> @@ -39,7 +39,7 @@ profile virt-aa-helper
> /usr/{lib,lib64}/libvirt/virt-aa-helper {
>deny /dev/mapper/ r,
>deny /dev/mapper/* r,
>
> - /usr/{lib,lib64}/libvirt/virt-aa-helper mr,
> + /usr/libexec/virt-aa-helper mr,
I suggest you use this instead here and in the rest of the patch series:
/usr/{lib,lib64,libexec}/libvirt/virt-aa-helper mr,
since it will let existing installs to continue to work.
>/{usr/,}sbin/apparmor_parser Ux,
>
>/etc/apparmor.d/libvirt/* r,
> diff --git a/src/security/apparmor/usr.sbin.libvirtd
> b/src/security/apparmor/usr.sbin.libvirtd
> index 29f9936ad9..2089ba1b3e 100644
> --- a/src/security/apparmor/usr.sbin.libvirtd
> +++ b/src/security/apparmor/usr.sbin.libvirtd
> @@ -100,8 +100,8 @@ profile libvirtd /usr/sbin/libvirtd
> flags=(attach_disconnected) {
>audit deny /sys/kernel/security/apparmor/.* rwxl,
>/sys/kernel/security/apparmor/profiles r,
>/usr/{lib,lib64}/libvirt/* PUxr,
> - /usr/{lib,lib64}/libvirt/libvirt_parthelper ix,
> - /usr/{lib,lib64}/libvirt/libvirt_iohelper ix,
> + /usr/libexec/libvirt_parthelper ix,
> + /usr/libexec/libvirt_iohelper ix,
>/etc/libvirt/hooks/** rmix,
>/etc/xen/scripts/** rmix,
>
> --
> 2.24.1
>
--
Jamie Strandboge | http://www.canonical.com
Re: [libvirt] [PATCH v2 3/3] virt-aa-helper: drop pointer checks in get_files
On Wed, 20 Nov 2019, Cole Robinson wrote: > On 11/19/19 4:31 PM, Jamie Strandboge wrote: > > On Thu, 14 Nov 2019, Christian Ehrhardt wrote: > > > >> It was mentioned that the pointers in loops like: > >> for (i = 0; i < ctl->def->nserials; i++) > >> if (ctl->def->serials[i] ... > >> will always be !NULL or we would have a much more serious problem. > >> > >> Simplify the if chains in get_files by dropping these checks. > > > > I don't really see why a NULL check is a problem so this feels a bit > > like busy work and it seems short-circuiting and not doing is exactly > > what you would want to do in the event of a 'much more serious problem'. > > Is this really required? +0 to apply on principle (I've not reviewed the > > changes closely). > > > > If for example def->nserials and def->serials[i] disagree, then there is > a serious bug somewhere. The internal API contract is that it should > never happen, so code shouldn't check for the condition. I'm pretty sure > if the XML parser is creating that situation, the qemu driver would > crash in a dozen different places. > > So the patch isn't strictly required, but it is an improvement IMO: it > reduces code, improves readability, and helps simplify review for other > libvirt devs who may be confused by this uncommon idiom (I was). But I > will leave it up to you guys to decide whether to push or not To be clear, I am intentionally not blocking on this. If, as upstream, you'd prefer this to be a certain way for reasons that outweigh my POV, by all means feel free to do that. The changes are mechanical and IMHO don't need an apparmor-focused review (though if you'd prefer I go through the full patch, I can). -- Jamie Strandboge | http://www.canonical.com signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] virt-aa-helper: add rules for shmem devices
On Wed, 20 Nov 2019, Christian Ehrhardt wrote:
> On Tue, Nov 19, 2019 at 10:25 PM Jamie Strandboge wrote:
> > On Tue, 22 Oct 2019, Christian Ehrhardt wrote:
> > > +for (i = 0; i < ctl->def->nshmems; i++) {
> > > +if (ctl->def->shmems[i]) {
> > > +virDomainShmemDef *shmem = ctl->def->shmems[i];
> > > +/* server path can be on any type and overwrites defaults */
> > > +if (shmem->server.enabled &&
> > > +shmem->server.chr.data.nix.path) {
> > > +if (vah_add_file(,
> > > shmem->server.chr.data.nix.path,
> > > +"rw") != 0)
> > > +goto cleanup;
> >
> > I'll let others comment on the code changes, but this apparmor rule
> > looks ok.
> >
> > > +} else {
> >
> > That said, I wonder about the logic here since up above we have:
> >
> > if (shmem->server.enabled && shmem->server.chr.data.nix.path)
> >
> > but here we just have 'else'. What happens if server.enabled is false
> > and server.chr.data.nix.path is set or vice versa? Does this 'else'
> > clause correctly handle those scenarios?
>
> Yes if either of the above isn't fulfilled it will fall back to use
> the default paths.
> So a single else without other checks should be correct.
> The following switch then differs the default paths used base on the model.
Thanks! We agreed on irc a small code comment would clarify this. LGTM
provided you add a code comment similar to what we discussed on IRC.
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/4] apparmor: let AppArmorSetSecurityImageLabel append rules
On Wed, 20 Nov 2019, Christian Ehrhardt wrote: > On Wed, Nov 20, 2019 at 3:40 PM Christian Ehrhardt > wrote: > > > > On Tue, Nov 19, 2019 at 10:12 PM Jamie Strandboge > > wrote: > > > > > > On Wed, 16 Oct 2019, Christian Ehrhardt wrote: > > > > > > > There are currently broken use cases, e.g. snapshotting more than one > > > > disk at > > > > once like: > > > > $ virsh snapshot-create-as --domain eoan --disk-only --atomic > > > >--diskspec vda,snapshot=no --diskspec vdb,snapshot=no > > > >--diskspec vdc,file=/test/disk1.snapshot1.qcow,snapshot=external > > > >--diskspec vdd,file=/test/disk2.snapshot1.qcow,snapshot=external > > > > The command above will iterate from qemuDomainSnapshotCreateDiskActive > > > > and > > > > eventually add /test/disk1.snapshot1.qcow first (appears in the rules) > > > > to then later add /test/disk2.snapshot1.qcow and while doing so throwing > > > > away the former rule causing it to fail. > > > > > > > > All other calls to (re)load_profile already use append=true when adding > > > > rules append=false is only used when restoring rules [1]. > > > > > > > > Fix this by letting AppArmorSetSecurityImageLabel use append=true as > > > > well. > > > > > > > > Bugs: > > > > https://bugs.launchpad.net/libvirt/+bug/1845506 > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1746684 > > > > > > > > [1]: https://bugs.launchpad.net/libvirt/+bug/1845506/comments/13 > > > > > > > > Signed-off-by: Christian Ehrhardt > > > > --- > > > > src/security/security_apparmor.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/src/security/security_apparmor.c > > > > b/src/security/security_apparmor.c > > > > index 320d69e52a..4dd7ba20b4 100644 > > > > --- a/src/security/security_apparmor.c > > > > +++ b/src/security/security_apparmor.c > > > > @@ -812,7 +812,7 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr > > > > mgr, > > > > return -1; > > > > } > > > > > > > > -return reload_profile(mgr, def, src->path, false); > > > > +return reload_profile(mgr, def, src->path, true); > > > > > > src/security/security_manager.c shows that > > > virSecurityManagerSetImageLabel() is called to label 'a storage image > > > with the configured security label'. Based on your report, it seems that > > > in addition to the underlying disk (vda in this case), it is also called > > > for each additional disk (ie, vdc and vdd). > > > > Yes in the "transaction" for e.g. a snapshot on multiple disks there will be > > calls for both disks before any of them becomes part of the VM-definition. > > That is where the second call "removes" the first rule and then things fail. > > > > To be clear, I didn't add any "disk" in the broken use-case it adds > > further backing image chain elements to existing disks as part of the > > snapshot action. > > So if I snapshot in one shot vdb and vdc it is called for each of > > those adding the new paths for the respective backing chain that > > grows. > > > > If you ever power cycle that it will be part of the definition and > > virt-aa-helper resolves backing chains as needed. > > > > > While your patch to append > > > makes sense for this scenario, what happens if you update the vm > > > definition to use 'vde' instead of 'vda', is the vda access removed and > > > vde added with vdc and vdd remaining? > > > > Hmm - not sure what exactly you mean, but lets try to break it ... > > > > This splits into three major paths to consider: > > > > a) adding/removing disks while the guest is off. > > This isn't interesting as that way it will be part (or not) of the > > definition when it starts. > > The guest will get initial rules based on that definition on > > start, nothing special. > > > > b) adding/removing disks at runtime of a guest > >b1) you can edit a live definition in XML, but that will only > > add/modify the disk on next start > > Even if you now add another disk via proper hot-add later the > > first will not be added > > as it isn't really part of the active definition yet. > >b2) you can ho
Re: [libvirt] [patch 1/1] virt-aa-helper: Add support for smartcard host-certificates
On Thu, 14 Nov 2019, Cole Robinson wrote:
> On 10/24/19 4:57 AM, Arnaud Patard wrote:
> > When emulating smartcard with host certificates, qemu needs to
> > be able to read the certificates files, which is denied by apparmor.
> > Add necessary code to add the smartcard certificates related directory
> > to the apparmor profile.
> >
> > This code supports only this case smartcard 'host' and 'passthrough'
> > settings are not supported, as I can't test them.
> >
> > Signed-off-by: Arnaud Patard
> > Index: libvirt-5.0.0/src/security/virt-aa-helper.c
> > ===
> > --- libvirt-5.0.0.orig/src/security/virt-aa-helper.c
> > +++ libvirt-5.0.0/src/security/virt-aa-helper.c
> > @@ -1251,6 +1251,26 @@ get_files(vahControl * ctl)
> > }
> > }
> >
> > +for (i = 0; i < ctl->def->nsmartcards; i++) {
> > +virDomainSmartcardDefPtr sc = ctl->def->smartcards[i];
> > +virDomainSmartcardType sc_type = sc->type;
> > +char *sc_db = (char *)VIR_DOMAIN_SMARTCARD_DEFAULT_DATABASE;
> > +if (sc->data.cert.database)
> > +sc_db = sc->data.cert.database;
> > +switch(sc_type) {
>
> Add a space after 'switch'. 'make syntax-check' will catch this. libvirt
> style is typically to not indent the 'case' keyword either, but this
> file is inconsistent on that front. With those fixed:
>
> Reviewed-by: Cole Robinson
>
> This matches what is done for the selinux driver AFAICT
>
> CCing apparmor maintainers, I'll defer to them to commit
>
> - Cole
>
> > +case VIR_DOMAIN_SMARTCARD_TYPE_HOST:
> > +break;
> > +case VIR_DOMAIN_SMARTCARD_TYPE_HOST_CERTIFICATES:
> > +virBufferAsprintf(, " \"%s/\" rk,\n", sc_db);
> > +virBufferAsprintf(, " \"%s/*\" rk,\n", sc_db);
> > +break;
I'll let other comment on the code changes, but these rules look ok. I
might suggest using this one line instead:
virBufferAsprintf(, " \"%s/{,*}" rk,\n", sc_db);
Is it possible that the certificates might be in a lower directory? Ie,
is '**' warranted?
> > +case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
> > + break;
> > +case VIR_DOMAIN_SMARTCARD_TYPE_LAST:
> > +break;
> > +}
> > +}
It probably makes sense to add a code comment on why
VIR_DOMAIN_SMARTCARD_TYPE_HOST, VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH
and VIR_DOMAIN_SMARTCARD_TYPE_LAST aren't supported and ignored.
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/3] virt-aa-helper: drop pointer checks in get_files
def->fss[i]->type == VIR_DOMAIN_FS_TYPE_MOUNT &&
> (ctl->def->fss[i]->fsdriver ==
> VIR_DOMAIN_FS_DRIVER_TYPE_PATH ||
> ctl->def->fss[i]->fsdriver ==
> VIR_DOMAIN_FS_DRIVER_TYPE_DEFAULT) &&
> ctl->def->fss[i]->src) {
> @@ -1166,16 +1160,14 @@ get_files(vahControl * ctl)
> }
>
> for (i = 0; i < ctl->def->ninputs; i++) {
> -if (ctl->def->inputs[i] &&
> -ctl->def->inputs[i]->type ==
> VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH) {
> +if (ctl->def->inputs[i]->type == VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH) {
> if (vah_add_file(, ctl->def->inputs[i]->source.evdev, "rw")
> != 0)
> goto cleanup;
> }
> }
>
> for (i = 0; i < ctl->def->nnets; i++) {
> -if (ctl->def->nets[i] &&
> -ctl->def->nets[i]->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER &&
> +if (ctl->def->nets[i]->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER &&
> ctl->def->nets[i]->data.vhostuser) {
> virDomainChrSourceDefPtr vhu = ctl->def->nets[i]->data.vhostuser;
>
> @@ -1186,8 +1178,7 @@ get_files(vahControl * ctl)
> }
>
> for (i = 0; i < ctl->def->nmems; i++) {
> -if (ctl->def->mems[i] &&
> -ctl->def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
> +if (ctl->def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
> if (vah_add_file(, ctl->def->mems[i]->nvdimmPath, "rw") != 0)
> goto cleanup;
> }
> --
> 2.24.0
>
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] virt-aa-helper: add rules for shmem devices
On Tue, 22 Oct 2019, Christian Ehrhardt wrote:
> Shared memory devices need qemu to be able to access certain paths
> either for the shared memory directly (mostly ivshmem-plain) or for a
> socket (mostly ivshmem-doorbell).
>
> Add logic to virt-aa-helper to render those apparmor rules based
> on the domain configuration.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1761645
>
> Signed-off-by: Christian Ehrhardt
> ---
> src/security/virt-aa-helper.c | 35 +++
> 1 file changed, 35 insertions(+)
>
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index 7d7262ca39..8c261f0010 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -958,6 +958,7 @@ get_files(vahControl * ctl)
> int rc = -1;
> size_t i;
> char *uuid;
> +char *mem_path = NULL;
> char uuidstr[VIR_UUID_STRING_BUFLEN];
> bool needsVfio = false, needsvhost = false, needsgl = false;
>
> @@ -1224,6 +1225,39 @@ get_files(vahControl * ctl)
> }
> }
>
> +for (i = 0; i < ctl->def->nshmems; i++) {
> +if (ctl->def->shmems[i]) {
> +virDomainShmemDef *shmem = ctl->def->shmems[i];
> +/* server path can be on any type and overwrites defaults */
> +if (shmem->server.enabled &&
> +shmem->server.chr.data.nix.path) {
> +if (vah_add_file(, shmem->server.chr.data.nix.path,
> +"rw") != 0)
> +goto cleanup;
I'll let others comment on the code changes, but this apparmor rule
looks ok.
> +} else {
That said, I wonder about the logic here since up above we have:
if (shmem->server.enabled && shmem->server.chr.data.nix.path)
but here we just have 'else'. What happens if server.enabled is false
and server.chr.data.nix.path is set or vice versa? Does this 'else'
clause correctly handle those scenarios?
> +switch (shmem->model) {
> +case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN:
> +/* until exposed, recreate qemuBuildShmemBackendMemProps
> */
> +if (virAsprintf(_path, "/dev/shm/%s", shmem->name) <
> 0)
> +goto cleanup;
> +break;
> +case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL:
> +case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM:
> + /* until exposed, recreate
> qemuDomainPrepareShmemChardev */
> +if (virAsprintf(_path,
> "/var/lib/libvirt/shmem-%s-sock",
> +shmem->name) < 0)
> +goto cleanup;
> + break;
> +}
> +if (mem_path != NULL) {
> +if (vah_add_file(, mem_path, "rw") != 0)
> +goto cleanup;
> +}
> +}
> +}
> +}
> +
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/4] apparmor: let AppArmorSetSecurityImageLabel append rules
On Wed, 16 Oct 2019, Christian Ehrhardt wrote: > There are currently broken use cases, e.g. snapshotting more than one disk at > once like: > $ virsh snapshot-create-as --domain eoan --disk-only --atomic >--diskspec vda,snapshot=no --diskspec vdb,snapshot=no >--diskspec vdc,file=/test/disk1.snapshot1.qcow,snapshot=external >--diskspec vdd,file=/test/disk2.snapshot1.qcow,snapshot=external > The command above will iterate from qemuDomainSnapshotCreateDiskActive and > eventually add /test/disk1.snapshot1.qcow first (appears in the rules) > to then later add /test/disk2.snapshot1.qcow and while doing so throwing > away the former rule causing it to fail. > > All other calls to (re)load_profile already use append=true when adding > rules append=false is only used when restoring rules [1]. > > Fix this by letting AppArmorSetSecurityImageLabel use append=true as well. > > Bugs: > https://bugs.launchpad.net/libvirt/+bug/1845506 > https://bugzilla.redhat.com/show_bug.cgi?id=1746684 > > [1]: https://bugs.launchpad.net/libvirt/+bug/1845506/comments/13 > > Signed-off-by: Christian Ehrhardt > --- > src/security/security_apparmor.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/security/security_apparmor.c > b/src/security/security_apparmor.c > index 320d69e52a..4dd7ba20b4 100644 > --- a/src/security/security_apparmor.c > +++ b/src/security/security_apparmor.c > @@ -812,7 +812,7 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr, > return -1; > } > > -return reload_profile(mgr, def, src->path, false); > +return reload_profile(mgr, def, src->path, true); src/security/security_manager.c shows that virSecurityManagerSetImageLabel() is called to label 'a storage image with the configured security label'. Based on your report, it seems that in addition to the underlying disk (vda in this case), it is also called for each additional disk (ie, vdc and vdd). While your patch to append makes sense for this scenario, what happens if you update the vm definition to use 'vde' instead of 'vda', is the vda access removed and vde added with vdc and vdd remaining? What if we remove vda and replace it with only vde, are vda, vdc and vdd removed? I fear that making this change will result in scenarios where the rule is (correctly) added, but previous rules are not removed. Can you comment on if this is working correctly? Is it possible to have tests that demonstrate everything is working as intended? -- Jamie Strandboge | http://www.canonical.com signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/4] apparmor: refactor AppArmorSetSecurityImageLabel
On Wed, 16 Oct 2019, Christian Ehrhardt wrote:
> A lot of the code in AppArmorSetSecurityImageLabel is a duplicate of
> what is in reload_profile, this refactors AppArmorSetSecurityImageLabel
> to use reload_profile instead.
>
> Signed-off-by: Christian Ehrhardt
> ---
> src/security/security_apparmor.c | 38
> 1 file changed, 9 insertions(+), 29 deletions(-)
>
> diff --git a/src/security/security_apparmor.c
> b/src/security/security_apparmor.c
> index 691833eb4b..320d69e52a 100644
> --- a/src/security/security_apparmor.c
> +++ b/src/security/security_apparmor.c
> @@ -792,8 +792,6 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
>virStorageSourcePtr src,
>virSecurityDomainImageLabelFlags flags
> G_GNUC_UNUSED)
> {
> -int rc = -1;
> -char *profile_name = NULL;
> virSecurityLabelDefPtr secdef;
>
> if (!src->path || !virStorageSourceIsLocalStorage(src))
> @@ -803,36 +801,18 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
> if (!secdef || !secdef->relabel)
> return 0;
>
> -if (secdef->imagelabel) {
> -/* if the device doesn't exist, error out */
> -if (!virFileExists(src->path)) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("\'%s\' does not exist"),
> - src->path);
> -return -1;
> -}
> -
> -if ((profile_name = get_profile_name(def)) == NULL)
> -return -1;
> +if (!secdef->imagelabel)
> +return 0;
>
> -/* update the profile only if it is loaded */
> -if (profile_loaded(secdef->imagelabel) >= 0) {
> -if (load_profile(mgr, secdef->imagelabel, def,
> - src->path, false) < 0) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("cannot update AppArmor profile "
> - "\'%s\'"),
> - secdef->imagelabel);
> -goto cleanup;
> -}
> -}
> +/* if the device doesn't exist, error out */
> +if (!virFileExists(src->path)) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("\'%s\' does not exist"),
> + src->path);
> +return -1;
> }
> -rc = 0;
>
> - cleanup:
> -VIR_FREE(profile_name);
> -
> -return rc;
> +return reload_profile(mgr, def, src->path, false);
The logic of the refactor looks fine, but note by calling
reload_profile() here, it will call virDomainDefGetSecurityLabelDef()
which we've already done in this function, so we are adding a needless
extra call. This doesn't seem to be a particularly expensive call (see
src/conf/domain_conf.c), so +1 as is, though you may want to consider
trying to get rid of it.
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/4] apparmor: drop useless call to get_profile_name
On Wed, 16 Oct 2019, Christian Ehrhardt wrote:
> reload_profile calls get_profile_name for no particular gain, lets
> remove that call. The string isn't used in that function later on
> and not registered/passed anywhere.
>
> It can only fail if it either can't allocate or if the
> virDomainDefPtr would have no uuid set (which isn't allowed).
>
> Thereby the only "check" it really provides is if it can allocate the
> string to then free it again.
>
> This was initially added in [1] when the code was still in
> AppArmorRestoreSecurityImageLabel (later moved) and even back then had
> no further effect than described above.
>
> [1]:
> https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/security/security_apparmor.c;h=16de0f26f41689e0c50481120d9f8a59ba1f4073;hb=bbaecd6a8f15345bc822ab4b79eb0955986bb2fd#l487
>
> Signed-off-by: Christian Ehrhardt
> ---
> src/security/security_apparmor.c | 14 ++
> 1 file changed, 2 insertions(+), 12 deletions(-)
>
> diff --git a/src/security/security_apparmor.c
> b/src/security/security_apparmor.c
> index 75203cc43a..691833eb4b 100644
> --- a/src/security/security_apparmor.c
> +++ b/src/security/security_apparmor.c
> @@ -282,17 +282,12 @@ reload_profile(virSecurityManagerPtr mgr,
> const char *fn,
> bool append)
> {
> -int rc = -1;
> -char *profile_name = NULL;
> virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef(
> def, SECURITY_APPARMOR_NAME);
>
> if (!secdef || !secdef->relabel)
> return 0;
>
> -if ((profile_name = get_profile_name(def)) == NULL)
> -return rc;
> -
> /* Update the profile only if it is loaded */
> if (profile_loaded(secdef->imagelabel) >= 0) {
> if (load_profile(mgr, secdef->imagelabel, def, fn, append) < 0) {
> @@ -300,15 +295,10 @@ reload_profile(virSecurityManagerPtr mgr,
> _("cannot update AppArmor profile "
> "\'%s\'"),
> secdef->imagelabel);
> -goto cleanup;
> +return -1;
> }
> }
> -
> -rc = 0;
> - cleanup:
> -VIR_FREE(profile_name);
> -
> -return rc;
> +return 0;
> }
>
> static int
LGTM. I don't recall why this was there initially but guessing it was
obviated by a refactor at some point (perhaps before I initially
submitted).
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] virt-aa-helper: clarify command line options
On Wed, 16 Oct 2019, Christian Ehrhardt wrote:
> While only used internally from libvirt the options still are misleading
> enough to cause issues every now and then.
> Group modes, options and an adding extra file and extend the wording of
> the latter which had the biggest lack of clarity.
> Both add a file to the end of the rules, but one re-generates the
> rules from XML and the other keeps the existing rules as-is not
> considering the XML content.
>
> Signed-off-by: Christian Ehrhardt
> ---
> src/security/virt-aa-helper.c | 14 --
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index 9157411133..e0c72e1b9c 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -95,18 +95,20 @@ vahDeinit(vahControl * ctl)
> static void
> vah_usage(void)
> {
> -printf(_("\n%s [options] [< def.xml]\n\n"
> -" Options:\n"
> +printf(_("\n%s mode [options] [extra file] [< def.xml]\n\n"
> +" Modes:\n"
> "-a | --add load profile\n"
> "-c | --create create profile from
> template\n"
> -"-d | --dryrun dry run\n"
> "-D | --delete unload and delete profile\n"
> -"-f | --add-file add file to profile\n"
> -"-F | --append-file append file to profile\n"
> "-r | --replace reload profile\n"
> "-R | --remove unload profile\n"
> -"-h | --helpthis help\n"
> +" Options:\n"
> +"-d | --dryrun dry run\n"
> "-u | --uuid uuid (profile name)\n"
> +"-h | --helpthis help\n"
> +" Extra File:\n"
> +"-f | --add-file add file to a profile
> generated from XML\n"
> +"-F | --append-file append file to an existing
> profile\n"
> "\n"), progname);
>
> puts(_("This command is intended to be used by libvirtd "
This LGTM
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] security: AppArmor profile fixes for swtpm
On Mon, 16 Sep 2019, Chris Coulson wrote: > The AppArmor profile generated by virt-aa-helper is too strict for swtpm. > This change contains 2 small fixes: > - Relax append access to swtpm's log file to permit write access instead. > Append access is insufficient because the log is opened with O_CREAT. > - Permit swtpm to acquire a lock on its lock file. > --- > src/security/virt-aa-helper.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c > index 326cfaf52a..3d7cc32459 100644 > --- a/src/security/virt-aa-helper.c > +++ b/src/security/virt-aa-helper.c > @@ -1238,10 +1238,10 @@ get_files(vahControl * ctl) > * directory, log, and PID files. > */ > virBufferAsprintf(, > -" \"%s/lib/libvirt/swtpm/%s/%s/**\" rw,\n", > +" \"%s/lib/libvirt/swtpm/%s/%s/**\" rwk,\n", > LOCALSTATEDIR, uuidstr, tpmpath); > virBufferAsprintf(, > -" \"%s/log/swtpm/libvirt/qemu/%s-swtpm.log\" a,\n", > +" \"%s/log/swtpm/libvirt/qemu/%s-swtpm.log\" w,\n", > LOCALSTATEDIR, ctl->def->name); > virBufferAsprintf(, > " \"%s/libvirt/qemu/swtpm/%s-swtpm.pid\" rw,\n", LGTM. +1 to apply -- Jamie Strandboge | http://www.canonical.com signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virt-aa-helper: Actually fix AppArmor profile
On Tue, 20 Aug 2019, Andrea Bolognani wrote:
> --- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper
> +++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper
> @@ -18,8 +18,8 @@ profile virt-aa-helper
> /usr/{lib,lib64}/libvirt/virt-aa-helper {
>@{PROC}/filesystems r,
>
># Used when internally running another command (namely apparmor_parser)
> - @{PROC}/self/fd r,
> - @{PROC}/@{pid}/fd r,
> + @{PROC}/self/fd/ r,
/proc/self is a 'magic symlink' and apparmor will resolve symlinks
before performing checks. As such, @{PROC}/self/fd/ is redundant with
the next rule.
> + @{PROC}/@{pid}/fd/ r,
This access LGTM. +1 to apply.
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] security: aa-helper: allow virt-aa-helper to read .vhd images
On Wed, 03 Jul 2019, Christian Ehrhardt wrote:
> VHD images can be used as any other, so we should add them to the list
> of types that virt-aa-helper can read when creating the per-guest rules
> for backing files.
>
> Signed-off-by: Christian Ehrhardt
> ---
> src/security/apparmor/usr.lib.libvirt.virt-aa-helper | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper
> b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper
> index 78994bcda6..bf6bd297d1 100644
> --- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper
> +++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper
> @@ -63,6 +63,7 @@ profile virt-aa-helper
> /usr/{lib,lib64}/libvirt/virt-aa-helper {
>/**.qcow{,2} r,
>/**.qed r,
>/**.vmdk r,
> + /**.vhd r,
This looks fine. +1 to apply.
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] apparmor: Add openGraphicsFD rule for named profile
On Wed, 19 Jun 2019, Christian Ehrhardt wrote: > Commit a3ab6d42 changed the libvirtd profile to a named profile > but neglected to accommodate the change in the qemu profile > ptrace and signal rules. > Later on 4ec3cf9a fixed that for ptrace and signal but openGraphicsFD > is still missing. > > As a result, libvirtd is unable to open UI on libvirt >=5.1 e.g. with > virt-manager. > > Add openGraphicsFD rule that references the libvirtd profile > by name in addition to full binary path. > > Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1833040 > > Signed-off-by: Christian Ehrhardt > --- > 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 165558fe83..d33348aa05 100644 > --- a/src/security/apparmor/libvirt-qemu > +++ b/src/security/apparmor/libvirt-qemu > @@ -208,6 +208,7 @@ >/sys/firmware/devicetree/** r, > ># allow connect with openGraphicsFD to work > + unix (send, receive) type=stream addr=none peer=(label=libvirtd), >unix (send, receive) type=stream addr=none peer=(label=/usr/sbin/libvirtd), > ># for gathering information about available host resources +1 to apply. Thanks for chasing this down. -- Jamie Strandboge | http://www.canonical.com signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] security: apparmor: make vhost-net access a static rule
On Wed, 20 Mar 2019, Christian Ehrhardt wrote:
> On Wed, Mar 20, 2019 at 8:45 AM Christian Ehrhardt
> wrote:
> >
> > On Mon, Mar 4, 2019 at 11:42 AM Christian Ehrhardt
> > wrote:
> > >
> > > On Fri, Mar 1, 2019 at 5:56 PM Jamie Strandboge
> > > wrote:
> > > >
> > > > On Mon, 18 Feb 2019, Christian Ehrhardt wrote:
> > > >
> > > > > So far we were detecting at guest start if any devices needed vhost
> > > > > net
> > > > > and only if that was true added a rule for /dev/vhost-net.
> > > > >
> > > > > It turns out that it is an absolutely valid case to start a guest
> > > > > without any vhost-net networking but later on wanting to hotplug such
> > > > > a
> > > > > device which then would be denied by apparmor.
> > > > >
> > > > > Unfortunately there also is no security labeling callback involved
> > > > > other
> > > > > than the one to /dev/net/tun. But on the other hand vhost-net is no
> > > > > more
> > > > > new and considered rather safe. Therefore drop the old detection and
> > > > > just add it as a static rule.
> > > > >
> > > > > Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1815910
> > > > >
> > > > > Signed-off-by: Christian Ehrhardt
> > > > > ---
> > > > > src/security/apparmor/libvirt-qemu | 1 +
> > > > > src/security/virt-aa-helper.c | 17 +
> > > > > 2 files changed, 2 insertions(+), 16 deletions(-)
> > > > >
> > > > > diff --git a/src/security/apparmor/libvirt-qemu
> > > > > b/src/security/apparmor/libvirt-qemu
> > > > > index eaa5167525..a71f34c175 100644
> > > > > --- a/src/security/apparmor/libvirt-qemu
> > > > > +++ b/src/security/apparmor/libvirt-qemu
> > > > > @@ -21,6 +21,7 @@
> > > > >signal (receive) peer=/usr/sbin/libvirtd,
> > > > >
> > > > >/dev/net/tun rw,
> > > > > + /dev/vhost-net rw,
> > > > >/dev/kvm rw,
> > > > >/dev/ptmx rw,
> > > > >/dev/kqemu rw,
> > > > > diff --git a/src/security/virt-aa-helper.c
> > > > > b/src/security/virt-aa-helper.c
> > > > > index 8e22e9978a..ebc4feac77 100644
> > > > > --- a/src/security/virt-aa-helper.c
> > > > > +++ b/src/security/virt-aa-helper.c
> > > > > @@ -937,7 +937,7 @@ get_files(vahControl * ctl)
> > > > > size_t i;
> > > > > char *uuid;
> > > > > char uuidstr[VIR_UUID_STRING_BUFLEN];
> > > > > -bool needsVfio = false, needsvhost = false;
> > > > > +bool needsVfio = false;
> > > > >
> > > > > /* verify uuid is same as what we were given on the command line
> > > > > */
> > > > > virUUIDFormat(ctl->def->uuid, uuidstr);
> > > > > @@ -1248,21 +1248,6 @@ get_files(vahControl * ctl)
> > > > > }
> > > > > }
> > > > >
> > > > > -if (ctl->def->virtType == VIR_DOMAIN_VIRT_KVM) {
> > > > > -for (i = 0; i < ctl->def->nnets; i++) {
> > > > > -virDomainNetDefPtr net = ctl->def->nets[i];
> > > > > -if (net && net->model) {
> > > > > -if (net->driver.virtio.name ==
> > > > > VIR_DOMAIN_NET_BACKEND_TYPE_QEMU)
> > > > > -continue;
> > > > > -if (!virDomainNetIsVirtioModel(net))
> > > > > -continue;
> > > > > -}
> > > > > -needsvhost = true;
> > > > > -}
> > > > > -}
> > > > > -if (needsvhost)
> > > > > -virBufferAddLit(, " \"/dev/vhost-net\" rw,\n");
> > > > > -
> > > > > if (needsVfio) {
> > > > > virBufferAddLit(, " \"/dev/vfio/vfio\" rw,\n");
> > > > > virBufferAddLit(, " \"/dev/vfio/[0-9]*\" rw,\n");
> > > >
> > > > A few things I noticed:
> > > >
> > > > - if I launch a vm with a virtio net device, th
Re: [libvirt] [PATCH 1/2] apparmor: Check libvirtd profile status by name
On Fri, 01 Mar 2019, Jim Fehlig wrote:
> Commit a3ab6d42 changed the libvirtd profile to a named profile,
> breaking the apparmor driver's ability to detect if the profile is
> active. When the apparmor driver loads it checks the status of the
> libvirtd profile using the full binary path, which fails since the
> profile is now referenced by name. If the apparmor driver is
> explicitly requested in /etc/libvirt/qemu.conf, then libvirtd fails
> to load too.
>
> Instead of only checking the profile status by full binary path,
> also check by profile name. The full path check is retained in case
> users have a customized libvirtd profile with full path.
>
> Signed-off-by: Jim Fehlig
> ---
> src/security/security_apparmor.c | 12 +---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/src/security/security_apparmor.c
> b/src/security/security_apparmor.c
> index 4afdef065a..6d16b15c65 100644
> --- a/src/security/security_apparmor.c
> +++ b/src/security/security_apparmor.c
> @@ -257,10 +257,16 @@ use_apparmor(void)
> if (access(APPARMOR_PROFILES_PATH, R_OK) != 0)
> goto cleanup;
>
> +/* First check profile status using full binary path. If that fails
> + * check using profile name.
> + */
> rc = profile_status(libvirt_daemon, 1);
> -/* Error or unconfined should all result in -1*/
> -if (rc < 0)
> -rc = -1;
> +if (rc < 0) {
> +rc = profile_status("libvirtd", 1);
> +/* Error or unconfined should all result in -1*/
> +if (rc < 0)
> +rc = -1;
> +}
LGTM. +1 to apply. Thanks!
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] security: aa-helper: gl devices in sysfs at arbitrary depth
On Tue, 05 Mar 2019, Christian Ehrhardt wrote:
> Further testing with more devices showed that we sometimes have a
> different depth of pci device paths when accessing sysfs for device
> attributes.
>
> But since the access is limited to a set of filenames and read only it
> is safe to use a wildcard for that.
>
> Related apparmor denies - while we formerly had only considered:
> apparmor="DENIED" operation="open"
> name="/sys/devices/pci:00/:00:02.1/uevent"
> requested_mask="r"
>
> We now also know of cases like:
> apparmor="DENIED" operation="open"
> name="/sys/devices/pci:00/:00:03.1/:1c:00.0/uevent"
> requested_mask="r"
>
> Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1817943
>
> Signed-off-by: Christian Ehrhardt
> ---
> src/security/virt-aa-helper.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index 13b507ff69..989dcf1784 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -1286,8 +1286,7 @@ get_files(vahControl * ctl)
> virBufferAddLit(, " \"/dev/nvidiactl\" rw,\n");
> virBufferAddLit(, " # Probe DRI device attributes\n");
> virBufferAddLit(, " \"/dev/dri/\" r,\n");
> -virBufferAddLit(, "
> \"/sys/devices/*/*/{uevent,vendor,device,subsystem_vendor,subsystem_device}\"
> r,\n");
> -virBufferAddLit(, "
> \"/sys/devices/*/*/drm/*/{uevent,vendor,device,subsystem_vendor,subsystem_device}\"
> r,\n");
> +virBufferAddLit(, "
> \"/sys/devices/**/{uevent,vendor,device,subsystem_vendor,subsystem_device}\"
> r,\n");
I'm curious about the new denials, but the reads for these files should be
fine.
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] security: aa-helper: nvidia rules for gl devices
On Tue, 05 Mar 2019, Christian Ehrhardt wrote:
> Further testing with different devices showed that we need more rules
> to drive gl backends with nvidia cards. Related denies look like:
>
> apparmor="DENIED" operation="open"
> name="/usr/share/egl/egl_external_platform.d/"
> requested_mask="r"
> apparmor="DENIED" operation="open"
> name="/proc/modules"
> requested_mask="r"
> apparmor="DENIED" operation="open"
> name="/proc/driver/nvidia/params"
> requested_mask="r"
> apparmor="DENIED" operation="mknod"
> name="/dev/nvidiactl"
> requested_mask="c"
>
> Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1817943
>
> Signed-off-by: Christian Ehrhardt
> ---
> src/security/virt-aa-helper.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index e9120213ff..13b507ff69 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -1279,6 +1279,11 @@ get_files(vahControl * ctl)
> virBufferAddLit(, " \"/usr/share/drirc.d/{,*.conf}\" r,\n");
> virBufferAddLit(, " \"/etc/glvnd/egl_vendor.d/{,*}\" r,\n");
> virBufferAddLit(, " \"/usr/share/glvnd/egl_vendor.d/{,*}\"
> r,\n");
> +virBufferAddLit(, " \"/usr/share/egl/egl_external_platform.d/\"
> r,\n");
> +virBufferAddLit(, "
> \"/usr/share/egl/egl_external_platform.d/*\" r,\n");
> +virBufferAddLit(, " \"/proc/modules\" r,\n");
> +virBufferAddLit(, " \"/proc/driver/nvidia/params\" r,\n");
> +virBufferAddLit(, " \"/dev/nvidiactl\" rw,\n");
All the reads are fine. The 'rw' for nvidiactl is unfortunate but there isn't
anything we can do about the need for it. At least the policy doesn't have
'capability mknod' and DAC will protect against creating/removing the device
where the VMs run as non-root.
+1 to apply
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] apparmor: Add ptrace and signal rules for named profile
On Fri, 01 Mar 2019, Jim Fehlig wrote: > Commit a3ab6d42 changed the libvirtd profile to a named profile > but neglected to accommodate the change in the qemu profile > ptrace and signal rules. As a result, libvirtd is unable to > signal confined qemu processes and hence unable to shutdown > or destroy VMs. > > Add ptrace and signal rules that reference the libvirtd profile > by name in addition to full binary path. > > Signed-off-by: Jim Fehlig > --- > src/security/apparmor/libvirt-qemu | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/src/security/apparmor/libvirt-qemu > b/src/security/apparmor/libvirt-qemu > index 7d28faa163..474aaefdf8 100644 > --- a/src/security/apparmor/libvirt-qemu > +++ b/src/security/apparmor/libvirt-qemu > @@ -16,8 +16,10 @@ >network inet stream, >network inet6 stream, > > + ptrace (readby, tracedby) peer=libvirtd, >ptrace (readby, tracedby) peer=/usr/sbin/libvirtd, > > + signal (receive) peer=libvirtd, >signal (receive) peer=/usr/sbin/libvirtd, > >/dev/net/tun rw, +1 to commit -- Jamie Strandboge | http://www.canonical.com signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] security: apparmor: make vhost-net access a static rule
On Mon, 18 Feb 2019, Christian Ehrhardt wrote:
> So far we were detecting at guest start if any devices needed vhost net
> and only if that was true added a rule for /dev/vhost-net.
>
> It turns out that it is an absolutely valid case to start a guest
> without any vhost-net networking but later on wanting to hotplug such a
> device which then would be denied by apparmor.
>
> Unfortunately there also is no security labeling callback involved other
> than the one to /dev/net/tun. But on the other hand vhost-net is no more
> new and considered rather safe. Therefore drop the old detection and
> just add it as a static rule.
>
> Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1815910
>
> Signed-off-by: Christian Ehrhardt
> ---
> src/security/apparmor/libvirt-qemu | 1 +
> src/security/virt-aa-helper.c | 17 +
> 2 files changed, 2 insertions(+), 16 deletions(-)
>
> diff --git a/src/security/apparmor/libvirt-qemu
> b/src/security/apparmor/libvirt-qemu
> index eaa5167525..a71f34c175 100644
> --- a/src/security/apparmor/libvirt-qemu
> +++ b/src/security/apparmor/libvirt-qemu
> @@ -21,6 +21,7 @@
>signal (receive) peer=/usr/sbin/libvirtd,
>
>/dev/net/tun rw,
> + /dev/vhost-net rw,
>/dev/kvm rw,
>/dev/ptmx rw,
>/dev/kqemu rw,
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index 8e22e9978a..ebc4feac77 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -937,7 +937,7 @@ get_files(vahControl * ctl)
> size_t i;
> char *uuid;
> char uuidstr[VIR_UUID_STRING_BUFLEN];
> -bool needsVfio = false, needsvhost = false;
> +bool needsVfio = false;
>
> /* verify uuid is same as what we were given on the command line */
> virUUIDFormat(ctl->def->uuid, uuidstr);
> @@ -1248,21 +1248,6 @@ get_files(vahControl * ctl)
> }
> }
>
> -if (ctl->def->virtType == VIR_DOMAIN_VIRT_KVM) {
> -for (i = 0; i < ctl->def->nnets; i++) {
> -virDomainNetDefPtr net = ctl->def->nets[i];
> -if (net && net->model) {
> -if (net->driver.virtio.name ==
> VIR_DOMAIN_NET_BACKEND_TYPE_QEMU)
> -continue;
> -if (!virDomainNetIsVirtioModel(net))
> -continue;
> -}
> -needsvhost = true;
> -}
> -}
> -if (needsvhost)
> -virBufferAddLit(, " \"/dev/vhost-net\" rw,\n");
> -
> if (needsVfio) {
> virBufferAddLit(, " \"/dev/vfio/vfio\" rw,\n");
> virBufferAddLit(, " \"/dev/vfio/[0-9]*\" rw,\n");
A few things I noticed:
- if I launch a vm with a virtio net device, then I can detach/attach as much
as I want
- if I launch a vm with one virtio net device, then detach it, /dev/vhost-net
is not removed from the profile.
- if launch a vm with one virtio net device, then I detach it and I shutdown
the vm with no attached network devices, on the next vm start, the device is
present and the profile still has the access. IIRC, this is by design
- if I remove the net device from the vm definition then libvirtd updates
profile to not allow the access. When I start the vm I cannot attach a virtio
net device because the access is denied (this bug)
- /dev/vhost-net is root:root 600 and vms typically run as non-root
(libvirt-qemu:kvm here) and therefore a qemu process would not be able to
open the device on its own. The only reason why qemu can is because libvirt
passes an fd to it, and it is when qemu writes to it that the access is
denied
To me, it seems the real bug is that hotplug attach/detach is not updating the
profile accordingly (attach adds it if it isn't there and detach removes it if
no more net devices are present). Ideally, this would be the fix for this bug
(we certainly support other hotplug/hotunplug events, like disks). Considering
that the device expands the attack surface to this part of the kernel and
because the root:root 600 permissions show that non-root is not meant to access
the device, this is most correct.
That said, libvirt itself is providing protection here because it is mediating
the access to /dev/vhost-net for the VMs since the VMs can't open the device
themselves and they must rely on libvirt to pass in the fd. Because of this,
adding the access to the profile is not providing additional protections beyond
DAC *for this common use case and configuration*. Conditionally adding the
access would provide benefit when 'user = "root"' is set in qemu.conf or the
device itself has different permissions that allow the access (eg, 660
root:kvm).
I maintain a prefe
Re: [libvirt] [PATCH v2 2/2] security: aa-helper: generate more rules for gl devices
On Mon, 18 Feb 2019, Christian Ehrhardt wrote:
> +virBufferAddLit(, " \"/usr/lib{,32,64}/dri/**.so\" mr,\n");
> +virBufferAddLit(, " \"/usr/lib/@{multiarch}/dri/**.so\" mr,\n");
> +virBufferAddLit(, " \"/usr/lib/fglrx/dri/**.so\" mr,\n");
I'm sorry I think I wasn't clear on how to add in the .so files. I suggest:
virBufferAddLit(, " \"/usr/lib{,32,64}/dri/*.so*\" mr,\n");
virBufferAddLit(, " \"/usr/lib/@{multiarch}/dri/*.so*\" mr,\n");
virBufferAddLit(, " \"/usr/lib/fglrx/dri/*.so*\" mr,\n");
This is slightly futureproofed with the trailing '*'. On my system, the '**'
wasn't needed, but if you observe systems where it is, feel free to keep it.
The other parts of this patch looked fine.
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/2] security: aa-helper: allow virt-aa-helper to read /dev/dri
On Mon, 18 Feb 2019, Christian Ehrhardt wrote:
>
> + # for gl enabled graphics
> + /dev/dri/{,*} r,
> +
+1 to include
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] security: aa-helper: generate more rules for gl devices
On Tue, 12 Feb 2019, Christian Ehrhardt wrote:
> Change fb01e1a44 "virt-aa-helper: generate rules for gl enabled
> graphics devices" implemented the detection for gl enabled
> devices in virt-aa-helper. But further testing showed
> that it will need much more access for the full gl stack
> to work.
>
> Upstream apparmor just recently split those things out and now
> has two related abstractions at
> https://gitlab.com/apparmor/apparmor/blob/master:
> - dri-common at /profiles/apparmor.d/abstractions/dri-common
> - mesa: at /profiles/apparmor.d/abstractions/mesa
>
> If would be great to just include that for the majority of
> rules, but they are not yet in any distribution so we need
> to add rules inspired by them based on the testing that we
> can do.
>
> Furthermore qemu with opengl will also probe the backing device
> of the rendernode for attributes which should be safe as
> read-only wildcard rules.
>
> Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1815452
>
> Signed-off-by: Christian Ehrhardt
> ---
> src/security/virt-aa-helper.c | 20 +++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index 8e22e9978a..46c1914f58 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -937,7 +937,7 @@ get_files(vahControl * ctl)
> size_t i;
> char *uuid;
> char uuidstr[VIR_UUID_STRING_BUFLEN];
> -bool needsVfio = false, needsvhost = false;
> +bool needsVfio = false, needsvhost = false, needsgl = false;
>
> /* verify uuid is same as what we were given on the command line */
> virUUIDFormat(ctl->def->uuid, uuidstr);
> @@ -1065,9 +1065,11 @@ get_files(vahControl * ctl)
>
> if (rendernode) {
> vah_add_file(, rendernode, "rw");
> +needsgl = true;
> } else {
> if (virDomainGraphicsNeedsAutoRenderNode(graphics)) {
> char *defaultRenderNode = virHostGetDRMRenderNode();
> +needsgl = true;
>
> if (defaultRenderNode) {
> vah_add_file(, defaultRenderNode, "rw");
> @@ -1267,6 +1269,22 @@ get_files(vahControl * ctl)
> virBufferAddLit(, " \"/dev/vfio/vfio\" rw,\n");
> virBufferAddLit(, " \"/dev/vfio/[0-9]*\" rw,\n");
> }
> +if (needsgl) {
> +/* if using gl all sorts of further dri related paths will be needed
> */
> +virBufferAddLit(, " # DRI/Mesa/(e)GL config and driver
> paths\n");
> +virBufferAddLit(, " \"/usr/lib{,32,64}/dri/**\" mr,\n");
> +virBufferAddLit(, " \"/usr/lib/@{multiarch}/dri/**\" mr,\n");
> +virBufferAddLit(, " \"/usr/lib/fglrx/dri/**\" mr,\n");
> +virBufferAddLit(, " \"/etc/drirc\" r,\n");
> +virBufferAddLit(, " \"/usr/share/drirc.d/{,*.conf}\" r,\n");
> +virBufferAddLit(, " \"/etc/glvnd/egl_vendor.d/{,*}\" r,\n");
> +virBufferAddLit(, " \"/usr/share/glvnd/egl_vendor.d/{,*}\"
> r,\n");
The above rules are fine. It would be nice to me slightly more specific in the
mmap rules to mmap only .so files, but not a blocker.
> +virBufferAddLit(, " owner \"/var/lib/libvirt/.cache/\" w,\n");
This doesn't seem to belong here and DAC is usually going to prevent it since
VMs run as non-root and /var/lib/libvirt is 755. Perhaps get rid of owner and
make this an explicit denial rule to silence the denial (with a code comment)?
> +virBufferAddLit(, " # Probe DRI device attributes\n");
> +virBufferAddLit(, " \"/dev/dri/\" r,\n");
> +virBufferAddLit(, "
> \"/sys/devices/*/*/{uevent,vendor,device,subsystem_vendor,subsystem_device}\"
> r,\n");
> +virBufferAddLit(, "
> \"/sys/devices/*/*/drm/*/{uevent,vendor,device,subsystem_vendor,subsystem_device}\"
> r,\n");
These are fine.
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] security: aa-helper: allow virt-aa-helper to read /dev/dri
On Tue, 12 Feb 2019, Christian Ehrhardt wrote:
> Change fb01e1a44 "virt-aa-helper: generate rules for gl enabled
> graphics devices" implemented the detection for gl enabled
> devices in virt-aa-helper. But it will in certain cases e.g. if
> no rendernode was explicitly specified need to read /dev/dri
> which it currently isn't allowed.
>
> Add a rule to the apparmor profile of virt-aa-helper itself to
> be able to do that.
>
> Signed-off-by: Christian Ehrhardt
> ---
> src/security/apparmor/usr.lib.libvirt.virt-aa-helper | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper
> b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper
> index de9436872c..78994bcda6 100644
> --- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper
> +++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper
> @@ -19,6 +19,9 @@ profile virt-aa-helper
> /usr/{lib,lib64}/libvirt/virt-aa-helper {
>
>/etc/libnl-3/classid r,
>
> + # for gl enabled graphics
> + /dev/dri/{,*} r,
> +
This looks fine.
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] virt-aa-helper: generate rules for gl enabled graphics devices
On Tue, 22 Jan 2019, Christian Ehrhardt wrote:
> This adds the virt-aa-helper support for gl enabled graphics devices to
> generate rules for the needed rendernode paths.
>
> Example in domain xml:
>
>
>
>
> results in:
> "/dev/dri/bar" rw,
>
> Special cases are:
> - multiple devices with rendernodes -> all are added
> - non explicit rendernodes -> follow recently added virHostGetDRMRenderNode
> - rendernode without opengl (in egl-headless for example) -> still add
> the node
>
> Updates in V2:
> - avoid leaking rendernode
> - logically group patch hunks
> - drop base /dev/dri access
> - simplify detection of the need for a default rendernode
> - add tests in tests/virt-aa-helper-test
> - rebased to recent master
>
> Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1757085
>
> Signed-off-by: Christian Ehrhardt
> ---
> src/security/virt-aa-helper.c | 12
> tests/virt-aa-helper-test | 6 ++
> 2 files changed, 18 insertions(+)
>
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index 64a425671d..a84dd5a8c9 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -1062,6 +1062,18 @@ get_files(vahControl * ctl)
> for (i = 0; i < ctl->def->ngraphics; i++) {
> virDomainGraphicsDefPtr graphics = ctl->def->graphics[i];
> size_t n;
> +const char *rendernode = virDomainGraphicsGetRenderNode(graphics);
> +
> +if (rendernode)
> +vah_add_file(, rendernode, "rw");
> +else
> +if (virDomainGraphicsNeedsAutoRenderNode(graphics)) {
> +char * defaultRenderNode = virHostGetDRMRenderNode();
> +if (defaultRenderNode) {
> +vah_add_file(, defaultRenderNode, "rw");
> +VIR_FREE(defaultRenderNode);
> +}
> +}
>
> for (n = 0; n < graphics->nListens; n++) {
> virDomainGraphicsListenDef listenObj = graphics->listens[n];
> diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test
> index fb400574dc..6e674bfe5c 100755
> --- a/tests/virt-aa-helper-test
> +++ b/tests/virt-aa-helper-test
> @@ -378,6 +378,12 @@ testme "0" "input dev passthrough" "-r -u $valid_uuid"
> "$test_xml" "$disk2.*rw,$
> sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e
> "s,524288,1048576,g" -e
> "s,, model='nvdimm'>$disk2 unit='KiB'>5242880,g"
> "$template_xml" > "$test_xml"
> testme "0" "nvdimm" "-r -u $valid_uuid" "$test_xml" "$disk2.*rw,$"
>
> +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e
> "s,, rendernode='/dev/dri/testegl1'/>,g" "$template_xml" >
> "$test_xml"
> +testme "0" "dri egl" "-r -u $valid_uuid" "$test_xml"
> "/dev/dri/testegl1.*rw,$"
> +
> +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e
> "s,, rendernode='/dev/dri/testegl2'/>,g" "$template_xml" >
> "$test_xml"
> +testme "0" "dri spice" "-r -u $valid_uuid" "$test_xml"
> "/dev/dri/testegl2.*rw,$"
> +
> testme "0" "help" "-h"
>
> echo "" >$output
> --
> 2.17.1
LGTM. Thanks for the v2 and the tests :)
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] apparmor: convert libvirtd profile to a named profile
On Mon, 14 Jan 2019, Jim Fehlig wrote:
> Signed-off-by: Jim Fehlig
> ---
>
> Optional patch that may need a bit of coorindation with upstream apparmor
> since the dnsmasq profile currently has 'peer=/usr/sbin/libvirtd'.
>
> src/security/apparmor/usr.sbin.libvirtd | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/src/security/apparmor/usr.sbin.libvirtd
> b/src/security/apparmor/usr.sbin.libvirtd
> index 0db52c524c..29f9936ad9 100644
> --- a/src/security/apparmor/usr.sbin.libvirtd
> +++ b/src/security/apparmor/usr.sbin.libvirtd
> @@ -2,7 +2,7 @@
> #include
> @{LIBVIRT}="libvirt"
>
> -/usr/sbin/libvirtd flags=(attach_disconnected) {
> +profile libvirtd /usr/sbin/libvirtd flags=(attach_disconnected) {
>#include
>#include
>
> @@ -51,7 +51,7 @@
>unix (send, receive) type=stream addr=none peer=(label=unconfined
> addr=none),
>
>ptrace (read,trace) peer=unconfined,
> - ptrace (read,trace) peer=/usr/sbin/libvirtd,
> + ptrace (read,trace) peer=@{profile_name},
>ptrace (read,trace) peer=dnsmasq,
>ptrace (read,trace) peer=/usr/sbin/dnsmasq,
>ptrace (read,trace) peer=libvirt-*,
> @@ -123,6 +123,7 @@
> # For communication/control from libvirtd
> unix (send, receive) type=stream addr=none
> peer=(label=/usr/sbin/libvirtd),
> signal (receive) set=("term") peer=/usr/sbin/libvirtd,
> + signal (receive) set=("term") peer=libvirtd,
>
> /dev/net/tun rw,
> /etc/qemu/** r,
This also LGTM. It'd be nice if there was a mechanism to specify the parent
profile like we can the current profile, but we can't now and this is fine.
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] apparmor: Add support for named profiles
On Mon, 14 Jan 2019, Jim Fehlig wrote:
> Upstream apparmor is switching to named profiles. In short,
>
> /usr/sbin/dnsmasq {
>
> becomes
>
> profile dnsmasq /usr/sbin/dnsmasq {
>
> Consequently, any profiles that reference profiles in a peer= condition
> need to be updated if the referenced profile switches to a named profile.
> Apparmor commit 9ab45d81 switched dnsmasq to a named profile. ATM it is
> the only named profile switch that has affected libvirt. Add rules to the
> libvirtd profile to reference dnsmasq in peer= conditions by profile name.
>
> Signed-off-by: Jim Fehlig
> ---
> src/security/apparmor/usr.sbin.libvirtd | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/src/security/apparmor/usr.sbin.libvirtd
> b/src/security/apparmor/usr.sbin.libvirtd
> index f0ffc53008..0db52c524c 100644
> --- a/src/security/apparmor/usr.sbin.libvirtd
> +++ b/src/security/apparmor/usr.sbin.libvirtd
> @@ -52,9 +52,11 @@
>
>ptrace (read,trace) peer=unconfined,
>ptrace (read,trace) peer=/usr/sbin/libvirtd,
> + ptrace (read,trace) peer=dnsmasq,
>ptrace (read,trace) peer=/usr/sbin/dnsmasq,
>ptrace (read,trace) peer=libvirt-*,
>
> + signal (send) peer=dnsmasq,
>signal (send) peer=/usr/sbin/dnsmasq,
>signal (read, send) peer=libvirt-*,
>signal (send) set=("kill", "term") peer=unconfined,
This LGTM.
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] apparmor: fix ptrace rules with kernel 4.18
On Fri, 2018-08-24 at 08:12 +0200, Christian Ehrhardt wrote:
> Due to kernel upstream change 338d0be4 ("apparmor: fix ptrace read
> check")
> libvirt now hits apparmor denies like:
> apparmor="DENIED" operation="ptrace" profile="/usr/sbin/libvirtd"
> pid=4409 comm="libvirtd" requested_mask="read" denied_mask="read"
> peer="libvirt-14e92a75-7668-4b97-8f92-322fc1b9c78a"
>
> Extend the ptrace rule to also allow 'ptrace (read)' for libvirtd to
> work
> with these newer kernels.
>
> Fixes: https://bugs.launchpad.net/bugs/1788603
>
> Reported-by: Thadeu Lima de Souza Cascardo .com>
> Signed-off-by: Christian Ehrhardt
> ---
> examples/apparmor/usr.sbin.libvirtd | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/examples/apparmor/usr.sbin.libvirtd
> b/examples/apparmor/usr.sbin.libvirtd
> index 80e348b7ee..f0ffc53008 100644
> --- a/examples/apparmor/usr.sbin.libvirtd
> +++ b/examples/apparmor/usr.sbin.libvirtd
> @@ -50,10 +50,10 @@
># for --p2p migrations
>unix (send, receive) type=stream addr=none peer=(label=unconfined
> addr=none),
>
> - ptrace (trace) peer=unconfined,
> - ptrace (trace) peer=/usr/sbin/libvirtd,
> - ptrace (trace) peer=/usr/sbin/dnsmasq,
> - ptrace (trace) peer=libvirt-*,
> + ptrace (read,trace) peer=unconfined,
> + ptrace (read,trace) peer=/usr/sbin/libvirtd,
> + ptrace (read,trace) peer=/usr/sbin/dnsmasq,
> + ptrace (read,trace) peer=libvirt-*,
LGTM. +1 to apply
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 5/5] apparmor: allow to preserve /dev mountpoints into qemu namespaces
On Tue, 2018-08-14 at 08:18 +0200, Christian Ehrhardt wrote:
> Libvirt now tries to preserve all mounts under /dev in qemu
> namespaces.
> The old rules only listed a set of known paths but those are no more
> enough.
>
> I found some due to containers like /dev/.lxc/* and such but also
> /dev/console
> and /dev/net/tun.
>
> Libvirt is correct to do so, but we can no more predict the names
> properly, so
> we modify the rule to allow a wildcard based pattern matching what
> libvirt does.
>
> Signed-off-by: Christian Ehrhardt
> ---
> examples/apparmor/usr.sbin.libvirtd | 16 +---
> 1 file changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/examples/apparmor/usr.sbin.libvirtd
> b/examples/apparmor/usr.sbin.libvirtd
> index 3ff43c32a2..8625862ced 100644
> --- a/examples/apparmor/usr.sbin.libvirtd
> +++ b/examples/apparmor/usr.sbin.libvirtd
> @@ -33,17 +33,11 @@
>mount options=(rw,rslave) -> /,
>mount options=(rw, nosuid) -> /{var/,}run/libvirt/qemu/*.dev/,
>
> - mount options=(rw, move) /dev/ ->
> /{var/,}run/libvirt/qemu/*.dev/,
> - mount options=(rw, move) /dev/hugepages/ ->
> /{var/,}run/libvirt/qemu/*.hugepages/,
> - mount options=(rw, move) /dev/mqueue/->
> /{var/,}run/libvirt/qemu/*.mqueue/,
> - mount options=(rw, move) /dev/pts/ ->
> /{var/,}run/libvirt/qemu/*.pts/,
> - mount options=(rw, move) /dev/shm/ ->
> /{var/,}run/libvirt/qemu/*.shm/,
> -
> - mount options=(rw, move) /{var/,}run/libvirt/qemu/*.dev/ ->
> /dev/,
> - mount options=(rw, move) /{var/,}run/libvirt/qemu/*.hugepages/ ->
> /dev/hugepages/,
> - mount options=(rw, move) /{var/,}run/libvirt/qemu/*.mqueue/->
> /dev/mqueue/,
> - mount options=(rw, move) /{var/,}run/libvirt/qemu/*.pts/ ->
> /dev/pts/,
> - mount options=(rw, move) /{var/,}run/libvirt/qemu/*.shm/ ->
> /dev/shm/,
> + # libvirt provides any mounts under /dev to qemu namespaces
> + mount options=(rw, move) /dev/ -> /{var/,}run/libvirt/qemu/*.dev/,
> + mount options=(rw, move) /dev/** ->
> /{var/,}run/libvirt/qemu/*{/,},
> + mount options=(rw, move) /{var/,}run/libvirt/qemu/*.dev/ -> /dev/,
> + mount options=(rw, move) /{var/,}run/libvirt/qemu/*{/,} ->
> /dev/**,
+1 for the rules. Please change '{var/,}' to '{,var/}' and '{/,}' to
'{,/}' since, while equivalent, the latter is a more widely use rule
style (I recognize that the previous rules used '{var/,}').
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 4/5] apparmor: allow qemu-smb access in /tmp
On Tue, 2018-08-14 at 08:18 +0200, Christian Ehrhardt wrote:
> The samba feature of qemu will place the samba config file in
> /tmp/qemu-smb..
>
> But at least it has a predictable path identifying qemu-smb feature
> itself by an infix in the path.
>
> This is a compromise of security and usability as the "owner"
> restriction
> will not protect guests among each other.
> Therefore the rule added makes the feature usable, but does not allow
> cross guest protection.
>
> Core issue is, that it is currently impossible to predict the PID
> which would
> follow "qemu-smb-", but long term, once the samba feature would be
> exposed in
> guest XML we'd prefer a virt-aa-helper based solution that can render
> the
> samba rule on demand and with a custom PID into the per guest
> profile.
>
> But the same is true for manual user overrides for this feature as
> well,
> they can neither predict the PID, nor have a local include per-guest.
> Thereby
> punting this to the user to add the rule later will not make it
> safer, but
> only less usable.
While the facts are true, there is something omitted and I come to a
different conclusion. It is true that the pid is unpredictable, but
with the 'owner /{,var/}tmp/**/ r,' rule, it is easy for a confined
process to find the directories. Also, the rule 'owner /tmp/qemu-
smb.*/{,**} rw,' (below) allows writing /tmp/qemu-smb.*/ so symlink
attacks are possible by a rogue VM against other VMs. Rogue VMs could
also use the directory in coordinated file sharing (but I mention this
only for completeness, not as an objection, since there are other rules
in the policy that 'overlap' and allow this sort of thing).
It is definitely a usability improvement to include the rule, but I
disagree that it isn't safer without it-- your addition applies to all
libvirt/apparmor users, many of whom will not use the smb feature that
isn't supported by the domain xml. Now, you could argue that users that
never use the smb feature aren't affected by the proposed global rule
(which is true), but omitting this patch, users can add the glob rule
to the per-VM site policy in /etc/apparmor.d/libvirt/libvirt- for
only the VMs they need it. This might be useful if they, for example,
have one trusted VM that uses the smb feature and other untrusted VMs
that do not. With the global rule, the untrusted VMs have access by
default.
These concerns are somewhat contrived and I'm ambivalent about the
addition, but it bothers me that we are adding a rule for a use case
that isn't directly supported by libvirt. +0 as is.
If you can demonstrate that qemu guards against symlink attacks on the
dir and is otherwise safe from attack (eg, open(..., O_CREAT|O_EXCL)
followed by unlink(...)), etc, etc then I could be persuaded to give a
+1.
> Signed-off-by: Christian Ehrhardt
> ---
> examples/apparmor/libvirt-qemu | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/examples/apparmor/libvirt-qemu
> b/examples/apparmor/libvirt-qemu
> index 6971d3db03..350b13b824 100644
> --- a/examples/apparmor/libvirt-qemu
> +++ b/examples/apparmor/libvirt-qemu
> @@ -191,6 +191,11 @@
># want more unique paths per rule.
>/{,var/}tmp/ r,
>owner /{,var/}tmp/**/ r,
> + # allow qemu smb feature specific path with write access
> + # TODO: This is a compromise between security and usability - once
> e.g. samba
> + # would be expressed in libvirt XML it should be added on demand
> via
> + # virt-aa-helper instead.
> + owner /tmp/qemu-smb.*/{,**} rw,
>
># for file-posix getting limits since 9103f1ce
>/sys/devices/**/block/*/queue/max_segments r,
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/5] apparmor: allow expected /tmp access patterns
On Tue, 2018-08-14 at 08:18 +0200, Christian Ehrhardt wrote:
> Several cases were found needing /tmp, for example ceph will try to
> list /tmp
> This is a compromise of security and usability:
> - we only allow generally enumerating the base dir
> - enumerating anything deeper in the dir is at least guarded by the
>"owner" restriction, but while that protects files of other
> services
>it won't protect qemu instances against each other as they usually
> run
>with the same user.
> - even with the owner restriction we only allow read for the
> wildcard
>path
>
> Signed-off-by: Christian Ehrhardt
> ---
> examples/apparmor/libvirt-qemu | 12
> 1 file changed, 12 insertions(+)
>
> diff --git a/examples/apparmor/libvirt-qemu
> b/examples/apparmor/libvirt-qemu
> index 5caf14e418..6971d3db03 100644
> --- a/examples/apparmor/libvirt-qemu
> +++ b/examples/apparmor/libvirt-qemu
> @@ -180,6 +180,18 @@
># for rbd
>/etc/ceph/ceph.conf r,
>
> + # Various functions will need to enumerate /tmp (e.g. ceph), allow
> the base
> + # dir and a few known functions like samba support.
> + # We want to avoid to give blanket rw permission to everything
> under /tmp,
> + # users are expected to add site specific addons for more uncommon
> cases.
> + # Qemu processes usually all run as the same users, so the "owner"
> restriction
> + # prevents access to other services files, but not across
> different instances.
> + # This is a tradeoff between usability and security - if paths
> would be more
> + # predictable that would be preferred - at least for write rules
> we would
> + # want more unique paths per rule.
> + /{,var/}tmp/ r,
> + owner /{,var/}tmp/**/ r,
> +
># for file-posix getting limits since 9103f1ce
>/sys/devices/**/block/*/queue/max_segments r,
Thanks for the changes! The comments seem longer than 80 characters,
but +1 to commit as is.
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/4] apparmor: allow to preserve /dev mountpoints into qemu namespaces
On Mon, 2018-08-13 at 16:39 +0200, Christian Ehrhardt wrote:
> Libvirt now tries to preserve all mounts under /dev in qemu
> namespaces.
> The old rules only listed a set of known paths but those are no more
> enough.
>
> I found some due to containers like /dev/.lxc/* and such but also
> /dev/console
> and /dev/net/tun.
>
> Libvirt is correct to do so, but we can no more predict the names
> properly, so
> we modify the rule to allow a wildcard based pattern matching what
> libvirt does.
>
> Signed-off-by: Christian Ehrhardt
> ---
> examples/apparmor/usr.sbin.libvirtd | 16 +---
> 1 file changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/examples/apparmor/usr.sbin.libvirtd
> b/examples/apparmor/usr.sbin.libvirtd
> index 3ff43c32a2..b2e38fe0ad 100644
> --- a/examples/apparmor/usr.sbin.libvirtd
> +++ b/examples/apparmor/usr.sbin.libvirtd
> @@ -33,17 +33,11 @@
>mount options=(rw,rslave) -> /,
>mount options=(rw, nosuid) -> /{var/,}run/libvirt/qemu/*.dev/,
>
> - mount options=(rw, move) /dev/ ->
> /{var/,}run/libvirt/qemu/*.dev/,
> - mount options=(rw, move) /dev/hugepages/ ->
> /{var/,}run/libvirt/qemu/*.hugepages/,
> - mount options=(rw, move) /dev/mqueue/->
> /{var/,}run/libvirt/qemu/*.mqueue/,
> - mount options=(rw, move) /dev/pts/ ->
> /{var/,}run/libvirt/qemu/*.pts/,
> - mount options=(rw, move) /dev/shm/ ->
> /{var/,}run/libvirt/qemu/*.shm/,
> -
> - mount options=(rw, move) /{var/,}run/libvirt/qemu/*.dev/ ->
> /dev/,
> - mount options=(rw, move) /{var/,}run/libvirt/qemu/*.hugepages/ ->
> /dev/hugepages/,
> - mount options=(rw, move) /{var/,}run/libvirt/qemu/*.mqueue/->
> /dev/mqueue/,
> - mount options=(rw, move) /{var/,}run/libvirt/qemu/*.pts/ ->
> /dev/pts/,
> - mount options=(rw, move) /{var/,}run/libvirt/qemu/*.shm/ ->
> /dev/shm/,
> + # libvirt provides any mounts under /dev to qemu namespaces
> + mount options=(rw, move) /dev/ -> /{var/,}run/libvirt/qemu/*.dev/,
> + mount options=(rw, move) /dev/**{/,} ->
> /{var/,}run/libvirt/qemu/*{/,},
What are you trying to convey with this rule? As written, the '{/,}' is
redundant since '**' will match that.
> + mount options=(rw, move) /{var/,}run/libvirt/qemu/*.dev/ -> /dev/,
> + mount options=(rw, move) /{var/,}run/libvirt/qemu/*{/,} ->
> /dev/**{/,},
ditto
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/4] apparmor: allow expected /tmp access patterns
On Mon, 2018-08-13 at 16:39 +0200, Christian Ehrhardt wrote:
> Several cases were found needing /tmp, for example ceph will try to
> list /tmp
> and the samba feature of qemu will place things in /tmp/qemu-smb.*.
> This is sort of safe because:
> - While /tmp could contain anything it is not recommended to put
> critical
>data there anyway
> - We restrict general access to only dir listing and reading of
> files owned
>(intentionally not the full power of user-tmp abstraction)
> - While it would be hard to predict the PID as part of the string
> for the
>qemu smb feature (this is not exposed through XML so virt-aa-
> helper
>can't help) it is guarded by the "owner" statement and a pretty
> clear
>qemu-smb infix in the path.
>
> Signed-off-by: Christian Ehrhardt
> ---
> examples/apparmor/libvirt-qemu | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/examples/apparmor/libvirt-qemu
> b/examples/apparmor/libvirt-qemu
> index 5caf14e418..c4f231b328 100644
> --- a/examples/apparmor/libvirt-qemu
> +++ b/examples/apparmor/libvirt-qemu
> @@ -180,6 +180,16 @@
># for rbd
>/etc/ceph/ceph.conf r,
>
> + # various functions will need /tmp (e.g. ceph), allow the base dir
> and a
> + # few known functions.
> + # we want to avoid to give blanket read or even write to
> everything under /tmp
> + # so users are expected to add site specific addons for more
> uncommon cases.
> + # allow only dir listing and owner based file read
> + /{,var/}tmp/ r,
> + owner /{,var/}tmp/**/ r,
> + # allow qemu smb feature specific path with write access
> + owner /tmp/qemu-smb.*/{,**} rw,
The actual rule additions are a compromise between security and
usability. Personally I'd prefer they not be there and let admins add
them or allow distros to patch them. That said, the comments and commit
message don't seem quite right for what you are adding. Eg, you mention
ceph, but there is no ceph rule and the profile comment doesn't mention
ceph only needs to list dirs; the profile comments are formatted a bit
weird too.
You mention 'sort of safe', but because of the '/{,var/}tmp/ r,' rule,
you are allowing guests to enumerate all the /tmp/qemu-smb.*
directories and then there is a 'rw' rule for that path. While this is
an 'owner' match, in practice, VMs all run under the same uid so this
means a rogue vm would be allowed to access files in these directories.
That said, I don't know what qemu is setting up in there-- so maybe it
is designed in such a way that this doesn't matter.
I'd much rather not call this 'sort of safe' but instead call out the
problem, justify why the rule should be there and perhaps add a TODO
that once smb is supported in domain xml that this rule will be added
conditionally.
+0 for rule inclusion provided the comments and commit message are
addressed. +1 if it can be demonstrated that qemu is handling those
dirs safely wrt vm isolation.
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/4] apparmor: add mediation rules for unconfined guests
On Mon, 2018-08-13 at 16:39 +0200, Christian Ehrhardt wrote: > If a guest runs unconfined , but libvirtd is > confined then the peer for signal can only be detected as > 'unconfined'. That triggers issues like: >apparmor="DENIED" operation="signal" >profile="/usr/sbin/libvirtd" pid=22395 comm="libvirtd" >requested_mask="send" denied_mask="send" signal=term > peer="unconfined" > > To fix this add unconfined as an allowed peer for those operations. > > I discussed with the apparmor folks, right now there is no better > separation to be made in this case. But there might be further down > the > road with "policy namespaces with scope and view control + stacking" > > This is more a use-case addition than a fix to the following two > changes: > - 3b1d19e6 AppArmor: add rules needed with additional mediation > features > - b482925c apparmor: support ptrace checks > > Signed-off-by: Christian Ehrhardt > Acked-by: Jamie Strandboge > Acked-by: intrigeri > --- > examples/apparmor/usr.sbin.libvirtd | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/examples/apparmor/usr.sbin.libvirtd > b/examples/apparmor/usr.sbin.libvirtd > index dd37866c2a..3ff43c32a2 100644 > --- a/examples/apparmor/usr.sbin.libvirtd > +++ b/examples/apparmor/usr.sbin.libvirtd > @@ -74,6 +74,9 @@ ># unconfined also required if guests run without security module >unix (send, receive) type=stream addr=none > peer=(label=unconfined), > > + # required if guests run unconfined seclabel type='none' but > libvirtd is confined > + signal (read, send) peer=unconfined, A tad unfortunate, but again, the libvirtd profile is meant to be super strict. +1 to apply -- Jamie Strandboge | http://www.canonical.com signature.asc Description: This is a digitally signed message part -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] apparmor: allow openGraphicsFD for virt manager >1.4
On Mon, 2018-08-13 at 16:39 +0200, Christian Ehrhardt wrote:
> virt-manager's UI connection will need socket access for
> openGraphicsFD
> to work - otherwise users will face a failed connection error when
> opening the UI view.
>
> Depending on the exact versions of libvirt and qemu involved this
> needs
> either a rule from qemu to libvirt or vice versa.
>
> Signed-off-by: Christian Ehrhardt
> ---
> examples/apparmor/libvirt-qemu | 3 +++
> examples/apparmor/usr.sbin.libvirtd | 5 +
> 2 files changed, 8 insertions(+)
>
> diff --git a/examples/apparmor/libvirt-qemu
> b/examples/apparmor/libvirt-qemu
> index df5f512487..5caf14e418 100644
> --- a/examples/apparmor/libvirt-qemu
> +++ b/examples/apparmor/libvirt-qemu
> @@ -188,6 +188,9 @@
>@{PROC}/device-tree/** r,
>/sys/firmware/devicetree/** r,
>
> + # allow connect with openGraphicsFD to work
> + unix (send, receive) type=stream addr=none
> peer=(label=/usr/sbin/libvirtd),
+1 to apply
> diff --git a/examples/apparmor/usr.sbin.libvirtd
> b/examples/apparmor/usr.sbin.libvirtd
> index 3102cab382..dd37866c2a 100644
> --- a/examples/apparmor/usr.sbin.libvirtd
> +++ b/examples/apparmor/usr.sbin.libvirtd
> @@ -69,6 +69,11 @@
>unix (send, receive) type=stream addr=none
> peer=(label=/usr/sbin/libvirtd//qemu_bridge_helper),
>signal (send) set=("term")
> peer=/usr/sbin/libvirtd//qemu_bridge_helper,
>
> + # allow connect with openGraphicsFD, direction reversed in newer
> versions
> + unix (send, receive) type=stream addr=none peer=(label=libvirt-[0-
> 9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*),
> + # unconfined also required if guests run without security module
> + unix (send, receive) type=stream addr=none
> peer=(label=unconfined),
Makes sense. This libvirtd policy is meant to be super restrictive, so
+1 to apply.
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] apparmor: fix vfio usage without initial hostdev
On Mon, 2018-06-11 at 13:52 +0200, Christian Ehrhardt wrote: > The base vfio has not much functionality but to provide a custom > container by opening this path. > See https://www.kernel.org/doc/Documentation/vfio.txt for more. > > Systems with static hostdevs will get /dev/vfio/vfio by virt-aa- > hotplug > right from the beginning. But if the guest initially had no hostdev > at > all it will run into the following deny before the security module > labelling callbacks will make the actual vfio device (like > /dev/vfio/93) > known. > > Access by qemu is "wr" even thou in theory it could maybe be "r": > [ 2652.756712] audit: type=1400 audit(1491303691.719:25): > apparmor="DENIED" operation="open" > profile="libvirt-17a61b87-5132-497c-b928-421ac2ee0c8a" > name="/dev/vfio/vfio" pid=8486 comm="qemu-system-x86" > requested_mask="wr" denied_mask="wr" fsuid=64055 ouid=0 > > Bug-Ubuntu: https://bugs.launchpad.net/bugs/1678322 > Bug-Ubuntu: https://bugs.launchpad.net/bugs/1775777 > > Signed-off-by: Christian Ehrhardt > Signed-off-by: Stefan Bader > --- > examples/apparmor/libvirt-qemu | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/examples/apparmor/libvirt-qemu > b/examples/apparmor/libvirt-qemu > index 2c47652250..874aca2092 100644 > --- a/examples/apparmor/libvirt-qemu > +++ b/examples/apparmor/libvirt-qemu > @@ -193,6 +193,9 @@ >deny /dev/shm/lttng-ust-wait-* r, >deny /run/shm/lttng-ust-wait-* r, > > + # for vfio hotplug on systems without static vfio (LP: #1775777) > + /dev/vfio/vfio rw, > + Makes sense. If the guest doesn't start with vfio then libvirtd is going to have to give it to the guest and so libvirtd would need this access. +1 to apply. -- Jamie Strandboge | http://www.canonical.com signature.asc Description: This is a digitally signed message part -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 5/5] virt-aa-helper: test: check for expected profile content
emplate_xml" > "$test_xml" > -testme "0" "serial (dev)" "-r -u $valid_uuid" "$test_xml" > +testme "0" "serial (dev)" "-r -u $valid_uuid" "$test_xml" > "/dev/ttyS0.*rw,$" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,, path='$tmpdir/serial.pipe'/>,g" > "$template_xml" > "$test_xml" > -mkfifo "$tmpdir/serial.pipe.in" "$tmpdir/serial.pipe.out" > +mkfifo "$tmpdir/serial.pipe.in" "$tmpdir/serial.pipe.out" > "$tmpdir/serial.pipe.in.*rw,$" > testme "0" "serial (pipe)" "-r -u $valid_uuid" "$test_xml" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,, path='$tmpdir/console.log'/>,g" > "$template_xml" > "$test_xml" > touch "$tmpdir/console.log" > -testme "0" "console" "-r -u $valid_uuid" "$test_xml" > +testme "0" "console" "-r -u $valid_uuid" "$test_xml" > "$tmpdir/console.log.*rw,$" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,, port='0'/>,g" "$template_xml" > "$test_xml" > testme "0" "console (pty)" "-r -u $valid_uuid" "$test_xml" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,, path='$tmpdir/console.pipe'/> port='0'/>,g" "$template_xml" > "$test_xml" > mkfifo "$tmpdir/console.pipe.in" "$tmpdir/console.pipe.out" > -testme "0" "console (pipe)" "-r -u $valid_uuid" "$test_xml" > +testme "0" "console (pipe)" "-r -u $valid_uuid" "$test_xml" > "$tmpdir/console.pipe.out.*rw,$" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,, port='0'/>,g" "$template_xml" > "$test_xml" > -testme "0" "parallel (pty)" "-r -u $valid_uuid" "$test_xml" > +testme "0" "parallel (pty)" "-r -u $valid_uuid" "$test_xml" > "/dev/pts/0.*rw,$" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,, path='$tmpdir/parallel.pipe'/> port='0'/>,g" "$template_xml" > "$test_xml" > mkfifo "$tmpdir/parallel.pipe.in" "$tmpdir/parallel.pipe.out" > -testme "0" "parallel (pipe)" "-r -u $valid_uuid" "$test_xml" > +testme "0" "parallel (pipe)" "-r -u $valid_uuid" "$test_xml" > "$tmpdir/parallel.pipe.in.*rw,$" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,, path='$tmpdir/guestfwd'/> port='4600'/>,g" "$template_xml" > "$test_xml" > touch "$tmpdir/guestfwd" > -testme "0" "channel (unix)" "-r -u $valid_uuid" "$test_xml" > +testme "0" "channel (unix)" "-r -u $valid_uuid" "$test_xml" > "$tmpdir/guestfwd.*rw,$" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,, type='virtio'/>,g" "$template_xml" > "$test_xml" > testme "0" "channel (pty)" "-r -u $valid_uuid" "$test_xml" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,,$tmpdir/kernel,g" "$template_xml" > > "$test_xml" > touch "$tmpdir/kernel" > -testme "0" "kernel" "-r -u $valid_uuid" "$test_xml" > +testme "0" "kernel" "-r -u $valid_uuid" "$test_xml" > "$tmpdir/kernel.*r,$" > > testfw "ovmf (old path)" "/usr/share/ovmf/OVMF.fd" > testfw "OVMF (new path)" "/usr/share/OVMF/OVMF_CODE.fd" > @@ -333,37 +346,37 @@ testfw "qemu-efi" "/usr/share/qemu- > efi/QEMU_EFI.fd" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,,$tmpdir/initrd,g" "$template_xml" > > "$test_xml" > touch "$tmpdir/initrd" > -testme "0" "initrd" "-r -u $valid_uuid" "$test_xml" > +testme "0" "initrd" "-r -u $valid_uuid" "$test_xml" > "$tmpdir/initrd.*r,$" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,,/boot/kernel,g" "$template_xml" > > "$test_xml" > -testme "0" "kernel in /boot" "-r -u $valid_uuid" "$test_xml" > +testme "0" "kernel in /boot" "-r -u $valid_uuid" "$test_xml" > "/boot/kernel.*r,$" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,,/boot/initrd,g" "$template_xml" > > "$test_xml" > -testme "0" "initrd in /boot" "-r -u $valid_uuid" "$test_xml" > +testme "0" "initrd in /boot" "-r -u $valid_uuid" "$test_xml" > "/boot/initrd.*r,$" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,,/vmlinuz,g" "$template_xml" > > "$test_xml" > -testme "0" "kernel is /vmlinuz" "-r -u $valid_uuid" "$test_xml" > +testme "0" "kernel is /vmlinuz" "-r -u $valid_uuid" "$test_xml" > "/vmlinuz.*r,$" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,,/initrd/ramdisk,g" "$template_xml" > > "$test_xml" > -testme "0" "initrd is /initrd/ramdisk" "-r -u $valid_uuid" > "$test_xml" > +testme "0" "initrd is /initrd/ramdisk" "-r -u $valid_uuid" > "$test_xml" "/initrd/ramdisk.*r,$" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,,/initrd.img,g" "$template_xml" > > "$test_xml" > -testme "0" "initrd is /initrd.img" "-r -u $valid_uuid" "$test_xml" > +testme "0" "initrd is /initrd.img" "-r -u $valid_uuid" "$test_xml" > "/initrd.img.*r,$" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,<graphics*, xauth='/home/myself/.Xauthority'/>,g" "$template_xml" > "$test_xml" > -testme "0" "sdl Xauthority" "-r -u $valid_uuid" "$test_xml" > +testme "0" "sdl Xauthority" "-r -u $valid_uuid" "$test_xml" > "/home/myself/.Xauthority.*r,$" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" > "$template_xml" > "$test_xml" > -testme "0" "hugepages" "-r -u $valid_uuid -F > /run/hugepages/kvm/\*\*" "$test_xml" > +testme "0" "hugepages" "-r -u $valid_uuid -F > /run/hugepages/kvm/\*\*" "$test_xml" "/run/hugepages/kvm/.*rwk,$" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,<graphics.*>, socket='/var/lib/libvirt/qemu/myself.vnc'> address='0.0.0.0'/>,g" "$template_xml" > "$test_xml" > -testme "0" "vnc socket" "-r -u $valid_uuid" "$test_xml" > +testme "0" "vnc socket" "-r -u $valid_uuid" "$test_xml" > "/var/lib/libvirt/qemu/myself.vnc.*rw,$" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,, evdev='$disk2' />,g" "$template_xml" > "$test_xml" > -testme "0" "input dev passthrough" "-r -u $valid_uuid" "$test_xml" > +testme "0" "input dev passthrough" "-r -u $valid_uuid" "$test_xml" > "$disk2.*rw,$" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,524288,1048576,g" -e > "s,, model='nvdimm'>$disk2 unit='KiB'>5242880,g" > "$template_xml" > "$test_xml" > -testme "0" "nvdimm" "-r -u $valid_uuid" "$test_xml" > +testme "0" "nvdimm" "-r -u $valid_uuid" "$test_xml" "$disk2.*rw,$" > > testme "0" "help" "-h" +1 to commit. Thanks! -- Jamie Strandboge | http://www.canonical.com signature.asc Description: This is a digitally signed message part -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 5/5] virt-aa-helper: test: check for expected profile content
##UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,, port='0'/>,g" "$template_xml" > "$test_xml" > testme "0" "serial (pty)" "-r -u $valid_uuid" "$test_xml" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,, port='0'/>,g" "$template_xml" > "$test_xml" > -testme "0" "serial (dev)" "-r -u $valid_uuid" "$test_xml" > +testme "0" "serial (dev)" "-r -u $valid_uuid" "$test_xml" > "/dev/ttyS0.*rw,$" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,, path='$tmpdir/serial.pipe'/>,g" > "$template_xml" > "$test_xml" > -mkfifo "$tmpdir/serial.pipe.in" "$tmpdir/serial.pipe.out" > +mkfifo "$tmpdir/serial.pipe.in" "$tmpdir/serial.pipe.out" > "$tmpdir/serial.pipe.in.*rw,$" > testme "0" "serial (pipe)" "-r -u $valid_uuid" "$test_xml" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,, path='$tmpdir/console.log'/>,g" > "$template_xml" > "$test_xml" > touch "$tmpdir/console.log" > -testme "0" "console" "-r -u $valid_uuid" "$test_xml" > +testme "0" "console" "-r -u $valid_uuid" "$test_xml" > "$tmpdir/console.log.*rw,$" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,, port='0'/>,g" "$template_xml" > "$test_xml" > testme "0" "console (pty)" "-r -u $valid_uuid" "$test_xml" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,, path='$tmpdir/console.pipe'/> port='0'/>,g" "$template_xml" > "$test_xml" > mkfifo "$tmpdir/console.pipe.in" "$tmpdir/console.pipe.out" > -testme "0" "console (pipe)" "-r -u $valid_uuid" "$test_xml" > +testme "0" "console (pipe)" "-r -u $valid_uuid" "$test_xml" > "$tmpdir/console.pipe.out.*rw,$" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,, port='0'/>,g" "$template_xml" > "$test_xml" > -testme "0" "parallel (pty)" "-r -u $valid_uuid" "$test_xml" > +testme "0" "parallel (pty)" "-r -u $valid_uuid" "$test_xml" > "/dev/pts/0.*rw,$" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,, path='$tmpdir/parallel.pipe'/> port='0'/>,g" "$template_xml" > "$test_xml" > mkfifo "$tmpdir/parallel.pipe.in" "$tmpdir/parallel.pipe.out" > -testme "0" "parallel (pipe)" "-r -u $valid_uuid" "$test_xml" > +testme "0" "parallel (pipe)" "-r -u $valid_uuid" "$test_xml" > "$tmpdir/parallel.pipe.in.*rw,$" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,, path='$tmpdir/guestfwd'/> port='4600'/>,g" "$template_xml" > "$test_xml" > touch "$tmpdir/guestfwd" > -testme "0" "channel (unix)" "-r -u $valid_uuid" "$test_xml" > +testme "0" "channel (unix)" "-r -u $valid_uuid" "$test_xml" > "$tmpdir/guestfwd.*rw,$" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,, type='virtio'/>,g" "$template_xml" > "$test_xml" > testme "0" "channel (pty)" "-r -u $valid_uuid" "$test_xml" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,,$tmpdir/kernel,g" "$template_xml" > > "$test_xml" > touch "$tmpdir/kernel" > -testme "0" "kernel" "-r -u $valid_uuid" "$test_xml" > +testme "0" "kernel" "-r -u $valid_uuid" "$test_xml" > "$tmpdir/kernel.*r,$" > > testfw "ovmf (old path)" "/usr/share/ovmf/OVMF.fd" > testfw "OVMF (new path)" "/usr/share/OVMF/OVMF_CODE.fd" > @@ -333,37 +347,37 @@ testfw "qemu-efi" "/usr/share/qemu- > efi/QEMU_EFI.fd" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,,$tmpdir/initrd,g" "$template_xml" > > "$test_xml" > touch "$tmpdir/initrd" > -testme "0" "initrd" "-r -u $valid_uuid" "$test_xml" > +testme "0" "initrd" "-r -u $valid_uuid" "$test_xml" > "$tmpdir/initrd.*r,$" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,,/boot/kernel,g" "$template_xml" > > "$test_xml" > -testme "0" "kernel in /boot" "-r -u $valid_uuid" "$test_xml" > +testme "0" "kernel in /boot" "-r -u $valid_uuid" "$test_xml" > "/boot/kernel.*r,$" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,,/boot/initrd,g" "$template_xml" > > "$test_xml" > -testme "0" "initrd in /boot" "-r -u $valid_uuid" "$test_xml" > +testme "0" "initrd in /boot" "-r -u $valid_uuid" "$test_xml" > "/boot/initrd.*r,$" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,,/vmlinuz,g" "$template_xml" > > "$test_xml" > -testme "0" "kernel is /vmlinuz" "-r -u $valid_uuid" "$test_xml" > +testme "0" "kernel is /vmlinuz" "-r -u $valid_uuid" "$test_xml" > "/vmlinuz.*r,$" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,,/initrd/ramdisk,g" "$template_xml" > > "$test_xml" > -testme "0" "initrd is /initrd/ramdisk" "-r -u $valid_uuid" > "$test_xml" > +testme "0" "initrd is /initrd/ramdisk" "-r -u $valid_uuid" > "$test_xml" "/initrd/ramdisk.*r,$" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,,/initrd.img,g" "$template_xml" > > "$test_xml" > -testme "0" "initrd is /initrd.img" "-r -u $valid_uuid" "$test_xml" > +testme "0" "initrd is /initrd.img" "-r -u $valid_uuid" "$test_xml" > "/initrd.img.*r,$" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,<graphics*, xauth='/home/myself/.Xauthority'/>,g" "$template_xml" > "$test_xml" > -testme "0" "sdl Xauthority" "-r -u $valid_uuid" "$test_xml" > +testme "0" "sdl Xauthority" "-r -u $valid_uuid" "$test_xml" > "/home/myself/.Xauthority.*r,$" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" > "$template_xml" > "$test_xml" > -testme "0" "hugepages" "-r -u $valid_uuid -F > /run/hugepages/kvm/\*\*" "$test_xml" > +testme "0" "hugepages" "-r -u $valid_uuid -F > /run/hugepages/kvm/\*\*" "$test_xml" "/run/hugepages/kvm/.*rwk,$" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,<graphics.*>, socket='/var/lib/libvirt/qemu/myself.vnc'> address='0.0.0.0'/>,g" "$template_xml" > "$test_xml" > -testme "0" "vnc socket" "-r -u $valid_uuid" "$test_xml" > +testme "0" "vnc socket" "-r -u $valid_uuid" "$test_xml" > "/var/lib/libvirt/qemu/myself.vnc.*rw,$" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,, evdev='$disk2' />,g" "$template_xml" > "$test_xml" > -testme "0" "input dev passthrough" "-r -u $valid_uuid" "$test_xml" > +testme "0" "input dev passthrough" "-r -u $valid_uuid" "$test_xml" > "$disk2.*rw,$" > > sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e > "s,524288,1048576,g" -e > "s,, model='nvdimm'>$disk2 unit='KiB'>5242880,g" > "$template_xml" > "$test_xml" > -testme "0" "nvdimm" "-r -u $valid_uuid" "$test_xml" > +testme "0" "nvdimm" "-r -u $valid_uuid" "$test_xml" "$disk2.*rw,$" > > testme "0" "help" "-h" > Nice! I love these test additions. :) -- Jamie Strandboge | http://www.canonical.com signature.asc Description: This is a digitally signed message part -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 4/5] virt-aa-helper: generate rules for nvdimm memory
On Wed, 2018-03-21 at 15:53 +0100, Christian Ehrhardt wrote:
> nvdimm memory is backed by a path on the host. This currently works
> only via
> hotplug where the AppArmor label is created via the domain label
> callbacks.
>
> This adds the virt-aa-helper support for nvdimm memory devices to
> generate
> rules for the needed paths from the initial guest definition as well.
>
> Example in domain xml:
>
>
> /tmp/nvdimm-base
>
>
> 524288
> 0
>
>
> Works to start now and creates:
> "/tmp/nvdimm-base" rw,
>
> Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1757085
>
> Acked-by: Jamie Strandboge <ja...@canonical.com>
> Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
> ---
> src/security/virt-aa-helper.c | 8
> tests/virt-aa-helper-test | 3 +++
> 2 files changed, 11 insertions(+)
>
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-
> helper.c
> index ad1371d..a1bc109 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -1177,6 +1177,14 @@ get_files(vahControl * ctl)
> }
> }
>
> +for (i = 0; i < ctl->def->nmems; i++) {
> +if (ctl->def->mems[i] &&
> +ctl->def->mems[i]->model ==
> VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
> +if (vah_add_file(, ctl->def->mems[i]->nvdimmPath,
> "rw") != 0)
> +goto cleanup;
> +}
> +}
> +
> if (ctl->def->virtType == VIR_DOMAIN_VIRT_KVM) {
> for (i = 0; i < ctl->def->nnets; i++) {
> virDomainNetDefPtr net = ctl->def->nets[i];
> diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test
> index 054269c..7c839e4 100755
> --- a/tests/virt-aa-helper-test
> +++ b/tests/virt-aa-helper-test
> @@ -362,6 +362,9 @@ testme "0" "vnc socket" "-r -u $valid_uuid"
> "$test_xml"
> sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e
> "s,, evdev='$disk2' />,g" "$template_xml" > "$test_xml"
> testme "0" "input dev passthrough" "-r -u $valid_uuid" "$test_xml"
>
> +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e
> "s,524288,1048576,g" -e
> "s,, model='nvdimm'>$disk2 unit='KiB'>5242880,g"
> "$template_xml" > "$test_xml"
> +testme "0" "nvdimm" "-r -u $valid_uuid" "$test_xml"
> +
> testme "0" "help" "-h"
>
> echo "" >$output
+1 to apply
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 3/5] virt-aa-helper: generate rules for passthrough input devices
On Wed, 2018-03-21 at 15:53 +0100, Christian Ehrhardt wrote:
> Input devices can passthrough an event device. This currently works
> only via
> hotplug where the AppArmor label is created via the domain label
> callbacks.
>
> This adds the virt-aa-helper support for passthrough input devices to
> generate
> rules for the needed paths from the initial guest definition as well.
>
> Example in domain xml:
>
>
>
> Works to start now and creates:
> "/dev/input/event0" rw,
>
> Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1757085
>
> Acked-by: Jamie Strandboge <ja...@canonical.com>
> Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
> ---
> src/security/virt-aa-helper.c | 8
> tests/virt-aa-helper-test | 3 +++
> 2 files changed, 11 insertions(+)
>
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-
> helper.c
> index 456cfce..ad1371d 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -1157,6 +1157,14 @@ get_files(vahControl * ctl)
> }
> }
>
> +for (i = 0; i < ctl->def->ninputs; i++) {
> +if (ctl->def->inputs[i] &&
> +ctl->def->inputs[i]->type ==
> VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH) {
> +if (vah_add_file(, ctl->def->inputs[i]-
> >source.evdev, "rw") != 0)
> +goto cleanup;
> +}
> +}
> +
> for (i = 0; i < ctl->def->nnets; i++) {
> if (ctl->def->nets[i] &&
> ctl->def->nets[i]->type ==
> VIR_DOMAIN_NET_TYPE_VHOSTUSER &&
> diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test
> index 1e96b8e..054269c 100755
> --- a/tests/virt-aa-helper-test
> +++ b/tests/virt-aa-helper-test
> @@ -359,6 +359,9 @@ testme "0" "hugepages" "-r -u $valid_uuid -F
> /run/hugepages/kvm/\*\*" "$test_xml
> sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e
> "s,<graphics.*>, socket='/var/lib/libvirt/qemu/myself.vnc'> address='0.0.0.0'/>,g" "$template_xml" > "$test_xml"
> testme "0" "vnc socket" "-r -u $valid_uuid" "$test_xml"
>
> +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e
> "s,, evdev='$disk2' />,g" "$template_xml" > "$test_xml"
> +testme "0" "input dev passthrough" "-r -u $valid_uuid" "$test_xml"
> +
> testme "0" "help" "-h"
>
> echo "" >$output
+1 to apply
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 2/5] security, apparmor: add (Set|Restore)InputLabel
On Wed, 2018-03-21 at 15:53 +0100, Christian Ehrhardt wrote:
> d8116b5a "security: Introduce functions for input device hot(un)plug"
> implemented the code (Set|Restore)InputLabel for several security
> modules,
> this patch adds an AppArmor implementation for it as well.
>
> That fixes hot-plugging event input devices by generating a rule for
> the
> path that needs to be accessed.
>
> Example hot adding:
>
>
>
> Creates now:
> "/dev/input/event0" rwk,
>
> Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1755153
>
> Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
> ---
> src/security/security_apparmor.c | 48
>
> 1 file changed, 48 insertions(+)
>
> diff --git a/src/security/security_apparmor.c
> b/src/security/security_apparmor.c
> index 18908c8..92acc9e 100644
> --- a/src/security/security_apparmor.c
> +++ b/src/security/security_apparmor.c
> @@ -761,6 +761,51 @@ AppArmorRestoreMemoryLabel(virSecurityManagerPtr
> mgr,
>
> /* Called when hotplugging */
> static int
> +AppArmorSetInputLabel(virSecurityManagerPtr mgr,
> + virDomainDefPtr def,
> + virDomainInputDefPtr input)
> +{
> +if (input == NULL)
> +return 0;
> +
> +switch ((virDomainInputType) input->type) {
> +case VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH:
> +if (input->source.evdev == NULL) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("%s: passthrough input device has no
> source"),
> + __func__);
> +return -1;
> +}
> +if (!virFileExists(input->source.evdev)) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("%s: \'%s\' does not exist"),
> + __func__, input->source.evdev);
> +return -1;
> +}
> +return reload_profile(mgr, def, input->source.evdev, true);
> +break;
> +
> +case VIR_DOMAIN_INPUT_TYPE_MOUSE:
> +case VIR_DOMAIN_INPUT_TYPE_TABLET:
> +case VIR_DOMAIN_INPUT_TYPE_KBD:
> +case VIR_DOMAIN_INPUT_TYPE_LAST:
> +break;
> +}
> +
> +return 0;
> +}
> +
> +
> +static int
> +AppArmorRestoreInputLabel(virSecurityManagerPtr mgr,
> + virDomainDefPtr def,
> + virDomainInputDefPtr input
> ATTRIBUTE_UNUSED)
> +{
> +return reload_profile(mgr, def, NULL, false);
> +}
> +
> +/* Called when hotplugging */
> +static int
> AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
>virDomainDefPtr def,
>virStorageSourcePtr src)
> @@ -1161,6 +1206,9 @@ virSecurityDriver virAppArmorSecurityDriver = {
> .domainSetSecurityMemoryLabel = AppArmorSetMemoryLabel,
> .domainRestoreSecurityMemoryLabel =
> AppArmorRestoreMemoryLabel,
>
> +.domainSetSecurityInputLabel= AppArmorSetInputLabel,
> + .domainRestoreSecurityInputLabel= AppArmorRestoreInputLabel,
> +
> .domainSetSecurityDaemonSocketLabel =
> AppArmorSetSecurityDaemonSocketLabel,
> .domainSetSecuritySocketLabel =
> AppArmorSetSecuritySocketLabel,
> .domainClearSecuritySocketLabel =
> AppArmorClearSecuritySocketLabel,
+1 to apply
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 1/5] security, apparmor: add (Set|Restore)MemoryLabel
On Wed, 2018-03-21 at 15:53 +0100, Christian Ehrhardt wrote:
> Recent changes have made implementing this mandatory to hot add any
> memory.
> Implementing this in apparmor fixes this as well as allows hot-add of
> nvdimm
> tpye memory with an nvdimmPath set generating a AppArmor rule for
> that
> path.
>
> Example hot adding:
>
>
> /tmp/nvdimm-test
>
>
> 524288
> 0
>
>
> Creates now:
> "/tmp/nvdimm-test" rwk,
>
> Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1755153
>
> Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
> ---
> src/security/security_apparmor.c | 46
>
> 1 file changed, 46 insertions(+)
>
> diff --git a/src/security/security_apparmor.c
> b/src/security/security_apparmor.c
> index a989992..18908c8 100644
> --- a/src/security/security_apparmor.c
> +++ b/src/security/security_apparmor.c
> @@ -718,6 +718,49 @@
> AppArmorRestoreSecurityDiskLabel(virSecurityManagerPtr mgr,
>
> /* Called when hotplugging */
> static int
> +AppArmorSetMemoryLabel(virSecurityManagerPtr mgr,
> + virDomainDefPtr def,
> + virDomainMemoryDefPtr mem)
> +{
> +if (mem == NULL)
> +return 0;
> +
> +switch ((virDomainMemoryModel) mem->model) {
> +case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
> +if (mem->nvdimmPath == NULL) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("%s: nvdimm without a path"),
> + __func__);
> +return -1;
> +}
> +if (!virFileExists(mem->nvdimmPath)) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("%s: \'%s\' does not exist"),
> + __func__, mem->nvdimmPath);
> +return -1;
> +}
> +return reload_profile(mgr, def, mem->nvdimmPath, true);
> +break;
> +case VIR_DOMAIN_MEMORY_MODEL_NONE:
> +case VIR_DOMAIN_MEMORY_MODEL_DIMM:
> +case VIR_DOMAIN_MEMORY_MODEL_LAST:
> +break;
> +}
> +
> +return 0;
> +}
> +
> +
> +static int
> +AppArmorRestoreMemoryLabel(virSecurityManagerPtr mgr,
> + virDomainDefPtr def,
> + virDomainMemoryDefPtr mem
> ATTRIBUTE_UNUSED)
> +{
> +return reload_profile(mgr, def, NULL, false);
> +}
> +
> +/* Called when hotplugging */
> +static int
> AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
>virDomainDefPtr def,
>virStorageSourcePtr src)
> @@ -1115,6 +1158,9 @@ virSecurityDriver virAppArmorSecurityDriver = {
> .domainSetSecurityImageLabel=
> AppArmorSetSecurityImageLabel,
> .domainRestoreSecurityImageLabel=
> AppArmorRestoreSecurityImageLabel,
>
> +.domainSetSecurityMemoryLabel = AppArmorSetMemoryLabel,
> +.domainRestoreSecurityMemoryLabel =
> AppArmorRestoreMemoryLabel,
> +
> .domainSetSecurityDaemonSocketLabel =
> AppArmorSetSecurityDaemonSocketLabel,
> .domainSetSecuritySocketLabel =
> AppArmorSetSecuritySocketLabel,
> .domainClearSecuritySocketLabel =
> AppArmorClearSecuritySocketLabel,
+1 to apply. Thanks!
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 4/5] virt-aa-helper: generate rules for nvdimm memory
On Wed, 2018-03-21 at 13:10 +0100, Christian Ehrhardt wrote:
> nvdimm memory is backed by a path on the host. This currently works
> only via
> hotplug where the AppArmor label is created via the domain label
> callbacks.
>
> This adds the virt-aa-helper support for nvdimm memory devices to
> generate
> rules for the needed paths from the initial guest definition as well.
>
> Example in domain xml:
>
>
> /tmp/nvdimm-base
>
>
> 524288
> 0
>
>
> Works to start now and creates:
> "/tmp/nvdimm-base" rw,
>
> Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1757085
>
> Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
> ---
> src/security/virt-aa-helper.c | 8
> tests/virt-aa-helper-test | 3 +++
> 2 files changed, 11 insertions(+)
>
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-
> helper.c
> index ad1371d..a1bc109 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -1177,6 +1177,14 @@ get_files(vahControl * ctl)
> }
> }
>
> +for (i = 0; i < ctl->def->nmems; i++) {
> +if (ctl->def->mems[i] &&
> +ctl->def->mems[i]->model ==
> VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
> +if (vah_add_file(, ctl->def->mems[i]->nvdimmPath,
> "rw") != 0)
> +goto cleanup;
> +}
> +}
> +
> if (ctl->def->virtType == VIR_DOMAIN_VIRT_KVM) {
> for (i = 0; i < ctl->def->nnets; i++) {
> virDomainNetDefPtr net = ctl->def->nets[i];
> diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test
> index 054269c..7c839e4 100755
> --- a/tests/virt-aa-helper-test
> +++ b/tests/virt-aa-helper-test
> @@ -362,6 +362,9 @@ testme "0" "vnc socket" "-r -u $valid_uuid"
> "$test_xml"
> sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e
> "s,, evdev='$disk2' />,g" "$template_xml" > "$test_xml"
> testme "0" "input dev passthrough" "-r -u $valid_uuid" "$test_xml"
>
> +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e
> "s,524288,1048576,g" -e
> "s,, model='nvdimm'>$disk2 unit='KiB'>5242880,g"
> "$template_xml" > "$test_xml"
> +testme "0" "nvdimm" "-r -u $valid_uuid" "$test_xml"
> +
LGTM. +1
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/5] virt-aa-helper: generate rules for passthrough input devices
On Wed, 2018-03-21 at 13:10 +0100, Christian Ehrhardt wrote:
> Input devices can passthrough an event device. This currently works
> only via
> hotplug where the AppArmor label is created via the domain label
> callbacks.
>
> This adds the virt-aa-helper support for passthrough input devices to
> generate
> rules for the needed paths from the initial guest definition as well.
>
> Example in domain xml:
>
>
>
> Works to start now and creates:
> "/dev/input/event0" rw,
>
> Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1757085
>
> Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
> ---
> src/security/virt-aa-helper.c | 8
> tests/virt-aa-helper-test | 3 +++
> 2 files changed, 11 insertions(+)
>
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-
> helper.c
> index 456cfce..ad1371d 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -1157,6 +1157,14 @@ get_files(vahControl * ctl)
> }
> }
>
> +for (i = 0; i < ctl->def->ninputs; i++) {
> +if (ctl->def->inputs[i] &&
> +ctl->def->inputs[i]->type ==
> VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH) {
> +if (vah_add_file(, ctl->def->inputs[i]-
> >source.evdev, "rw") != 0)
> +goto cleanup;
> +}
> +}
> +
> for (i = 0; i < ctl->def->nnets; i++) {
> if (ctl->def->nets[i] &&
> ctl->def->nets[i]->type ==
> VIR_DOMAIN_NET_TYPE_VHOSTUSER &&
> diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test
> index 1e96b8e..054269c 100755
> --- a/tests/virt-aa-helper-test
> +++ b/tests/virt-aa-helper-test
> @@ -359,6 +359,9 @@ testme "0" "hugepages" "-r -u $valid_uuid -F
> /run/hugepages/kvm/\*\*" "$test_xml
> sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e
> "s,<graphics.*>, socket='/var/lib/libvirt/qemu/myself.vnc'> address='0.0.0.0'/>,g" "$template_xml" > "$test_xml"
> testme "0" "vnc socket" "-r -u $valid_uuid" "$test_xml"
>
> +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e
> "s,, evdev='$disk2' />,g" "$template_xml" > "$test_xml"
> +testme "0" "input dev passthrough" "-r -u $valid_uuid" "$test_xml"
> +
Man, these tests are pretty ugly. Thanks for adding it! :)
LGTM, +1
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 2/5] security, apparmor: add (Set|Restore)InputLabel
On Wed, 2018-03-21 at 13:10 +0100, Christian Ehrhardt wrote:
> d8116b5a "security: Introduce functions for input device hot(un)plug"
> implemented the code (Set|Restore)InputLabel for several security
> modules,
> this patch adds an AppArmor implementation for it as well.
>
> That fixes hot-plugging event input devices by generating a rule for
> the
> path that needs to be accessed.
>
> Example hot adding:
>
>
>
> Creates now:
> "/dev/input/event0" rwk,
>
> Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1755153
>
> Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
> ---
> src/security/security_apparmor.c | 45
>
> 1 file changed, 45 insertions(+)
>
> diff --git a/src/security/security_apparmor.c
> b/src/security/security_apparmor.c
> index 7509552..3be8eeb 100644
> --- a/src/security/security_apparmor.c
> +++ b/src/security/security_apparmor.c
> @@ -758,6 +758,48 @@ AppArmorRestoreMemoryLabel(virSecurityManagerPtr
> mgr,
>
> /* Called when hotplugging */
> static int
> +AppArmorSetInputLabel(virSecurityManagerPtr mgr,
> + virDomainDefPtr def,
> + virDomainInputDefPtr input)
> +{
> +switch ((virDomainInputType) input->type) {
> +case VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH:
> +if (!virFileExists(input->source.evdev)) {
Check if input->type and input->source are NULL?
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("%s: \'%s\' does not exist"),
> + __func__, input->source.evdev);
> +return -1;
> +}
> +return reload_profile(mgr, def, input->source.evdev, true);
> +break;
> +
> +case VIR_DOMAIN_INPUT_TYPE_MOUSE:
> +case VIR_DOMAIN_INPUT_TYPE_TABLET:
> +case VIR_DOMAIN_INPUT_TYPE_KBD:
> +case VIR_DOMAIN_INPUT_TYPE_LAST:
> +break;
> +}
> +
> +return 0;
> +}
> +
> +
> +static int
> +AppArmorRestoreInputLabel(virSecurityManagerPtr mgr,
> + virDomainDefPtr def,
> + virDomainInputDefPtr input
> ATTRIBUTE_UNUSED)
> +{
> +virSecurityLabelDefPtr secdef =
> +virDomainDefGetSecurityLabelDef(def,
> SECURITY_APPARMOR_NAME);
> +
> +if (!secdef || !secdef->relabel)
> +return 0;
> +
secdef unneeded due to reload_profile.
> +return reload_profile(mgr, def, NULL, false);
> +}
> +
> +/* Called when hotplugging */
> +static int
> AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
>virDomainDefPtr def,
>virStorageSourcePtr src)
> @@ -1158,6 +1200,9 @@ virSecurityDriver virAppArmorSecurityDriver = {
> .domainSetSecurityMemoryLabel = AppArmorSetMemoryLabel,
> .domainRestoreSecurityMemoryLabel =
> AppArmorRestoreMemoryLabel,
>
> +.domainSetSecurityInputLabel= AppArmorSetInputLabel,
> + .domainRestoreSecurityInputLabel= AppArmorRestoreInputLabel,
> +
> .domainSetSecurityDaemonSocketLabel =
> AppArmorSetSecurityDaemonSocketLabel,
> .domainSetSecuritySocketLabel =
> AppArmorSetSecuritySocketLabel,
> .domainClearSecuritySocketLabel =
> AppArmorClearSecuritySocketLabel,
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/5] security, apparmor: add (Set|Restore)MemoryLabel
On Wed, 2018-03-21 at 13:10 +0100, Christian Ehrhardt wrote:
> Recent changes have made implementing this mandatory to hot add any
> memory.
> Implementing this in apparmor fixes this as well as allows hot-add of
> nvdimm
> tpye memory with an nvdimmPath set generating a AppArmor rule for
> that
> path.
>
> Example hot adding:
>
>
> /tmp/nvdimm-test
>
>
> 524288
> 0
>
>
> Creates now:
> "/tmp/nvdimm-test" rwk,
>
> Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1755153
>
> Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
> ---
> src/security/security_apparmor.c | 43
>
> 1 file changed, 43 insertions(+)
>
> diff --git a/src/security/security_apparmor.c
> b/src/security/security_apparmor.c
> index a989992..7509552 100644
> --- a/src/security/security_apparmor.c
> +++ b/src/security/security_apparmor.c
> @@ -718,6 +718,46 @@
> AppArmorRestoreSecurityDiskLabel(virSecurityManagerPtr mgr,
>
> /* Called when hotplugging */
> static int
> +AppArmorSetMemoryLabel(virSecurityManagerPtr mgr,
> + virDomainDefPtr def,
> + virDomainMemoryDefPtr mem)
> +{
> +switch ((virDomainMemoryModel) mem->model) {
Perhaps check if mem->model is not NULL? Sorry for not noticing this
before...
> +case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
> +if (!virFileExists(mem->nvdimmPath)) {
and the same for mem->nvdimmPath?
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("%s: \'%s\' does not exist"),
> + __func__, mem->nvdimmPath);
> +return -1;
> +}
> +return reload_profile(mgr, def, mem->nvdimmPath, true);
> +break;
> +case VIR_DOMAIN_MEMORY_MODEL_NONE:
> +case VIR_DOMAIN_MEMORY_MODEL_DIMM:
> +case VIR_DOMAIN_MEMORY_MODEL_LAST:
> +break;
> +}
> +
> +return 0;
> +}
> +
> +
> +static int
> +AppArmorRestoreMemoryLabel(virSecurityManagerPtr mgr,
> + virDomainDefPtr def,
> + virDomainMemoryDefPtr mem
> ATTRIBUTE_UNUSED)
> +{
> +virSecurityLabelDefPtr secdef =
> +virDomainDefGetSecurityLabelDef(def,
> SECURITY_APPARMOR_NAME);
> +
> +if (!secdef || !secdef->relabel)
> +return 0;
> +
You forgot to remove the secdef tests here too (they are already in
reload_profile).
> +return reload_profile(mgr, def, NULL, false);
> +}
> +
> +/* Called when hotplugging */
> +static int
> AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
>virDomainDefPtr def,
>virStorageSourcePtr src)
> @@ -1115,6 +1155,9 @@ virSecurityDriver virAppArmorSecurityDriver = {
> .domainSetSecurityImageLabel=
> AppArmorSetSecurityImageLabel,
> .domainRestoreSecurityImageLabel=
> AppArmorRestoreSecurityImageLabel,
>
> +.domainSetSecurityMemoryLabel = AppArmorSetMemoryLabel,
> + .domainRestoreSecurityMemoryLabel =
> AppArmorRestoreMemoryLabel,
> +
> .domainSetSecurityDaemonSocketLabel =
> AppArmorSetSecurityDaemonSocketLabel,
> .domainSetSecuritySocketLabel =
> AppArmorSetSecuritySocketLabel,
> .domainClearSecuritySocketLabel =
> AppArmorClearSecuritySocketLabel,
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/4] virt-aa-helper: generate rules for nvdimm memory
On Tue, 2018-03-20 at 13:08 +0100, Christian Ehrhardt wrote:
> nvdimm memory is backed by a path on the host. This currently works
> only via
> hotplug where the AppArmor label is created via the domain label
> callbacks.
>
> This adds the virt-aa-helper support for nvdimm memory devices to
> generate
> rules for the needed paths from the initial guest definition as well.
>
> Example in domain xml:
>
>
> /tmp/nvdimm-base
>
>
> 524288
> 0
>
>
> Works to start now and creates:
> "/tmp/nvdimm-base" rw,
>
> Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1757085
>
> Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
> ---
> src/security/virt-aa-helper.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-
> helper.c
> index 1fc482d..0d0db0b 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -1176,6 +1176,13 @@ get_files(vahControl * ctl)
> }
> }
>
> +for (i = 0; i < ctl->def->nmems; i++) {
> +if (ctl->def->mems[i]->model ==
> VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
'if (ctl->def->mems[i] && ctl->def->mems[i]->model ==
VIR_DOMAIN_MEMORY_MODEL_NVDIMM)', no?
> +if (vah_add_file(, ctl->def->mems[i]->nvdimmPath,
> "rw") != 0)
> +goto cleanup;
> +}
> +}
> +
> if (ctl->def->virtType == VIR_DOMAIN_VIRT_KVM) {
> for (i = 0; i < ctl->def->nnets; i++) {
> virDomainNetDefPtr net = ctl->def->nets[i];
Adding a test case for this would be good.
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/4] virt-aa-helper: generate rules for passthrough input devices
On Tue, 2018-03-20 at 13:08 +0100, Christian Ehrhardt wrote:
> Input devices can passthrough an event device. This currently works
> only via
> hotplug where the AppArmor label is created via the domain label
> callbacks.
>
> This adds the virt-aa-helper support for passthrough input devices to
> generate
> rules for the needed paths from the initial guest definition as well.
>
> Example in domain xml:
>
>
>
> Works to start now and creates:
> "/dev/input/event0" rw,
>
> Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1757085
>
> Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
> ---
> src/security/virt-aa-helper.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-
> helper.c
> index 456cfce..1fc482d 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -1157,6 +1157,13 @@ get_files(vahControl * ctl)
> }
> }
>
> +for (i = 0; i < ctl->def->ninputs; i++) {
> +if (ctl->def->inputs[i]->type ==
> VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH) {
'if (ctl->def->inputs[i] && ctl->def->inputs[i]->type ==
VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH)', no?
> +if (vah_add_file(, ctl->def->inputs[i]-
> >source.evdev, "rw") != 0)
> +goto cleanup;
> +}
> +}
> +
> for (i = 0; i < ctl->def->nnets; i++) {
> if (ctl->def->nets[i] &&
> ctl->def->nets[i]->type ==
> VIR_DOMAIN_NET_TYPE_VHOSTUSER &&
Adding test cases for this would be good.
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/4] security, apparmor: add (Set|Restore)InputLabel
On Tue, 2018-03-20 at 13:08 +0100, Christian Ehrhardt wrote:
> d8116b5a "security: Introduce functions for input device hot(un)plug"
> implemented the code (Set|Restore)InputLabel for several security
> modules,
> this patch adds an AppArmor implementation for it as well.
>
> That fixes hot-plugging event input devices by generating a rule for
> the
> path that needs to be accessed.
>
> Example hot adding:
>
>
>
> Creates now:
> "/dev/input/event0" rwk,
>
> Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
> ---
> src/security/security_apparmor.c | 51
>
> 1 file changed, 51 insertions(+)
>
> diff --git a/src/security/security_apparmor.c
> b/src/security/security_apparmor.c
> index 4ae1e3d..7924b9a 100644
> --- a/src/security/security_apparmor.c
> +++ b/src/security/security_apparmor.c
> @@ -765,6 +765,54 @@ AppArmorRestoreMemoryLabel(virSecurityManagerPtr
> mgr,
>
> /* Called when hotplugging */
> static int
> +AppArmorSetInputLabel(virSecurityManagerPtr mgr,
> + virDomainDefPtr def,
> + virDomainInputDefPtr input)
> +{
> +virSecurityLabelDefPtr secdef;
> +
> +secdef = virDomainDefGetSecurityLabelDef(def,
> SECURITY_APPARMOR_NAME);
> +if (!secdef || !secdef->relabel)
> +return 0;
> +
> +switch ((virDomainInputType) input->type) {
> +case VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH:
> +if (!virFileExists(input->source.evdev)) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("%s: \'%s\' does not exist"),
> + __func__, input->source.evdev);
> +return -1;
> +}
> +return reload_profile(mgr, def, input->source.evdev, true);
> +break;
> +
> +case VIR_DOMAIN_INPUT_TYPE_MOUSE:
> +case VIR_DOMAIN_INPUT_TYPE_TABLET:
> +case VIR_DOMAIN_INPUT_TYPE_KBD:
> +case VIR_DOMAIN_INPUT_TYPE_LAST:
> +break;
> +}
> +
> +return 0;
> +}
> +
> +
> +static int
> +AppArmorRestoreInputLabel(virSecurityManagerPtr mgr,
> + virDomainDefPtr def,
> + virDomainInputDefPtr input
> ATTRIBUTE_UNUSED)
> +{
> +virSecurityLabelDefPtr secdef =
> +virDomainDefGetSecurityLabelDef(def,
> SECURITY_APPARMOR_NAME);
> +
> +if (!secdef || !secdef->relabel)
> +return 0;
> +
> +return reload_profile(mgr, def, NULL, false);
> +}
> +
> +/* Called when hotplugging */
> +static int
> AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
>virDomainDefPtr def,
>virStorageSourcePtr src)
> @@ -1165,6 +1213,9 @@ virSecurityDriver virAppArmorSecurityDriver = {
> .domainSetSecurityMemoryLabel = AppArmorSetMemoryLabel,
> .domainRestoreSecurityMemoryLabel =
> AppArmorRestoreMemoryLabel,
>
> +.domainSetSecurityInputLabel= AppArmorSetInputLabel,
> +.domainRestoreSecurityInputLabel= AppArmorRestoreInputLabel,
> +
> .domainSetSecurityDaemonSocketLabel =
> AppArmorSetSecurityDaemonSocketLabel,
> .domainSetSecuritySocketLabel =
> AppArmorSetSecuritySocketLabel,
> .domainClearSecuritySocketLabel =
> AppArmorClearSecuritySocketLabel,
Same comments on this as for '[PATCH 1/4] security, apparmor: add
(Set|Restore)MemoryLabel'.
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] security, apparmor: add (Set|Restore)MemoryLabel
On Tue, 2018-03-20 at 13:08 +0100, Christian Ehrhardt wrote:
> Recent changes have made implementing this mandatory to hot add any
> memory.
> Implementing this in apparmor fixes this as well as allows hot-add of
> nvdimm
> tpye memory with an nvdimmPath set generating a AppArmor rule for
> that
> path.
>
> Example hot adding:
>
>
> /tmp/nvdimm-test
>
>
> 524288
> 0
>
>
> Creates now:
> "/tmp/nvdimm-test" rwk,
>
> Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
> ---
> src/security/security_apparmor.c | 50
>
> 1 file changed, 50 insertions(+)
>
> diff --git a/src/security/security_apparmor.c
> b/src/security/security_apparmor.c
> index a989992..4ae1e3d 100644
> --- a/src/security/security_apparmor.c
> +++ b/src/security/security_apparmor.c
> @@ -718,6 +718,53 @@
> AppArmorRestoreSecurityDiskLabel(virSecurityManagerPtr mgr,
>
> /* Called when hotplugging */
> static int
> +AppArmorSetMemoryLabel(virSecurityManagerPtr mgr,
> + virDomainDefPtr def,
> + virDomainMemoryDefPtr mem)
> +{
> +virSecurityLabelDefPtr secdef;
> +
> +switch ((virDomainMemoryModel) mem->model) {
> +case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
> +secdef = virDomainDefGetSecurityLabelDef(def,
> SECURITY_APPARMOR_NAME);
> +if (!secdef || !secdef->relabel)
> +return 0;
> +
> +if (!virFileExists(mem->nvdimmPath)) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("%s: \'%s\' does not exist"),
> + __func__, mem->nvdimmPath);
> +return -1;
> +}
> +
> +return reload_profile(mgr, def, mem->nvdimmPath, true);
> +break;
> +case VIR_DOMAIN_MEMORY_MODEL_NONE:
> +case VIR_DOMAIN_MEMORY_MODEL_DIMM:
> +case VIR_DOMAIN_MEMORY_MODEL_LAST:
> +break;
> +}
> +
> +return 0;
> +}
> +
> +
> +static int
> +AppArmorRestoreMemoryLabel(virSecurityManagerPtr mgr,
> + virDomainDefPtr def,
> + virDomainMemoryDefPtr mem
> ATTRIBUTE_UNUSED)
> +{
> +virSecurityLabelDefPtr secdef =
> +virDomainDefGetSecurityLabelDef(def,
> SECURITY_APPARMOR_NAME);
> +
> +if (!secdef || !secdef->relabel)
> +return 0;
> +
This secdef check is done already in reload_profile.
> +return reload_profile(mgr, def, NULL, false);
Calling this with NULL means that virt-aa-helper will be called and
remove the mem file, but will it also remove anything else that was
hotplugged and not present in 'xml = virDomainDefFormat(def, ...)'?
I've not looked at this in a while, so maybe it's ok (it seems like
this is what the various other AppArmorRestore* functions do after
all). A test case for hotplugging all these different devices and
hotunplugging each would be good to show if hotunplugging one
inadvertently removes rules for other devices that are still
hotplugged.
> +}
> +
> +/* Called when hotplugging */
> +static int
> AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
>virDomainDefPtr def,
>virStorageSourcePtr src)
> @@ -1115,6 +1162,9 @@ virSecurityDriver virAppArmorSecurityDriver = {
> .domainSetSecurityImageLabel=
> AppArmorSetSecurityImageLabel,
> .domainRestoreSecurityImageLabel=
> AppArmorRestoreSecurityImageLabel,
>
> +.domainSetSecurityMemoryLabel = AppArmorSetMemoryLabel,
> + .domainRestoreSecurityMemoryLabel =
> AppArmorRestoreMemoryLabel,
> +
> .domainSetSecurityDaemonSocketLabel =
> AppArmorSetSecurityDaemonSocketLabel,
> .domainSetSecuritySocketLabel =
> AppArmorSetSecuritySocketLabel,
> .domainClearSecuritySocketLabel =
> AppArmorClearSecuritySocketLabel,
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] apparmor: add ro rule for sasl GSSAPI plugin on /etc/gss/mech.d/
On Wed, 2018-03-07 at 11:16 +0100, Christian Ehrhardt wrote: > If a system has sasl GSSAPI plugin available qemu with sasl support > will > try to read /etc/gss/mech.d/. > > It is required to allow that to let the modules fully work and it > should > be safe to do so as it only registers/configures plugins but has no > secrets. > > Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com> > --- > examples/apparmor/libvirt-qemu | 4 > 1 file changed, 4 insertions(+) > > diff --git a/examples/apparmor/libvirt-qemu > b/examples/apparmor/libvirt-qemu > index 950b042..2c47652 100644 > --- a/examples/apparmor/libvirt-qemu > +++ b/examples/apparmor/libvirt-qemu > @@ -192,3 +192,7 @@ ># silence refusals to open lttng files (see LP: #1432644) >deny /dev/shm/lttng-ust-wait-* r, >deny /run/shm/lttng-ust-wait-* r, > + > + # required for sasl GSSAPI plugin > + /etc/gss/mech.d/ r, > + /etc/gss/mech.d/* r, LGTM. +1 Thanks! -- Jamie Strandboge | http://www.canonical.com signature.asc Description: This is a digitally signed message part -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] apparmor: allow libvirt to send term signal to unconfined
On Wed, 2018-01-24 at 10:41 +0100, intrigeri wrote:
> Hi,
>
>
> Guido Günther:
> > --- a/examples/apparmor/usr.sbin.libvirtd
> > +++ b/examples/apparmor/usr.sbin.libvirtd
> > @@ -63,7 +63,7 @@
> >signal (send) peer=/usr/sbin/dnsmasq,
> >signal (read, send) peer=libvirt-*,
> > - signal (send) set=("kill") peer=unconfined,
> > + signal (send) set=("kill", "term") peer=unconfined,
>
LGTM too. +1 to apply.
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] apparmor: fix virt-aa-helper profile
On Wed, 2018-01-03 at 10:55 +0100, Cédric Bosdonnat wrote:
> Fix rule introduced by commit 0f33025a:
> * to handle /var/run not being a symlink to /run
> * to be properly parsed: missing comma at the end.
> ---
> examples/apparmor/usr.lib.libvirt.virt-aa-helper | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> index 9c822b644..105f09e43 100644
> --- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> +++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> @@ -51,7 +51,7 @@ profile virt-aa-helper
> /usr/{lib,lib64}/libvirt/virt-aa-helper {
>/var/lib/libvirt/images/** r,
>/{media,mnt,opt,srv}/** r,
># For virt-sandbox
> - /run/libvirt/**/[sv]d[a-z] r
> + /{,var/}run/libvirt/**/[sv]d[a-z] r,
>
LGTM. +1 to commit as is.
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virt-aa-helper: handle more disk images
On Mon, 2017-12-11 at 16:23 +0100, Cédric Bosdonnat wrote:
...
> diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> index bd6181d00..f3069d369 100644
> --- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> +++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> @@ -3,6 +3,7 @@
>
> profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper {
>#include
> + #include
>
># needed for searching directories
>capability dac_override,
> @@ -50,8 +51,11 @@ profile virt-aa-helper
> /usr/{lib,lib64}/libvirt/virt-aa-helper {
>/var/lib/libvirt/images/ r,
>/var/lib/libvirt/images/** r,
>/{media,mnt,opt,srv}/** r,
> + # For virt-sandbox
> + /run/libvirt/**/[sv]d[a-z] r
>
>/**.img r,
> + /**.raw r,
>/**.qcow{,2} r,
>/**.qed r,
>/**.vmdk r,
These profile changes LGTM. +1 to apply them. Like intrigeri, I'll let
someone else ACK the build system changes.
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/12] apparmor, libvirt-qemu: Allow access to hugepage mounts
On Wed, 2017-12-20 at 14:43 +0100, Christian Ehrhardt wrote:
> On Tue, Dec 19, 2017 at 5:21 PM, Jamie Strandboge <ja...@canonical.co
> m> wrote:
> > On Tue, 2017-12-19 at 16:03 +0100, Christian Ehrhardt wrote:
> > > From: Serge Hallyn <serge.hal...@ubuntu.com>
> > >
> > > Allows owner access to hugepage mounts (both, the old and
> > > new systemd variant).
> > >
> > > Bug-Ubuntu: https://bugs.launchpad.net/bugs/1250216
> > > Bug-Ubuntu: https://bugs.launchpad.net/bugs/1524737
> > >
> > > Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
> > > ---
> > > examples/apparmor/libvirt-qemu | 4
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/examples/apparmor/libvirt-qemu
> > > b/examples/apparmor/libvirt-qemu
> > > index 912b4ac..bb30530 100644
> > > --- a/examples/apparmor/libvirt-qemu
> > > +++ b/examples/apparmor/libvirt-qemu
> > > @@ -184,6 +184,10 @@
> > >/sys/bus/ r,
> > >/sys/class/ r,
> > >
> > > + # for access to hugepages (LP: #1250216 and LP: #1524737)
> > > + owner "/run/hugepages/kvm/libvirt/qemu/**" rw,
> > > + owner "/dev/hugepages/libvirt/qemu/**" rw,
> > > +
> >
> > These rules are not vm-specific. I'm not familiar with the
> > hugepages
> > feature in libvirt, but the rule suggests that libvirtd will create
> > files in these directories per-vm, and then the vm uses what
> > libvirtd
> > created for it. I see virSecurityManagerSetHugepages() in
> > security_manager.c,
> >
> > is it possible that these rules can be removed and
> > vm-specific ones added dynamically with virt-aa-helper?
>
> I agree - and in general it would also be nice to do a per guest rule
> with a full path to keep the guests away from each other.
> I see different ways to go thou:
>
> #1 - per security_apparmor.c
> The particular function you mention was generalized by [1] and
> further
> reused in [3].
> The creation of the per guest paths is in [2].
>
> So essentially it comes down to a call to domainSetPathLabel with the
> path as an argument.
> But then security_apparmor.c so far does not implement
> domainSetPathLabel at all.
>
> If we add a function for domainSetPathLabel it could call
> reload_profile with the path (as others in security_apparmor.c do).
> But this path is in use by other places as well:
> - qemuDomainWriteMasterKeyFile
> - qemuProcessMakeDir
>
> OTOH domainSetPathLabel is used in those places for the reson of it's
> meaning "add this to the scope of the security label right?"
> So we might after all want to add all these anyway?
>
> If that is so, an (untested) change could be like:
> --- a/src/security/security_apparmor.c
> +++ b/src/security/security_apparmor.c
> @@ -953,6 +953,13 @@ AppArmorSetSavedStateLabel(virSecurityManagerPtr
> mgr,
> return reload_profile(mgr, def, savefile, true);
> }
>
> +static int
> +AppArmorSetPathLabel(virSecurityManagerPtr mgr,
> + virDomainDefPtr def,
> + const char *path)
> +{
> +return reload_profile(mgr, def, path, true);
> +}
>
> static int
> AppArmorRestoreSavedStateLabel(virSecurityManagerPtr mgr,
> @@ -1045,6 +1052,8 @@ virSecurityDriver virAppArmorSecurityDriver = {
> .domainSetSavedStateLabel = AppArmorSetSavedStateLabel,
> .domainRestoreSavedStateLabel =
> AppArmorRestoreSavedStateLabel,
>
> +.domainSetPathLabel = AppArmorSetPathLabel,
> +
> .domainSetSecurityImageFDLabel = AppArmorSetFDLabel,
> .domainSetSecurityTapFDLabel= AppArmorSetFDLabel,
>
>
> #2 in virt-aa-helper with XML parsing
> The next option would to try to detect if hugepages are used from the
> xml description in virt-aa-helper.
>
>
> #3 unconditional per domain rule
> And finally we could avoid too much (?over?)engineering and
> unconditionally add this in virt-aa-helper (at the cost of breaking
> if
> paths generated [2] ever change):
> owner "/dev/hugepages/libvirt/qemu/" rw,
>
>
> IMHO - [1] or [3] - please let me know your opinions before I follow
> any of those paths.
>
> [1]: https://libvirt.org/git/?p=libvirt.git;a=commit;h=ce937d37105c2f
>
Or a combination of 2 and 3. Ie, if hugepages is enabled, add the rule
in 3.
To me, 1 feels most correct cause while the other two fix hugepages,
there seem to be lurking bugs since we aren't implementing
domainSetPathLabel.
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] apparmor, libvirt-qemu: add default pki path of libvirt-spice
On Wed, 2017-12-20 at 12:41 +0100, Christian Ehrhardt wrote:
> Adding the PKI path that is used as default suggestion in
> src/qemu/qemu.conf
> If people use non-default paths they should use local overrides but
> the
> suggested defaults we should open up.
>
> This is the default path as referenced by src/qemu/qemu.conf in
> libvirt.
>
> While doing so merge the several places we have to cover PKI access
> into
> one.
>
> Bug-Ubuntu: https://bugs.launchpad.net/bugs/1690140
>
> Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
> ---
> examples/apparmor/libvirt-qemu | 13 +
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/examples/apparmor/libvirt-qemu
> b/examples/apparmor/libvirt-qemu
> index fa2b753..f206f6c 100644
> --- a/examples/apparmor/libvirt-qemu
> +++ b/examples/apparmor/libvirt-qemu
> @@ -88,8 +88,11 @@
>/usr/share/qemu-efi/** r,
>/usr/share/slof/** r,
>
> - # access PKI infrastructure
> - /etc/pki/libvirt-vnc/** r,
> + # pki for libvirt-vnc and libvirt-spice (LP: #901272, #1690140)
> + /etc/pki/CA/ r,
> + /etc/pki/CA/* r,
> + /etc/pki/libvirt{,-spice,-vnc}/ r,
> + /etc/pki/libvirt{,-spice,-vnc}/** r,
>
># the various binaries
>/usr/bin/kvm rmix,
> @@ -156,12 +159,6 @@
>/usr/{lib,lib64}/qemu/*.so mr,
>/usr/lib/@{multiarch}/qemu/*.so mr,
>
> - # for use by libvirt-vnc (LP: #901272)
> - /etc/pki/CA/ r,
> - /etc/pki/CA/* r,
> - /etc/pki/libvirt/ r,
> - /etc/pki/libvirt/** r,
> -
> # for save and resume
>/{usr/,}bin/dash rmix,
>/{usr/,}bin/dd rmix,
+1 to apply. Thanks for the patch and intrigeri for the feedback.
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] apparmor, virt-aa-helper: Allow access to /sys/bus/usb/devices
On Wed, 2017-12-20 at 11:56 +0100, Christian Ehrhardt wrote:
> From: Jamie Strandboge <ja...@ubuntu.com>
>
> Required to generate correct profiles when using usb passthrough.
>
> Bug-Ubuntu: https://bugs.launchpad.net/bugs/565691
>
> Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
> Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
> Acked-by: Jamie Strandboge <ja...@ubuntu.com>
> Acked-by: Intrigeri <intrig...@boum.org>
> ---
> examples/apparmor/usr.lib.libvirt.virt-aa-helper | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> index d63c844..6a6558e 100644
> --- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> +++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> @@ -22,6 +22,7 @@ profile virt-aa-helper
> /usr/{lib,lib64}/libvirt/virt-aa-helper {
># for hostdev
>/sys/devices/ r,
>/sys/devices/** r,
> + /sys/bus/usb/devices/ r,
+1 to apply
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] apparmor, libvirt-qemu: Allow qemu-block-extra libraries
On Wed, 2017-12-20 at 08:41 +0100, Christian Ehrhardt wrote:
> From: Jamie Strandboge <ja...@ubuntu.com>
>
> Allows (multi-arch enabled) access to libraries under the
> /usr/lib/@{multiarch}/qemu/*.so path in the Debian/Ubuntu
> qemu-block-extra package and all such libs for the paths
> of rpm qemu-block-* packages.
>
> Bug-Ubuntu: https://bugs.launchpad.net/bugs/1554761
>
> Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
> ---
> examples/apparmor/libvirt-qemu | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/examples/apparmor/libvirt-qemu
> b/examples/apparmor/libvirt-qemu
> index 91d0e02..34a564f 100644
> --- a/examples/apparmor/libvirt-qemu
> +++ b/examples/apparmor/libvirt-qemu
> @@ -158,8 +158,9 @@
>/usr/bin/qemu-sparc64 rmix,
>/usr/bin/qemu-unicore32 rmix,
>/usr/bin/qemu-x86_64 rmix,
> - /usr/{lib,lib64}/qemu/block-curl.so mr,
> - /usr/{lib,lib64}/qemu/block-rbd.so mr,
> + # for Debian/Ubuntu qemu-block-extra / RPMs qemu-block-* (LP:
> #1554761)
> + /usr/{lib,lib64}/qemu/*.so mr,
> + /usr/lib/@{multiarch}/qemu/*.so mr,
>
+1 to apply. Thanks for the update. :)
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 11/12] apparmor, virt-aa-helper: Allow access to ecryptfs files
On Tue, 2017-12-19 at 16:03 +0100, Christian Ehrhardt wrote:
> From: Jamie Strandboge <ja...@ubuntu.com>
>
> Bug-Ubuntu: https://bugs.launchpad.net/bugs/591769
>
> Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
> ---
> examples/apparmor/usr.lib.libvirt.virt-aa-helper | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> index bd6181d..d63c844 100644
> --- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> +++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> @@ -47,6 +47,10 @@ profile virt-aa-helper
> /usr/{lib,lib64}/libvirt/virt-aa-helper {
>audit deny @{HOME}/bin/** mrwkl,
>@{HOME}/ r,
>@{HOME}/** r,
> + # Alow access to ecryptfs files (LP: #591769)
> + @{HOME}/.Private/** mrwlk,
> + @{HOMEDIRS}/.ecryptfs/*/.Private/** mrwlk,
> +
Hrmm, these rules were never meant to last as long as they have. That
said, they are already a part of the AppArmor base abstraction (using
owner match though) and virt-aa-helper uses '#include
'. Are these rules still needed considering the base
abstraction? I imagine at worst virt-aa-helper would only need 'r' for
some of these...
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 12/12] apparmor, virt-aa-helper: Allow access to /sys/bus/usb/devices
On Tue, 2017-12-19 at 16:03 +0100, Christian Ehrhardt wrote:
> From: Jamie Strandboge <ja...@ubuntu.com>
>
> Required to generate correct profiles when using usb passthrough.
>
> Bug-Ubuntu: https://bugs.launchpad.net/bugs/565691
>
> Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
> ---
> examples/apparmor/usr.lib.libvirt.virt-aa-helper | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> index d63c844..de92291 100644
> --- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> +++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> @@ -22,6 +22,8 @@ profile virt-aa-helper
> /usr/{lib,lib64}/libvirt/virt-aa-helper {
># for hostdev
>/sys/devices/ r,
>/sys/devices/** r,
> + /sys/bus/usb/devices/ r,
> + /sys/bus/usb/devices/** r,
>deny /dev/sd* r,
>deny /dev/vd* r,
>deny /dev/dm-* r,
Note that only the first rule is needed since /sys/bus/usb/devices/ is
a symlink farm (at least these days). AppArmor resolves symlinks before
applying mediation so the glob rule won't ever match anything.
+1 for first rule.
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 10/12] apparmor, libvirtd: Allow ixr to /var/lib/libvirt/virtd*
On Tue, 2017-12-19 at 16:03 +0100, Christian Ehrhardt wrote:
> From: Jamie Strandboge <ja...@ubuntu.com>
>
> This is required for the ebtables functionality added in
> libvirt 0.8.0.
>
> Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
> ---
> examples/apparmor/usr.sbin.libvirtd | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/examples/apparmor/usr.sbin.libvirtd
> b/examples/apparmor/usr.sbin.libvirtd
> index 8d61d15..2b6b33a 100644
> --- a/examples/apparmor/usr.sbin.libvirtd
> +++ b/examples/apparmor/usr.sbin.libvirtd
> @@ -76,6 +76,10 @@
>/usr/{lib,lib64}/xen/bin/* Ux,
>/usr/lib/xen-*/bin/libxl-save-helper PUx,
>
> + # Required by
> nwfilter_ebiptables_driver.c:ebiptablesWriteToTempFile() to
> + # write and run an ebtables script.
> + /var/lib/libvirt/virtd* ixr,
> +
s/write and run/read and run/, then +1 to apply. The ixr rule makes it
so anything that is executed in there inherits this profile. This
profile is of course super-lenient, but the ix means if in the future
we choose to go more strict, the virtd scripts will also be affected.
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 09/12] apparmor, libvirt-qemu: qemu won't call qemu-nbd
On Tue, 2017-12-19 at 16:03 +0100, Christian Ehrhardt wrote: > While libvirtd might do so, qemu itself as a guest will not need > to call qemu-nbd so remove it from the profile. > > Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com> > Signed-off-by: Stefan Bader <stefan.ba...@canonical.com> > --- > examples/apparmor/libvirt-qemu | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/examples/apparmor/libvirt-qemu > b/examples/apparmor/libvirt-qemu > index eb4d58c..2bffb04 100644 > --- a/examples/apparmor/libvirt-qemu > +++ b/examples/apparmor/libvirt-qemu > @@ -144,7 +144,6 @@ >/usr/bin/qemu-mipsel rmix, >/usr/bin/qemu-mipsn32 rmix, >/usr/bin/qemu-mipsn32el rmix, > - /usr/bin/qemu-nbd rmix, >/usr/bin/qemu-or32 rmix, >/usr/bin/qemu-ppc rmix, >/usr/bin/qemu-ppc64 rmix, Nice catch. +1 to apply. -- Jamie Strandboge | http://www.canonical.com signature.asc Description: This is a digitally signed message part -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/12] apparmor, libvirt-qemu: Allow access to hugepage mounts
On Tue, 2017-12-19 at 16:03 +0100, Christian Ehrhardt wrote: > From: Serge Hallyn <serge.hal...@ubuntu.com> > > Allows owner access to hugepage mounts (both, the old and > new systemd variant). > > Bug-Ubuntu: https://bugs.launchpad.net/bugs/1250216 > Bug-Ubuntu: https://bugs.launchpad.net/bugs/1524737 > > Signed-off-by: Stefan Bader <stefan.ba...@canonical.com> > --- > examples/apparmor/libvirt-qemu | 4 > 1 file changed, 4 insertions(+) > > diff --git a/examples/apparmor/libvirt-qemu > b/examples/apparmor/libvirt-qemu > index 912b4ac..bb30530 100644 > --- a/examples/apparmor/libvirt-qemu > +++ b/examples/apparmor/libvirt-qemu > @@ -184,6 +184,10 @@ >/sys/bus/ r, >/sys/class/ r, > > + # for access to hugepages (LP: #1250216 and LP: #1524737) > + owner "/run/hugepages/kvm/libvirt/qemu/**" rw, > + owner "/dev/hugepages/libvirt/qemu/**" rw, > + These rules are not vm-specific. I'm not familiar with the hugepages feature in libvirt, but the rule suggests that libvirtd will create files in these directories per-vm, and then the vm uses what libvirtd created for it. I see virSecurityManagerSetHugepages() in security_manager.c, is it possible that these rules can be removed and vm-specific ones added dynamically with virt-aa-helper? -- Jamie Strandboge | http://www.canonical.com signature.asc Description: This is a digitally signed message part -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 08/12] apparmor, libvirt-qemu: add generic base vfio device
On Tue, 2017-12-19 at 16:03 +0100, Christian Ehrhardt wrote: > vfio devices are generated on the fly, but the generic base is > missing. > > The base vfio has not much functionality but to provide a custom > container by opening this path. > See https://www.kernel.org/doc/Documentation/vfio.txt for more. > > Current access by qemu is "wr": > [ 2652.756712] audit: type=1400 audit(1491303691.719:25): > apparmor="DENIED" operation="open" > profile="libvirt-17a61b87-5132-497c-b928-421ac2ee0c8a" > name="/dev/vfio/vfio" pid=8486 comm="qemu-system-x86" > requested_mask="wr" denied_mask="wr" fsuid=64055 ouid=0 > > Bug-Ubuntu: https://bugs.launchpad.net/bugs/1678322 > > Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com> > Signed-off-by: Stefan Bader <stefan.ba...@canonical.com> > --- > examples/apparmor/libvirt-qemu | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/examples/apparmor/libvirt-qemu > b/examples/apparmor/libvirt-qemu > index 5d811f9..eb4d58c 100644 > --- a/examples/apparmor/libvirt-qemu > +++ b/examples/apparmor/libvirt-qemu > @@ -212,3 +212,6 @@ ># silence refusals to open lttng files (see LP: #1432644) >deny /dev/shm/lttng-ust-wait-* r, >deny /run/shm/lttng-ust-wait-* r, > + > + # for vfio (LP: #1678322) > + /dev/vfio/vfio rw, Why not just also add this rule iff there is a vfio-specific device rule? Ie, just add this along with the vfio device rule in virt-aa- helper instead of giving all VMs access when it isn't needed. -- Jamie Strandboge | http://www.canonical.com signature.asc Description: This is a digitally signed message part -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 07/12] apparmor, libvirt-qemu: add default pki path of lbvirt-spice
On Tue, 2017-12-19 at 16:03 +0100, Christian Ehrhardt wrote: > Adding the PKI path that is used as default suggestion in > src/qemu/qemu.conf > If people use non-default paths they should use local overrides but > the > suggested defaults we should open up. > > This is the default path as referenced by src/qemu/qemu.conf in > libvirt. > > Bug-Ubuntu: https://bugs.launchpad.net/bugs/1690140 > > Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com> > --- > examples/apparmor/libvirt-qemu | 4 > 1 file changed, 4 insertions(+) > > diff --git a/examples/apparmor/libvirt-qemu > b/examples/apparmor/libvirt-qemu > index bb30530..5d811f9 100644 > --- a/examples/apparmor/libvirt-qemu > +++ b/examples/apparmor/libvirt-qemu > @@ -170,6 +170,10 @@ >/etc/pki/libvirt/ r, >/etc/pki/libvirt/** r, > > + # for use by libvirt-spice (LP: #1690140) > + /etc/pki/libvirt-spice/ r, > + /etc/pki/libvirt-spice/** r, > + +1 to apply -- Jamie Strandboge | http://www.canonical.com signature.asc Description: This is a digitally signed message part -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 05/12] apparmor, libvirt-qemu: Allow qemu-block-extra libraries
On Tue, 2017-12-19 at 16:03 +0100, Christian Ehrhardt wrote:
> From: Jamie Strandboge <ja...@ubuntu.com>
>
> Allows (multi-arch enabled) access to libraries under the
> /usr/lib/@{multiarch}/qemu/*.so path in the Debian/Ubuntu
> qemu-block-extra package.
>
> Bug-Ubuntu: https://bugs.launchpad.net/bugs/1554761
>
> Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
> ---
> examples/apparmor/libvirt-qemu | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/examples/apparmor/libvirt-qemu
> b/examples/apparmor/libvirt-qemu
> index 91d0e02..912b4ac 100644
> --- a/examples/apparmor/libvirt-qemu
> +++ b/examples/apparmor/libvirt-qemu
> @@ -161,6 +161,9 @@
>/usr/{lib,lib64}/qemu/block-curl.so mr,
>/usr/{lib,lib64}/qemu/block-rbd.so mr,
>
> + # for Debian/Ubuntu qemu-block-extra (LP: #1554761)
> + /usr/lib/@{multiarch}/qemu/*.so rm,
> +
+1 as is (though s/rm/mr/ for consistency), but on my system I see
block-curl.so, block-isci.so and block-rdb.so. I think it probably
makes to adjust this rule block to simply be:
/usr/{lib,lib64}/qemu/*.so mr,
/usr/lib/@{multiarch}/qemu/*.so mr,
Ie, rather than limiting the libraries that qemu can mmap that are in
its system library directory, allow qemu access to all of them and then
mediate the accesses those libraries need in policy.
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 02/12] apparmor, libvirt-qemu: Silence lttng related deny messages
On Tue, 2017-12-19 at 16:03 +0100, Christian Ehrhardt wrote: > From: Stefan Bader <stefan.ba...@canonical.com> > > Prevent denial messages related to attempted reads on lttng > files from spamming the logs. > > Bug-Ubuntu: https://bugs.launchpad.net/bugs/1432644 > > Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com> > Signed-off-by: Stefan Bader <stefan.ba...@canonical.com> > --- > examples/apparmor/libvirt-qemu | 4 > 1 file changed, 4 insertions(+) > > diff --git a/examples/apparmor/libvirt-qemu > b/examples/apparmor/libvirt-qemu > index 77c72a5..651d841 100644 > --- a/examples/apparmor/libvirt-qemu > +++ b/examples/apparmor/libvirt-qemu > @@ -191,3 +191,7 @@ >/sys/devices/system/node/ r, >/sys/devices/system/node/node[0-9]*/meminfo r, >/sys/module/vhost/parameters/max_mem_regions r, > + > + # silence refusals to open lttng files (see LP: #1432644) > + deny /dev/shm/lttng-ust-wait-* r, > + deny /run/shm/lttng-ust-wait-* r, +1 to apply. These are noisy and not needed by typical guests. -- Jamie Strandboge | http://www.canonical.com signature.asc Description: This is a digitally signed message part -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 04/12] apparmor, libvirt-qemu: Allow read access to max_mem_regions
On Tue, 2017-12-19 at 16:03 +0100, Christian Ehrhardt wrote: > From: Serge Hallyn <serge.hal...@ubuntu.com> > > Allows read access to /sys/module/vhost/parameters/max_mem_regions. > > Bug-Ubuntu: https://bugs.launchpad.net/bugs/1531564 > > Signed-off-by: Stefan Bader <stefan.ba...@canonical.com> > --- > examples/apparmor/libvirt-qemu | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/examples/apparmor/libvirt-qemu > b/examples/apparmor/libvirt-qemu > index b9e45bd..91d0e02 100644 > --- a/examples/apparmor/libvirt-qemu > +++ b/examples/apparmor/libvirt-qemu > @@ -38,6 +38,8 @@ >/sys/devices/system/node/node[0-9]*/meminfo r, >/sys/devices/system/cpu/ r, > > + /sys/module/vhost/parameters/max_mem_regions r, > + +1 to apply -- Jamie Strandboge | http://www.canonical.com signature.asc Description: This is a digitally signed message part -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/12] apparmor, libvirt-qemu: Allow read access to sysfs system info
On Tue, 2017-12-19 at 16:03 +0100, Christian Ehrhardt wrote:
> From: Jamie Strandboge <ja...@ubuntu.com>
>
> Newer qemu wants to read
> /sys/devices/system/node/
> /sys/devices/system/cpu/
> /sys/devices/system/node/node[0-9]*/meminfo
>
> Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
> ---
> examples/apparmor/libvirt-qemu | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/examples/apparmor/libvirt-qemu
> b/examples/apparmor/libvirt-qemu
> index 651d841..b9e45bd 100644
> --- a/examples/apparmor/libvirt-qemu
> +++ b/examples/apparmor/libvirt-qemu
> @@ -34,6 +34,10 @@
>owner @{PROC}/@{pid}/task/@{tid}/comm rw,
>@{PROC}/sys/kernel/cap_last_cap r,
>
> + /sys/devices/system/node/ r,
> + /sys/devices/system/node/node[0-9]*/meminfo r,
> + /sys/devices/system/cpu/ r,
> +
These read accesses are fine. +1
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] apparmor: allow unix stream for p2p migrations
On Tue, 2017-12-19 at 14:13 +0100, Christian Ehrhardt wrote: > On live migration with --p2p like: > $ virsh migrate --live --p2p kvmguest-bionic-normal \ >qemu+ssh://10.6.221.80/system > > We hit an apparmor deny like: > apparmor="DENIED" operation="file_inherit" > profile="/usr/sbin/libvirtd" pid=23477 comm="ssh" family="unix" > sock_type="stream" protocol=0 requested_mask="send receive" > denied_mask="send" addr=none peer_addr=none peer="unconfined" > > The rule is not perfect, but can't be restricted further at the > moment > (new upstream kernel features needed). For now the lack of a profile > on the > peer as well as comm not being a conditional on rules do not allow to > filter > further. > > Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com> > --- > examples/apparmor/usr.sbin.libvirtd | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/examples/apparmor/usr.sbin.libvirtd > b/examples/apparmor/usr.sbin.libvirtd > index 8d61d15..febe8a4 100644 > --- a/examples/apparmor/usr.sbin.libvirtd > +++ b/examples/apparmor/usr.sbin.libvirtd > @@ -53,6 +53,9 @@ >network packet dgram, >network packet raw, > > + # for --p2p migrations > + unix (send, receive) type=stream addr=none peer=(label=unconfined > addr=none), > + This rule is fine, but for completeness note that the upstream kernel doesn't support unix rules yet. apparmor_parsers that do will handle this rule fine though, so +1 to apply. -- Jamie Strandboge | http://www.canonical.com signature.asc Description: This is a digitally signed message part -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 01/12] apparmor, libvirt-qemu: Allow use of sgabios
On Tue, 2017-12-19 at 16:03 +0100, Christian Ehrhardt wrote: > From: Serge Hallyn <serge.hal...@ubuntu.com> > > Bug-Ubuntu: https://bugs.launchpad.net/bugs/1393548 > > Signed-off-by: Stefan Bader <stefan.ba...@canonical.com> > --- > examples/apparmor/libvirt-qemu | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/examples/apparmor/libvirt-qemu > b/examples/apparmor/libvirt-qemu > index d4fad85..77c72a5 100644 > --- a/examples/apparmor/libvirt-qemu > +++ b/examples/apparmor/libvirt-qemu > @@ -81,6 +81,7 @@ >/usr/share/proll/** r, >/usr/share/vgabios/** r, >/usr/share/seabios/** r, > + /usr/share/misc/sgabios.bin r, >/usr/share/ovmf/** r, > /usr/share/OVMF/** r, >/usr/share/AAVMF/** r, +1 to apply -- Jamie Strandboge | http://www.canonical.com signature.asc Description: This is a digitally signed message part -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] apparmor: add ptrace/mediation rules for unconfined guests
On Fri, 2017-12-15 at 08:52 +0100, Christian Ehrhardt wrote: > If a guest runs unconfined , but libvirtd is > confined then the peer for signal/ptrace can only be detected as > 'unconfined'. That triggers issues like: >apparmor="DENIED" operation="signal" >profile="/usr/sbin/libvirtd" pid=22395 comm="libvirtd" >requested_mask="send" denied_mask="send" signal=term > peer="unconfined" > > To fix this add unconfined as an allowed peer for those operations. > > I discussed with the apparmor folks, right now there is no better > separation to be made in this case. But there might be further down > the > road with "policy namespaces with scope and view control + stacking" > > This is more a use-case addition than a fix to the following two > changes: > - 3b1d19e6 AppArmor: add rules needed with additional mediation > features > - b482925c apparmor: support ptrace checks > > Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com> > --- > examples/apparmor/usr.sbin.libvirtd | 4 > 1 file changed, 4 insertions(+) > > diff --git a/examples/apparmor/usr.sbin.libvirtd > b/examples/apparmor/usr.sbin.libvirtd > index 8d61d15..23e8aa3 100644 > --- a/examples/apparmor/usr.sbin.libvirtd > +++ b/examples/apparmor/usr.sbin.libvirtd > @@ -61,6 +61,10 @@ >signal (send) peer=/usr/sbin/dnsmasq, >signal (read, send) peer=libvirt-*, > > + # required if guests run unconfined seclabel type='none' but > libvirtd is confined > + signal (read, send) peer=unconfined, > + ptrace (trace) peer=unconfined, > + ># Very lenient profile for libvirtd since we want to first focus > on confining ># the guests. Guests will have a very restricted profile. >/ r, These rules are unfortunate, but it is important to note that this is in the libvirtd profile, not the guest profiles. As mentioned in the contextual diff, the profile is intentionally very lenient since libvirtd is necessarily highly trusted. As Christian mentioned, we discussed that this is the best option for the moment. +1 to apply. Thanks for the patch! -- Jamie Strandboge | http://www.canonical.com signature.asc Description: This is a digitally signed message part -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] apparmor: allow qemu abstraction to read /proc/pid/cmdline
On Mon, 2017-12-04 at 12:03 +0100, Michal Privoznik wrote:
> On 12/01/2017 02:26 PM, Jamie Strandboge wrote:
> > On Thu, 2017-11-30 at 10:43 -0700, Jim Fehlig wrote:
> > > Noticed the following denial in audit.log when shutting down
> > > an apparmor confined domain
> > >
> > > type=AVC msg=audit(1512002299.742:131): apparmor="DENIED"
> > > operation="open" profile="libvirt-66154842-e926-4f92-92f0-
> > > 1c1bf61dd1ff"
> > > name="/proc/1475/cmdline" pid=2958 comm="qemu-system-x86"
> > > requested_mask="r" denied_mask="r" fsuid=469 ouid=0
> > >
> > > Squelch the denial by allowing read access to
> > > /proc//cmdline.
> > >
> > > Signed-off-by: Jim Fehlig <jfeh...@suse.com>
> > > ---
> > >
> > > Note: In the audit.log snippet, PID 1475 is libvirtd and 2958 is
> > > the
> > > qemu process. I must admit it is not clear to me why
> > > /proc//cmdline is read on domain shutdown.
> > >
> > > examples/apparmor/libvirt-qemu | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/examples/apparmor/libvirt-qemu
> > > b/examples/apparmor/libvirt-qemu
> > > index 73bdbae87..3d9eed9ec 100644
> > > --- a/examples/apparmor/libvirt-qemu
> > > +++ b/examples/apparmor/libvirt-qemu
> > > @@ -25,6 +25,7 @@
> > >/dev/ptmx rw,
> > >/dev/kqemu rw,
> > >@{PROC}/*/status r,
> > > + @{PROC}/@{pid}/cmdline r,
> >
> > Note this is an information leak and allows reading potentially
> > sensitive information, such as passwords given on the command line.
> > Eg:
> >
> > $ cat /proc/13335/cmdline | tr '\0' ' '
> > sh /tmp/testme --password=sensitive
>
> Well, I'd say that passing passwords (or any sensitive information)
> through command line is doomed by definition. Anybody can read that
> (doing mere ps is enough).
>
> >
> > Would it be possible to use 'owner' match? Eg:
> >
> > owner @{PROC}/@{pid}/cmdline r,
>
> Okay, this narrows the attack surface, but I guess that somebody else
> doing `ps' on the system will be able to obtain the password anyway.
Sure, but what is different here is that the qemu processes are
intended to be confined/untrusted whereas other processes on the system
(eg, the user's shell) are not. Therefore, IMO, we should try to avoid
information leaks in the qemu policy where possible.
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] apparmor: allow qemu abstraction to read /proc/pid/cmdline
On Thu, 2017-11-30 at 10:43 -0700, Jim Fehlig wrote:
> Noticed the following denial in audit.log when shutting down
> an apparmor confined domain
>
> type=AVC msg=audit(1512002299.742:131): apparmor="DENIED"
> operation="open" profile="libvirt-66154842-e926-4f92-92f0-
> 1c1bf61dd1ff"
> name="/proc/1475/cmdline" pid=2958 comm="qemu-system-x86"
> requested_mask="r" denied_mask="r" fsuid=469 ouid=0
>
> Squelch the denial by allowing read access to /proc//cmdline.
>
> Signed-off-by: Jim Fehlig <jfeh...@suse.com>
> ---
>
> Note: In the audit.log snippet, PID 1475 is libvirtd and 2958 is the
> qemu process. I must admit it is not clear to me why
> /proc//cmdline is read on domain shutdown.
>
> examples/apparmor/libvirt-qemu | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/examples/apparmor/libvirt-qemu
> b/examples/apparmor/libvirt-qemu
> index 73bdbae87..3d9eed9ec 100644
> --- a/examples/apparmor/libvirt-qemu
> +++ b/examples/apparmor/libvirt-qemu
> @@ -25,6 +25,7 @@
>/dev/ptmx rw,
>/dev/kqemu rw,
>@{PROC}/*/status r,
> + @{PROC}/@{pid}/cmdline r,
Note this is an information leak and allows reading potentially
sensitive information, such as passwords given on the command line. Eg:
$ cat /proc/13335/cmdline | tr '\0' ' '
sh /tmp/testme --password=sensitive
Would it be possible to use 'owner' match? Eg:
owner @{PROC}/@{pid}/cmdline r,
Either way, I think a comment is warranted above the rule stating it is
an information leak.
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] AppArmor: use fine-grained mount rules instead of a blanket catch-all one
On Sun, 2017-11-05 at 15:29 +, intrigeri+libv...@boum.org wrote:
> From: intrigeri <intrigeri+libv...@boum.org>
>
> This set of rules was proposed by Christian Boltz <appar...@cboltz.de
> >
> on https://bugzilla.opensuse.org/show_bug.cgi?id=1065123.
> ---
> examples/apparmor/usr.sbin.libvirtd | 15 ++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/examples/apparmor/usr.sbin.libvirtd
> b/examples/apparmor/usr.sbin.libvirtd
> index d2831aa491..8d61d154e1 100644
> --- a/examples/apparmor/usr.sbin.libvirtd
> +++ b/examples/apparmor/usr.sbin.libvirtd
> @@ -30,7 +30,20 @@
># Needed for vfio
>capability sys_resource,
>
> - mount,
> + mount options=(rw,rslave) -> /,
> + mount options=(rw, nosuid) -> /{var/,}run/libvirt/qemu/*.dev/,
> +
> + mount options=(rw, move) /dev/ ->
> /{var/,}run/libvirt/qemu/*.dev/,
> + mount options=(rw, move) /dev/hugepages/ ->
> /{var/,}run/libvirt/qemu/*.hugepages/,
> + mount options=(rw, move) /dev/mqueue/->
> /{var/,}run/libvirt/qemu/*.mqueue/,
> + mount options=(rw, move) /dev/pts/ ->
> /{var/,}run/libvirt/qemu/*.pts/,
> + mount options=(rw, move) /dev/shm/ ->
> /{var/,}run/libvirt/qemu/*.shm/,
> +
> + mount options=(rw, move) /{var/,}run/libvirt/qemu/*.dev/ ->
> /dev/,
> + mount options=(rw, move) /{var/,}run/libvirt/qemu/*.hugepages/ ->
> /dev/hugepages/,
> + mount options=(rw, move) /{var/,}run/libvirt/qemu/*.mqueue/->
> /dev/mqueue/,
> + mount options=(rw, move) /{var/,}run/libvirt/qemu/*.pts/ ->
> /dev/pts/,
> + mount options=(rw, move) /{var/,}run/libvirt/qemu/*.shm/ ->
> /dev/shm/,
>
These all look fine. I suspect the rules may need to be fine-tuned for
different distributions, but that's ok, this is example policy and we
can iterate/adjust as needed.
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] AppArmor: add rules needed with additional mediation features brought by Linux 4.14.
On Sun, 2017-11-05 at 15:29 +, intrigeri+libv...@boum.org wrote: > From: intrigeri <intrigeri+libv...@boum.org> > > --- > examples/apparmor/libvirt-qemu | 4 > examples/apparmor/usr.sbin.libvirtd | 6 ++ > 2 files changed, 10 insertions(+) > > diff --git a/examples/apparmor/libvirt-qemu > b/examples/apparmor/libvirt-qemu > index 97dd2d45a9..9d487bf92f 100644 > --- a/examples/apparmor/libvirt-qemu > +++ b/examples/apparmor/libvirt-qemu > @@ -16,6 +16,10 @@ >network inet stream, >network inet6 stream, > > + ptrace (readby, tracedby) peer=/usr/sbin/libvirtd, > + > + signal (receive) peer=/usr/sbin/libvirtd, > + These LGTM >/dev/net/tun rw, >/dev/kvm rw, >/dev/ptmx rw, > diff --git a/examples/apparmor/usr.sbin.libvirtd > b/examples/apparmor/usr.sbin.libvirtd > index 819068ffc3..d2831aa491 100644 > --- a/examples/apparmor/usr.sbin.libvirtd > +++ b/examples/apparmor/usr.sbin.libvirtd > @@ -30,10 +30,13 @@ ># Needed for vfio >capability sys_resource, > > + mount, > + Yuck, but fixed in 2/2. Better might've been to skip this rule and add all the mount rules in 2/2. >network inet stream, >network inet dgram, >network inet6 stream, >network inet6 dgram, > + network netlink raw, Looks fine. Almost certainly needed for udev. >network packet dgram, >network packet raw, > > @@ -42,6 +45,9 @@ >ptrace (trace) peer=/usr/sbin/dnsmasq, >ptrace (trace) peer=libvirt-*, > > + signal (send) peer=/usr/sbin/dnsmasq, > + signal (read, send) peer=libvirt-*, > + LGTM, thanks! -- Jamie Strandboge | http://www.canonical.com signature.asc Description: This is a digitally signed message part -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] apparmor, virt-aa-helper: allow ipv6
On Fri, 2017-11-03 at 09:46 +0100, Christian Ehrhardt wrote:
> In case ipv6 is used the network inet6 permission is required for
> virt-aa-helper.
>
> Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
> ---
> examples/apparmor/usr.lib.libvirt.virt-aa-helper | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> index 012080c..bd6181d 100644
> --- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> +++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> @@ -10,6 +10,7 @@ profile virt-aa-helper
> /usr/{lib,lib64}/libvirt/virt-aa-helper {
>
># needed for when disk is on a network filesystem
>network inet,
> + network inet6,
>
>deny @{PROC}/[0-9]*/mounts r,
>@{PROC}/[0-9]*/net/psched r,
LGTM. Thanks!
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] apparmor: allow qemu to read max_segments
On Fri, 2017-11-03 at 09:46 +0100, Christian Ehrhardt wrote:
> Since qemu 2.9 via 9103f1ce "file-posix: Consider max_segments for
> BlockLimits.max_transfer" this is a new access that is denied by the
> qemu profile.
>
> It is non fatal, but prevents the fix mentioned to actually work.
> It should be safe to allow reading from that path.
>
> Since qemu opens a symlink path we need to translate that for
> apparmor from
> "/sys/dev/block/*/queue/max_segments" to
> "/sys/devices/**/block/*/queue/max_segments"
>
> Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
> ---
> examples/apparmor/libvirt-qemu | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/examples/apparmor/libvirt-qemu
> b/examples/apparmor/libvirt-qemu
> index 97dd2d4..064501f 100644
> --- a/examples/apparmor/libvirt-qemu
> +++ b/examples/apparmor/libvirt-qemu
> @@ -169,6 +169,9 @@
># for rbd
>/etc/ceph/ceph.conf r,
>
> + # for file-posix getting limits since 9103f1ce
> + /sys/devices/**/block/*/queue/max_segments r,
> +
># for ppc device-tree access
>@{PROC}/device-tree/ r,
>@{PROC}/device-tree/** r,
This LGTM. Thanks for the patch!
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] virt-aa-helper-test: only fails go to stdout by default
On Thu, 2017-10-26 at 15:22 +0200, Christian Ehrhardt wrote:
> By Default (without -d) the tests will only print Failures.
> So a log should follow general "no message is a good message" style.
>
> But the testfw checks always emit the skip info to stdout. Instead
> they should use the redirection that is controlled by -d.
>
> This avoids mesages like the following to clutter the log:
> Skipping FW AAVMF32 test. Could not find
> /usr/share/AAVMF/AAVMF32_CODE.fd
>
> Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
> ---
> tests/virt-aa-helper-test | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test
> index e837668..d2a557e 100755
> --- a/tests/virt-aa-helper-test
> +++ b/tests/virt-aa-helper-test
> @@ -155,7 +155,7 @@ testfw() {
> -e "s,, type='pflash'>$fwpath,g" "$template_xml" > "$test_xml"
> testme "0" "$title" "-r -u $valid_uuid" "$test_xml"
> else
> -echo "Skipping FW $title test. Could not find $fwpath"
> +echo "Skipping FW $title test. Could not find $fwpath"
> >$output
> fi
> }
LGTM. Thanks!
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] virt-aa-helper: apparmor wildcards to forbidden chars
On Thu, 2017-10-26 at 15:22 +0200, Christian Ehrhardt wrote:
> Some globbing chars in the domain name could be used to break out of
> apparmor rules, so lets forbid these when in virt-aa-helper.
>
> Also adding a test to ensure all those cases were detected as bad
> char.
>
> Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
> ---
> src/security/virt-aa-helper.c | 2 +-
> tests/virt-aa-helper-test | 20
> 2 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-
> helper.c
> index ef1bf01..4f1c195 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -449,7 +449,7 @@ valid_name(const char *name)
> {
> /* just try to filter out any dangerous characters in the name
> that can be
> * used to subvert the profile */
> -const char *bad = "/[]*";
> +const char *bad = "/[]{}?^,\"*";
>
> if (strlen(name) == 0)
> return -1;
> diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test
> index 0599046..e837668 100755
> --- a/tests/virt-aa-helper-test
> +++ b/tests/virt-aa-helper-test
> @@ -211,6 +211,26 @@ testme "1" "-c with no os.type" "-c -u
> $valid_uuid" "$test_xml"
> sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e
> "s,hvm,hvm_invalid,g" "$template_xml" > "$test_xml"
> testme "1" "-c with invalid hvm" "-c -u $valid_uuid" "$test_xml"
>
> +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,virt-
> aa-helper-test,virt-aa-helper-test-/,g" "$template_xml" > "$test_xml"
> +testme "1" "-c with invalid domain name char /" "-c -u $valid_uuid"
> "$test_xml"
> +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,virt-
> aa-helper-test,virt-aa-helper-test-[,g" "$template_xml" > "$test_xml"
> +testme "1" "-c with invalid domain name char [" "-c -u $valid_uuid"
> "$test_xml"
> +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,virt-
> aa-helper-test,virt-aa-helper-test-],g" "$template_xml" > "$test_xml"
> +testme "1" "-c with invalid domain name char ]" "-c -u $valid_uuid"
> "$test_xml"
> +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,virt-
> aa-helper-test,virt-aa-helper-test-{,g" "$template_xml" > "$test_xml"
> +testme "1" "-c with invalid domain name char {" "-c -u $valid_uuid"
> "$test_xml"
> +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,virt-
> aa-helper-test,virt-aa-helper-test-},g" "$template_xml" > "$test_xml"
> +testme "1" "-c with invalid domain name char }" "-c -u $valid_uuid"
> "$test_xml"
> +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,virt-
> aa-helper-test,virt-aa-helper-test-?,g" "$template_xml" > "$test_xml"
> +testme "1" "-c with invalid domain name char ?" "-c -u $valid_uuid"
> "$test_xml"
> +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,virt-
> aa-helper-test,virt-aa-helper-test-^,g" "$template_xml" > "$test_xml"
> +testme "1" "-c with invalid domain name char ^" "-c -u $valid_uuid"
> "$test_xml"
> +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,virt-
> aa-helper-test,virt-aa-helper-test-\,,g" "$template_xml" >
> "$test_xml"
> +testme "1" "-c with invalid domain name char ," "-c -u $valid_uuid"
> "$test_xml"
> +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,virt-
> aa-helper-test,virt-aa-helper-test-\",g" "$template_xml" >
> "$test_xml"
> +testme "1" "-c with invalid domain name char \"" "-c -u $valid_uuid"
> "$test_xml"
> +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,virt-
> aa-helper-test,virt-aa-helper-test-*,g" "$template_xml" > "$test_xml"
> +testme "1" "-c with invalid domain name char *" "-c -u $valid_uuid"
> "$test_xml"
>
> echo "Expected pass:" >$output
> sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g"
> "$template_xml" > "$test_xml"
LGTM, thanks!
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
