Re: [PATCH] string: Fix strscpy() uninitialized data copy bug

[Ingo Molnar <mingo@kernel.org>]

* Chris Metcalf <cmetcalf@ezchip.com> wrote:

> Unfortunately using memset() like that will break on big-endian machines.

doh ... and I somehow convinced myself that it was endian safe ;-)

> [...]  I always have to go back and play around with the word-at-a-time.h 
> definitions to get this right, but I think it's possible that the "data" itself 
> has the mask to clear the unwanted bytes, i.e. you could do something like the 
> following (untested).
> 
> I'm still not totally convinced it's necessary, as programmers should generally 
> assume anything beyond the end of a copied string is garbage anyway, and since 
> we're not copying it to userspace we're not exposing any possibly secure data.
> 
> Races shouldn't be a concern either since, after all, there is already a window 
> where we may have overwritten the NUL end of an earlier shorter string, and now 
> a racy copy from the partially-written dest buf could walk right off the end of 
> the buffer itself, so you'd already better not be doing that.
> 
> But, all that said, I'm not opposed to a simple fix to avoid carrying along the 
> uninitialized bytes from beyond the end of the source string, since it does seem 
> a bit cleaner, even if I can't put my finger in a reason why it would actually 
> matter.

So it would matter for more advanced sharing ABIs: for example if there's an 
mlock()-ed area registered on the kernel side as well as kernel accessible memory, 
and if we do an strscpy() to such a target area, we don't want to leak 
uninitialized data to user-space.

(This is not theoretical, the perf ring-buffer is such a construct for example.)

So IMHO this is a quality of implementation issue that we should fix.

> diff --git a/lib/string.c b/lib/string.c
> index 8dbb7b1eab50..ba64f4e0382d 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -203,12 +203,13 @@ ssize_t strscpy(char *dest, const char *src, size_t count)
>  		unsigned long c, data;
>  		c = *(unsigned long *)(src+res);
> -		*(unsigned long *)(dest+res) = c;
>  		if (has_zero(c, &data, &constants)) {
>  			data = prep_zero_mask(c, data, &constants);
>  			data = create_zero_mask(data);
> +			*(unsigned long *)(dest+res) = c & data;
>  			return res + find_zero(data);
>  		}
> +		*(unsigned long *)(dest+res) = c;
>  		res += sizeof(unsigned long);
>  		count -= sizeof(unsigned long);
>  		max -= sizeof(unsigned long);

Looks good to me!

Thanks,

	Ingo