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 <[email protected]>' provided others
are ok with this last hunk.
--
Jamie Strandboge http://www.ubuntu.com/
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
