On Mon, May 15, 2017 at 03:23:10PM +0200, Stefan Bader wrote: > From: Serge Hallyn <serge.hallyn(a)ubuntu.com&gt; > > Just because a disk element only requests read access doesn't mean > there may not be another readwrite request. > > Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/bugs/1554031 The URL is wrong (drop the "ubuntu" part. From the bug report this looks like a workaround:

https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1554031/comments/8 or am I misreading this? Shouldn't we only change to 'w' on blockcommit?

Cheers -- Guido > > Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com&gt; > Signed-off-by: Stefan Bader <stefan.bader(a)canonical.com&gt; > --- > 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 5f5d1cd..d976a00 100644 > --- a/src/security/virt-aa-helper.c > +++ b/src/security/virt-aa-helper.c > @@ -888,11 +888,11 @@ add_file_path(virDomainDiskDefPtr disk, > > if (depth == 0) { > if (disk->src->readonly) > - ret = vah_add_file(buf, path, "r"); > + ret = vah_add_file(buf, path, "R"); > else > ret = vah_add_file(buf, path, "rw"); > } else { > - ret = vah_add_file(buf, path, "r"); > + ret = vah_add_file(buf, path, "R"); > } > > if (ret != 0) > -- > 2.7.4 > -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

From: Simon McVittie <smcv(a)debian.org&gt; The split firmware and variables files introduced by https://bugs.debian.org/764918 are in a different directory for some reason. Let the virtual machine read both. Extended by Christian Ehrhardt to generalize FW test (simplifies additional testing on firmware files in future). Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com&gt; Signed-off-by: Stefan Bader <stefan.bader(a)canonical.com&gt; --- examples/apparmor/libvirt-qemu | 1 + src/security/virt-aa-helper.c | 1 + tests/virt-aa-helper-test | 24 ++++++++++++++++-------- 3 files changed, 18 insertions(+), 8 deletions(-) diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu index a9020aa..e0988bb 100644 --- a/examples/apparmor/libvirt-qemu +++ b/examples/apparmor/libvirt-qemu @@ -70,6 +70,7 @@ /usr/share/vgabios/** r, /usr/share/seabios/** r, /usr/share/ovmf/** r, + /usr/share/OVMF/** r, # access PKI infrastructure /etc/pki/libvirt-vnc/** r, diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index d976a00..dd166c2 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -512,6 +512,7 @@ valid_path(const char *path, const bool readonly) "/vmlinuz", "/initrd", "/initrd.img", + "/usr/share/OVMF/", /* for OVMF images */ "/usr/share/ovmf/" /* for OVMF images */ }; /* override the above with these */ diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test index 68e9399..73f3080 100755 --- a/tests/virt-aa-helper-test +++ b/tests/virt-aa-helper-test @@ -145,6 +145,20 @@ testme() { fi } +testfw() { + title="$1" + fwpath="$2" + + if [ -f "$fwpath" ]; then + sed -e "s,###UUID###,$uuid,g" \ + -e "s,###DISK###,$disk1,g" \ + -e "s,</os>,<loader readonly='yes' type='pflash'>$fwpath</loader></os>,g" "$template_xml" > "$test_xml" + testme "0" "$title" "-r -u $valid_uuid" "$test_xml" + else + echo "Skipping FW $title test. Could not find $fwpath" + fi +} + # Expected failures echo "Expected failures:" >$output testme "1" "invalid arg" "-z" @@ -291,14 +305,8 @@ sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</os>,<kernel>$tm touch "$tmpdir/kernel" testme "0" "kernel" "-r -u $valid_uuid" "$test_xml" -if [ -f /usr/share/ovmf/OVMF.fd ]; then - sed -e "s,###UUID###,$uuid,g" \ - -e "s,###DISK###,$disk1,g" \ - -e "s,</os>,<loader readonly='yes' type='pflash'>/usr/share/ovmf/OVMF.fd</loader></os>,g" "$template_xml" > "$test_xml" - testme "0" "ovmf" "-r -u $valid_uuid" "$test_xml" -else - echo "Skipping OVMF test. Could not find /usr/share/ovmf/OVMF.fd" -fi +testfw "ovmf (old path)" "/usr/share/ovmf/OVMF.fd" +testfw "OVMF (new path)" "/usr/share/OVMF/OVMF_CODE.fd" sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</os>,<initrd>$tmpdir/initrd</initrd></os>,g" "$template_xml" > "$test_xml" touch "$tmpdir/initrd" -- 2.7.4

From: William Grant <wgrant(a)ubuntu.com&gt; Allow access to aarch64 UEFI images. Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com&gt; Signed-off-by: Stefan Bader <stefan.bader(a)canonical.com&gt; --- examples/apparmor/libvirt-qemu | 2 ++ src/security/virt-aa-helper.c | 4 +++- tests/virt-aa-helper-test | 2 ++ 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu index e0988bb..89466c9 100644 --- a/examples/apparmor/libvirt-qemu +++ b/examples/apparmor/libvirt-qemu @@ -71,6 +71,8 @@ /usr/share/seabios/** r, /usr/share/ovmf/** r, /usr/share/OVMF/** r, + /usr/share/AAVMF/** r, + /usr/share/qemu-efi/** r, # access PKI infrastructure /etc/pki/libvirt-vnc/** r, diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index dd166c2..a2d5c21 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -513,7 +513,9 @@ valid_path(const char *path, const bool readonly) "/initrd", "/initrd.img", "/usr/share/OVMF/", /* for OVMF images */ - "/usr/share/ovmf/" /* for OVMF images */ + "/usr/share/ovmf/", /* for OVMF images */ + "/usr/share/AAVMF/", /* for AAVMF images */ + "/usr/share/qemu-efi/" /* for AAVMF images */ }; /* override the above with these */ const char * const override[] = { diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test index 73f3080..51072f6 100755 --- a/tests/virt-aa-helper-test +++ b/tests/virt-aa-helper-test @@ -307,6 +307,8 @@ testme "0" "kernel" "-r -u $valid_uuid" "$test_xml" testfw "ovmf (old path)" "/usr/share/ovmf/OVMF.fd" testfw "OVMF (new path)" "/usr/share/OVMF/OVMF_CODE.fd" +testfw "AAVMF" "/usr/share/AAVMF/AAVMF_CODE.fd" +testfw "qemu-efi" "/usr/share/qemu-efi/QEMU_EFI.fd" sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</os>,<initrd>$tmpdir/initrd</initrd></os>,g" "$template_xml" > "$test_xml" touch "$tmpdir/initrd" -- 2.7.4

From: Felix Geyer <fgeyer(a)debian.org&gt; Allow access to libnl-3 config files Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com&gt; Signed-off-by: Stefan Bader <stefan.bader(a)canonical.com&gt; --- 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 4a8f197..ee53c2c 100644 --- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper +++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper @@ -16,6 +16,8 @@ profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper { owner @{PROC}/[0-9]*/status r, @{PROC}/filesystems r, + /etc/libnl-3/classid r, + # for hostdev /sys/devices/ r, /sys/devices/** r, --

From: Felix Geyer <fgeyer(a)debian.org&gt; Add explicit denies for disk devices to avoid cluttering dmesg with (acceptable) denials. Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com&gt; Signed-off-by: Stefan Bader <stefan.bader(a)canonical.com&gt; --- examples/apparmor/usr.lib.libvirt.virt-aa-helper | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper b/examples/apparmor/usr.lib.libvirt.virt-aa-helper index ee53c2c..7804b72 100644 --- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper +++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper @@ -21,6 +21,11 @@ profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper { # for hostdev /sys/devices/ r, /sys/devices/** r, + deny /dev/sd* r, + deny /dev/vd* r, + deny /dev/dm-* r, + deny /dev/mapper/ r, + deny /dev/mapper/* r, /usr/{lib,lib64}/libvirt/virt-aa-helper mr, /{usr/,}sbin/apparmor_parser Ux, -- 2.7.4

From: Christian Ehrhardt <christian.ehrhardt(a)canonical.com&gt; This adds further explicit denies for host devices to silence (acceptable) denial warnings. Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com&gt; Signed-off-by: Stefan Bader <stefan.bader(a)canonical.com&gt; --- 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 7804b72..012080c 100644 --- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper +++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper @@ -24,6 +24,10 @@ profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper { deny /dev/sd* r, deny /dev/vd* r, deny /dev/dm-* r, + deny /dev/drbd[0-9]* r, + deny /dev/dasd* r, + deny /dev/nvme* r, + deny /dev/zd[0-9]* r, deny /dev/mapper/ r, deny /dev/mapper/* r,

On Mon, May 15, 2017 at 03:23:18PM +0200, Stefan Bader wrote: > From: Serge Hallyn <serge.hallyn(a)ubuntu.com&gt; > > Add fowner and fsetid to libvirt-qemu profile and add link > to 9p file options in virt-aa-helper. > > Bug-Ubuntu: https://bugs.launchpad.net/bugs/1378434 > > Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com&gt; > Signed-off-by: Stefan Bader <stefan.bader(a)canonical.com&gt; > --- > examples/apparmor/libvirt-qemu | 4 ++++ > src/security/virt-aa-helper.c | 2 +- > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu > index 89466c9..f04ce04 100644 > --- a/examples/apparmor/libvirt-qemu > +++ b/examples/apparmor/libvirt-qemu > @@ -13,6 +13,10 @@ > capability setgid, > capability setuid, > > + # for 9p > + capability fsetid, > + capability fowner, > + > network inet stream, > network inet6 stream, I would put this into a separate patch. > > diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c > index a2d5c21..667241b 100644 > --- a/src/security/virt-aa-helper.c > +++ b/src/security/virt-aa-helper.c > @@ -1108,7 +1108,7 @@ get_files(vahControl * ctl) > /* We don't need to add deny rw rules for readonly mounts, > * this can only lead to troubles when mounting / readonly. > */ > - if (vah_add_path(&buf, fs->src->path, fs->readonly ? "R" : "rw", true) != 0) > + if (vah_add_path(&buf, fs->src->path, fs->readonly ? "R" : "rwl", true) != 0) Given the recent QEMU 9pfs CVS that allowed to access paths outside src.path I would feel better if the rule produces s.th. like link subset src.path/** -> src.path/**, instead of allowing links to /**.

Cheers, -- Guido > goto cleanup; > } > } > -- > 2.7.4 > -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list