[apparmor] [libvirt] [patch] Add support for OVMF in virt-aa-helper [Was: virt-aa-helper: does not support OVMF?]

[Martin Kletzander]

On Wed, Aug 12, 2015 at 11:19:54AM -0500, Jamie Strandboge wrote:
>On 08/12/2015 10:59 AM, intrigeri at debian.org wrote:
>> Hi,
>>
>> [please Cc me any reply, I'm not subscribed to libvir-list.]
>>
>> it was reported [1] to Ubuntu that virt-aa-helper blocks access to the
>> OVMF files needed to boot UEFI virtual machines in QEMU. After I've
>> confirmed that on Debian sid, Jamie Strandboge suggested a fix.
>> I've successfully tested in my environment (applied on top of 1.2.18)
>> so I'm forwarding it here.
>>
>> [1] https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1483071
>>
>
>diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
>index 4ce1e7a..357dde4 100644
>--- a/src/security/virt-aa-helper.c
>+++ b/src/security/virt-aa-helper.c
>@@ -572,8 +572,9 @@ valid_path(const char *path, const bool readonly)
>     };
>     /* override the above with these */
>     const char * const override[] = {
>-        "/sys/devices/pci",              /* for hostdev pci devices */
>-        "/etc/libvirt-sandbox/services/" /* for virt-sandbox service config */
>+        "/sys/devices/pci",               /* for hostdev pci devices */
>+        "/etc/libvirt-sandbox/services/", /* for virt-sandbox service config */
>+        "/usr/share/ovmf/"                /* for OVMF images */
>     };
>

Disclaimer: I know nearly nothing about the apparmor insides, so
please excuse any inappropriate ideas.

Good catch, this makes sense, but to be strictly precise about this, I
would say this makes the directory accessible for R/W, but readonly
would be enough, wouldn't it?  There could be a small code adjustment,
I'd even dare calling it a clean-up, that would make it possible for
this direcotry to be put in the 'restricted_rw'.  It would change the
semantic a bit, but since there is no path that could start with
string from both 'restricted' and 'restricted_rw' currently, I don't
see a problem there.

>     if (path == NULL) {
>
>
>Unsurprisingly, this looks good to me.
>
>ACK
>

Consider my idea of moving the line (and maybe some others from
'override') to 'restricted_rw' after applying the following patch.

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 4ce1e7a20590..2eae3eebdecf 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -595,18 +595,19 @@ valid_path(const char *path, const bool readonly)
         vah_warning(_("path does not exist, skipping file type checks"));

     opaths = sizeof(override)/sizeof(*(override));
-
-    npaths = sizeof(restricted)/sizeof(*(restricted));
-    if (array_starts_with(path, restricted, npaths) == 0 &&
-        array_starts_with(path, override, opaths) != 0)
-            return 1;
+    if (array_starts_with(path, override, opaths) == 0)
+        return 0;

     npaths = sizeof(restricted_rw)/sizeof(*(restricted_rw));
-    if (!readonly) {
+    if (readonly) {
         if (array_starts_with(path, restricted_rw, npaths) == 0)
-            return 1;
+            return 0;
     }

+    npaths = sizeof(restricted)/sizeof(*(restricted));
+    if (array_starts_with(path, restricted, npaths) != 0)
+        return 1;
+
     return 0;
 }

--

Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20150813/3e457bc7/attachment.pgp>