[apparmor] Apparmor rules for dconf confinement

[William Hua]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Thanks John, just going to reply to your initial remarks for now.



>> +	/* We are going to leave space for two numbers. The first is
>> the total +	 * number of bytes we are writing after the first
>> number. This is so +	 * users can read the full output without
>> reallocation. +	 * +	 * The second number is the number of data
>> blocks we're writing. An +	 * application might be confined by
>> multiple policies having data in +	 * the same key. */ +
>> memset(buf, 0, 2 * sizeof(ssize_t)); +	out = buf + 2 *
>> sizeof(ssize_t);
> use of size_t for an interface between kernel and userspace is
> problematic. We run into some weird combinations, 32bit userspace
> on 64bit kernel (common). And potentially other weird combos with
> endian switching. I am not opposed to using binary data, but define
> it exactly
> 
> u32 or u64 and whether it is little or big endian, that way we
> avoid any chance of an issue between user and kernel space
> communication.
> 
>> + +	label_for_each_confined(i, label, profile) { +		if
>> (!profile->data) +			continue; + +		hash =
>> rhashtable_hashfn(profile->data, &key, sizeof(key)); +		data =
>> rhashtable_lookup_compare(profile->data, hash, keycmp, &key); + +
>> if (data) { +			if (out + sizeof(ssize_t) + data->size > buf +
>> buf_len) { +				aa_put_label(label); +				return -EINVAL; /* not
>> enough space */ +			} +			memcpy(out, &data->size,
>> sizeof(ssize_t)); +			out += sizeof(ssize_t);
> same with the size written here

Ack, I had no idea this was an issue, but TIL.



>> +	if (profile->data) { +		rht = profile->data; +		profile->data =
>> NULL; +		for (i = 0; i < rht->tbl->size; i++) { +
>> rht_for_each_entry_safe(data, next, rht->tbl->buckets[i], rht,
>> head) {
> this ends up leaking the data->key

data->key and data->data are just weak pointers into profile->blob.



>> @@ -668,14 +680,54 @@ static struct aa_profile
>> *unpack_profile(struct aa_ext *e) if (!unpack_trans_table(e,
>> profile)) goto fail;
>> 
>> +	size = unpack_blob(e, &blob, "data");
> while you can get away with this, it forces you to mess with the
> sub "aa_ext f" nor do you actually end up checking that your sub
> size consumed is what the outer blob says it is.
> 
> Is there any reason we would want to allocate a single larger blob,
> instead a blob per entry? That would keep the kmalloc smaller and
> make it more likely to succeed with larger blobs. At a minimum we
> should move the blob allocation to kvmalloc and kvfree which can
> handle larger blobs of data well.
> 
> I've appended a patch (totally untested and not even built) at the
> end that converts to individual allocations, what do you think?

There's no particular reason other than you and Seth recommending
reducing the number of kmallocs for the first iteration. I'm totally
fine with your changes if you think it'll work better in practice.



>> fail: -	if (profile) +	/* don't worry about profile->data,
>> aa_free_profile should free it */ +	if (profile) { +
>> kzfree(profile->blob);
> free_profile also frees profile->blob so either don't worry about
> it here either or assign NULL to it, so we don't get a double free

Agreed, thanks for catching that. I only looked at your patch briefly,
but I think removing the kzfree at this point is better than removing
it in aa_free_profile().
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJViXFkAAoJEGaNijJ4Mbw+cqgH/imQMAhKv+BauVK4hUWxVQgL
MBzdoSYZpYqLco5T7fZSUC1TcSvzqnShm0QW03O9TrItYIdNnFFMSoTXK/C0U+gw
m2NkZlcAinS1HobJhdyWrJHM319Y9zUCy8nMgWMwDasauTE2Am+R90415FeKwMTe
vkaT4BaUxOYv6dspN+6pEeyGQLTU2W8kWp7a5onySpQV57TTQAN5LFWE8GY9TdZf
6NLfiwfsjdOaibigiAIfkAYhIBO26rk+9wvv3Ki5lPV8GMR4WkrxnITYIyVapxMl
zPY4jodlbJmR5JPxO0DRpFE1mDUiymdMc52r8PZO38YHMXKnwN/Rp/IYxaKWDiE=
=cbcd
-----END PGP SIGNATURE-----