Re: [libvirt] [PATCH] Avoid segfault in virt-aa-helper when handling read-only mount filesystems
On 08/24/2016 07:15 PM, Rufo Dogav wrote:
This patch fixes a segfault in virt-aa-helper caused by attempting to
modify a
string literal in situ. It is triggered when a domain has a <filesystem> with
type='mount' configured readonly, and libvirt is using the AppArmor security
driver for sVirt confinement.
---
Existing code seems to use VIR_STRDUP_QUIET for similar purposes, please
change if VIR_STRDUP is preferred. Needed new cleanup rather than simply
return -1; to avoid introducing leak of tmp.
src/security/virt-aa-helper.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
I adjusted the commit message slightly and made the adjustments listed
below and pushed... Congrats on your first libvirt patch.
Tks
John
diff --git a/src/security/virt-aa-helper.c
b/src/security/virt-aa-helper.c
index 49e12b9..0bcf642 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -740,6 +740,7 @@ vah_add_path(virBufferPtr buf, const char *path, const char *perms,
bool recursi
bool readonly = true;
bool explicit_deny_rule = true;
char *sub = NULL;
+ char *perms_new = NULL;
if (path == NULL)
return rc;
@@ -764,12 +765,15 @@ vah_add_path(virBufferPtr buf, const char *path, const char *perms,
bool recursi
return rc;
}
- if (strchr(perms, 'w') != NULL) {
+ if (VIR_STRDUP_QUIET(perms_new, perms) < 0)
I kept the _QUIET for this one... Printing of errors in this module is a
bit different than libvirt usually uses. The _QUIET version won't
virReportError the failed allocation.
In this case, if the allocation failed "eventually" the stack seems to
unwind to get_files() which returns failure to vahParseArgv which will
print a very generic vah_error().
-
+ goto clean_tmp;
No need to create a new label... A VIR_FREE() of something that's been
initialized to NULL doesn't do anything. A single "cleanup" label is
something that's more consistent with other libvirt code.
+
+ if (strchr(perms_new, 'w') != NULL) {
readonly = false;
explicit_deny_rule = false;
}
- if ((sub = strchr(perms, 'R')) != NULL) {
+ if ((sub = strchr(perms_new, 'R')) != NULL) {
/* Don't write the invalid R permission, replace it with 'r' */
sub[0] = 'r';
explicit_deny_rule = false;
@@ -787,7 +791,7 @@ vah_add_path(virBufferPtr buf, const char *path, const char *perms,
bool recursi
if (tmp[strlen(tmp) - 1] == '/')
tmp[strlen(tmp) - 1] = '\0';
- virBufferAsprintf(buf, " \"%s%s\" %s,\n", tmp, recursive ?
"/**" : "", perms);
+ virBufferAsprintf(buf, " \"%s%s\" %s,\n", tmp, recursive ?
"/**" : "", perms_new);
This is a long line - I moved perms_new to it's own line
if (explicit_deny_rule) {
virBufferAddLit(buf, " # don't audit writes to readonly
files\n");
virBufferAsprintf(buf, " deny \"%s%s\" w,\n", tmp,
recursive ? "/**" : "");
@@ -798,6 +802,8 @@ vah_add_path(virBufferPtr buf, const char *path, const char *perms,
bool recursi
}
cleanup:
+ VIR_FREE(perms_new);
+ clean_tmp:
VIR_FREE(tmp);
return rc;