Re: [BUG] taskstats: REGISTER_CPUMASK silently truncates the last byte of the cpumask string

[Bradley Morgan]

Hi Andrew,

I dont think this actually fixes it... :(

[...]

> -	data = kmalloc(len, GFP_KERNEL);
> +	data = kmalloc(len + 1, GFP_KERNEL);
>  	if (!data)
>  		return -ENOMEM;
>  	nla_strscpy(data, na, len);

nla_strscpy() copies at most dstsize - 1 bytes, and dstsize is still
len here. When the attr payload comes in without a trailing NUL,
srclen == len >= dstsize and the last byte still gets cut off. The
bigger buffer just adds a byte nothing ever writes to (the zero pad
in nla_strscpy() only goes up to dstsize).

Olegs report even spells this out in its suggested fix direction:
allocate len + 1 *and* pass len + 1 as dstsize. The patch only
picked up the first half. The fix is something like:

	data = kmalloc(len + 1, GFP_KERNEL);
	if (!data)
		return -ENOMEM;
	nla_strscpy(data, na, len + 1);

or skip the dance entirely, nla_strdup() already does exactly this:

	data = nla_strdup(na, GFP_KERNEL);
	if (!data)
		return -ENOMEM;

Btw the bug only bites when the sender doesnt NUL terminate the
attr payload; senders that include the NUL (srclen gets decremented
for the trailing NUL, so srclen < dstsize) were always fine. Thats
probably why this survived 20 years. Might be worth a line in
the changelog. And as the report already notes, the policy is
NLA_STRING, not NLA_NUL_STRING, so a payload without the trailing
NUL is legit input here.
Thanks!