[apparmor] [PATCH 1/3] apparmor: Add support for attaching profiles via xattr, presence and value

[John Johansen]

On 02/08/2018 02:07 PM, Seth Arnold wrote:
> Hello,
> 
> On Thu, Feb 08, 2018 at 12:37:19PM -0800, John Johansen wrote:
>> +static bool unpack_xattrs(struct aa_ext *e, struct aa_profile *profile)
>> +{
>> +	void *pos = e->pos;
>> +
>> +	if (unpack_nameX(e, AA_STRUCT, "xattrs")) {
>> +		int i, size;
>> +
>> +		size = unpack_array(e, NULL);
>> +		profile->xattr_count = size;
>> +		profile->xattrs = kmalloc_array(size, sizeof(char *),
>> +						GFP_KERNEL);
>> +		if (!profile->xattrs)
>> +			goto fail;
>> +		for (i = 0; i < size; i++) {
>> +			if (!unpack_strdup(e, &profile->xattrs[i], NULL))
>> +				goto fail;
> 
> If this step fails before completion, the xattrs array may have some
> entries that weren't properly initialized; I suspect the free operation
> will cause serious trouble in this case.
> 
yep we can switch the kmalloc_array for a kcalloc, and that will fix it

thanks

>> +		}
>> +		if (!unpack_nameX(e, AA_ARRAYEND, NULL))
>> +			goto fail;
>> +		if (!unpack_nameX(e, AA_STRUCTEND, NULL))
>> +			goto fail;
>> +	}
>> +
>> +	if (unpack_nameX(e, AA_STRUCT, "xattr_values")) {
>> +		int i, size;
>> +
>> +		size = unpack_array(e, NULL);
>> +
>> +		/* Must be the same number of xattr values as xattrs */
>> +		if (size != profile->xattr_count)
>> +			goto fail;
>> +
>> +		profile->xattr_lens = kmalloc_array(size, sizeof(size_t),
>> +						    GFP_KERNEL);
>> +		if (!profile->xattr_lens)
>> +			goto fail;
>> +
>> +		profile->xattr_values = kmalloc_array(size, sizeof(char *),
>> +						      GFP_KERNEL);
> 
> Same thing here with the xattr_lens and xattr_values arrays.
> 
>> +		if (!profile->xattr_values)
>> +			goto fail;
>> +
>> +		for (i = 0; i < size; i++) {
>> +			profile->xattr_lens[i] = unpack_blob(e,
>> +					      &profile->xattr_values[i], NULL);
>> +			profile->xattr_values[i] =
>> +				kvmemdup(profile->xattr_values[i],
>> +					 profile->xattr_lens[i]);
>> +		}
>> +
>> +		if (!unpack_nameX(e, AA_ARRAYEND, NULL))
>> +			goto fail;
>> +		if (!unpack_nameX(e, AA_STRUCTEND, NULL))
>> +			goto fail;
>> +	}
>> +	return 1;
>> +
>> +fail:
>> +	e->pos = pos;
>> +	return 0;
>> +}
> 
> Thanks
> 
> 
>