On 08/23/2016 08:02 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.
> ---
>
> Thanks for the advice Martin - I completely overlooked the free.
>
> src/security/virt-aa-helper.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index 49e12b9..b385d8c 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 = strdup(perms);
VIR_STRDUP
Then what happens if perms_new == NULL? (eg, failure to strdup).
Typically this is done via:
if (VIR_STRDUP(perms_new, perms) < 0))
return -1
The other problem with what you've modified is that there's a number of
places after allocation where return 0 or return rc are done, but the
memory isn't free'd...
>
> if (path == NULL)
> return rc;
> @@ -764,12 +765,12 @@ vah_add_path(virBufferPtr buf, const char *path, const char *perms, bool recursi
> return rc;
> }
>
Suggestion to add your VIR_STRDUP here since after here failure does the
goto cleanup.
John
> - if (strchr(perms, 'w') != NULL) {
> + 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 +788,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);
> if (explicit_deny_rule) {
> virBufferAddLit(buf, " # don't audit writes to readonly files\n");
> virBufferAsprintf(buf, " deny \"%s%s\" w,\n", tmp, recursive ? "/**" : "");
> @@ -799,6 +800,7 @@ vah_add_path(virBufferPtr buf, const char *path, const char *perms, bool recursi
>
> cleanup:
> VIR_FREE(tmp);
> + VIR_FREE(perms_new);
>
> return rc;
> }
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list