[Sorry, the Ubuntu package suggests this came from Cèdric, although I can't quite find this patch on the mailing list. Those patches which I did see from Cèdric did not have a Signed-off-by, so I didn't add one for him.] From: Cèdric Bosdonnat <cbosdonnat(a)suse.com&gt; Upstream changed get_files to unconditionally be "rw" to allow audit files to be written. Still under discussion but the proposed approach is to have a way of saying readonly but do not implicitely create a write deny rule. --- Changelog: do not overwrite const memory, that leads to segv (serge) --- src/security/virt-aa-helper.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index b466626..34d08c8 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -785,12 +785,19 @@ get_definition(vahControl * ctl, const char *xmlStr) return rc; } +/* + * The permissions allowed are apparmor valid permissions and 'R'. 'R' stands + * for read with no explicit deny rule. + */ static int -vah_add_path(virBufferPtr buf, const char *path, const char *perms, bool recursive) +vah_add_path(virBufferPtr buf, const char *path, const char *inperms, bool recursive) { char *tmp = NULL; int rc = -1; bool readonly = true; + bool explicit_deny_rule = true; + char *perms = strdupa(inperms); + char *sub = NULL; if (path == NULL) return rc; @@ -815,8 +822,16 @@ vah_add_path(virBufferPtr buf, const char *path, const char *perms, bool recursi return rc; } - if (strchr(perms, 'w') != NULL) + if (strchr(perms, 'w') != NULL) { readonly = false; + explicit_deny_rule = false; + } + + if ((sub = strchr(perms, 'R')) != NULL) { + /* Don't write the invalid 'R' permission, replace with 'r' */ + sub[0] = 'r'; + explicit_deny_rule = false; + } rc = valid_path(tmp, readonly); if (rc != 0) { @@ -831,7 +846,7 @@ vah_add_path(virBufferPtr buf, const char *path, const char *perms, bool recursi tmp[strlen(tmp) - 1] = '\0'; virBufferAsprintf(buf, " \"%s%s\" %s,\n", tmp, recursive ? "/**" : "", perms); - if (readonly) { + if (explicit_deny_rule) { virBufferAddLit(buf, " # don't audit writes to readonly files\n"); virBufferAsprintf(buf, " deny \"%s%s\" w,\n", tmp, recursive ? "/**" : ""); } @@ -1130,7 +1145,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, "rw", true) != 0) + if (vah_add_path(&buf, fs->src, fs->readonly ? "R" : "rw", true) != 0) goto cleanup; } } -- 2.7.0

On Fri, Mar 11, 2016 at 08:07:02PM +0000, Serge Hallyn wrote: This would be a newcomer, the rest of the code uses VIR_STRDUP. I'm not sure how hard this is being "enforced" at the moment. This otherwise looks good to me (as does the whole series) but Jamie probably should comment on it. Cheers, -- Guido

[ This depends on patch 2/3, so don't cherrypick just this one :) ] Just because a disk element only requests read access doesn't mean there may not be another readwrite request. This fixes 'virsh blockcommit' which otherwise fails due to inability to write to the basefile. Signed-off-by: Serge Hallyn <serge.hallyn(a)ubuntu.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 34d08c8..2d05522 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -939,11 +939,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.0