On 01/28/2014 12:12 PM, Felix Geyer wrote: > On 28.01.2014 15:04, Jamie Strandboge wrote: >> On 01/26/2014 03:47 PM, Felix Geyer wrote: >>> Make virt-aa-helper create rules to allow VMs access to filesystem >>> mounts from the host. >> >> Note that virt-aa-helper access to various parts of the filesystem is generally >> ok. However, can you be more specific about the problem you're trying to solve? >> Eg, is there a bug number? > > virt-aa-helper doesn't create the appropriate rules to allow qemu access to > shared filesystem mounts: > http://libvirt.org/formatdomain.html#elementsFilesystems > This made it necessary to manually modify the libivrt-<UUID> profile. > > There is a report about this on the Ubuntu bugtracker: > https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/943680 > Ok, thanks for the extra information. > >> >>> --- >>> src/security/virt-aa-helper.c | 26 ++++++++++++++++++++------ >>> 1 file changed, 20 insertions(+), 6 deletions(-) >>> >>> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c >>> index b9282b4..e1f7848 100644 >>> --- a/src/security/virt-aa-helper.c >>> +++ b/src/security/virt-aa-helper.c >>> @@ -578,9 +578,6 @@ valid_path(const char *path, const bool readonly) >>> return -1; >>> >>> switch (sb.st_mode & S_IFMT) { >>> - case S_IFDIR: >>> - return 1; >>> - break; >>> case S_IFSOCK: >>> return 1; >>> break; >>> @@ -747,7 +744,7 @@ get_definition(vahControl * ctl, const char *xmlStr) >>> } >>> >>> static int >>> -vah_add_file(virBufferPtr buf, const char *path, const char *perms) >>> +vah_add_path(virBufferPtr buf, const char *path, const char *perms, bool recursive) >>> { >>> char *tmp = NULL; >>> int rc = -1; >>> @@ -788,10 +785,14 @@ vah_add_file(virBufferPtr buf, const char *path, const char *perms) >>> goto cleanup; >>> } >>> >>> - virBufferAsprintf(buf, " \"%s\" %s,\n", tmp, perms); >>> + virBufferAsprintf(buf, " \"%s%s\" %s,\n", tmp, recursive ? "/**" : "", perms); >>> if (readonly) { >>> virBufferAddLit(buf, " # don't audit writes to readonly files\n"); >>> - virBufferAsprintf(buf, " deny \"%s\" w,\n", tmp); >>> + virBufferAsprintf(buf, " deny \"%s%s\" w,\n", tmp, recursive ? "/**" : ""); >>> + } >>> + if (recursive) { >>> + // allow reading (but not creating) the dir >>> + virBufferAsprintf(buf, " \"%s/\" r,\n", tmp); >>> } >>> >>> cleanup: >>> @@ -801,6 +802,12 @@ vah_add_file(virBufferPtr buf, const char *path, const char *perms) >>> } >>> >>> static int >>> +vah_add_file(virBufferPtr buf, const char *path, const char *perms) >>> +{ >>> + return vah_add_path(buf, path, perms, false); >>> +} >>> + >>> +static int >>> vah_add_file_chardev(virBufferPtr buf, >>> const char *path, >>> const char *perms, >>> @@ -1049,6 +1056,13 @@ get_files(vahControl * ctl) >>> } /* switch */ >>> } >>> >>> + for (i = 0; i < ctl->def->nfss; i++) { >>> + virDomainFSDefPtr fs = ctl->def->fss[i]; >>> + >>> + if (vah_add_path(&buf, fs->src, fs->readonly ? "r" : "rw", true) != 0) >>> + goto cleanup; >>> + } >>> + >>> if (ctl->newfile) >>> if (vah_add_file(&buf, ctl->newfile, "rw") != 0) >>> goto cleanup; >>> I've not tested this personally, but the patch looks fine from an AppArmor and virt-aa-helper perspective. The only bit I wasn't sure of was this last hunk of the patch-- I'd like upstream to comment on the correctness of (at least) this portion of the patch. I will say that I noticed the SElinux driver doesn't seem to use ctl->def->nfss, etc at all-- perhaps because it assumes the filesystem to be correctly labelled to begin with? (I didn't look any further than security_selinux.c). Conditional 'Acked-By: Jamie Strandboge <jamie@xxxxxxxxxxxxx>' provided others are ok with this last hunk. -- Jamie Strandboge http://www.ubuntu.com/
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list