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

[Chris Metcalf <cmetcalf@ezchip.com>]

On 10/05/2015 12:36 PM, Ingo Molnar wrote:
> So the patch below got tested a bit more seriously, with the strscpy() based
> strlcpy() patch I sent earlier: at least a typical Fedora bootup with a few
> thousand strlcpy() uses does not crash in any obvious way.
>
> Still needs review to make sure I have not missed anything ...
>
> Thanks,
>
> 	Ingo
>
> ===================>
>  From 946bab4d7138e5db53c5f1759e97809ebdf89551 Mon Sep 17 00:00:00 2001
> From: Ingo Molnar<mingo@kernel.org>
> Date: Mon, 5 Oct 2015 18:30:37 +0200
> Subject: [PATCH] string: Fix strscpy() uninitialized data copy bug
>
> Alexey Dobriyan noticed that our new strscpy() implementation will copy
> potentially out of range or uninitialized data from post the end of the
> source string.
>
> Fix this by zeroing out the tail of the final word of the copy.
>
> Reported-by: Alexey Dobriyan<adobriyan@gmail.com>
> Signed-off-by: Ingo Molnar<mingo@kernel.org>
> ---
>   lib/string.c | 14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/lib/string.c b/lib/string.c
> index 6b89c035df74..548f52b7a145 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -177,12 +177,24 @@ 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)) {
> +			unsigned int zero_pos;
> +
>   			data = prep_zero_mask(c, data, &constants);
>   			data = create_zero_mask(data);
> +
> +			zero_pos = find_zero(data);
> +
> +			/* Clear out undefined data within the final word after the NUL (if any): */
> +			memset((void *)&c + zero_pos, 0, sizeof(long)-zero_pos);

Unfortunately using memset() like that will break on big-endian
machines.  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.

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);

-- 
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com