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

[Ingo Molnar <mingo@kernel.org>]

* Ingo Molnar <mingo@kernel.org> wrote:

> A slightly more paranoid version would be:
> 
> 		c = *(unsigned long *)(src+res);
>  
> 		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: */
>  			memset((void *)&c + zero_pos, 0, sizeof(long)-zero_pos);
> 
> 			*(unsigned long *)(dest+res) = c;
>  
> 			return res+zero_pos;
> 		}
> 		*(unsigned long *)(dest+res) = c;
> 
> This would solve any theoretical races in the _target_ buffer: if the target 
> buffer may be copied to user-space in a racy fashion and we don't ever want it 
> to have undefined data, then this variant does the tail-zeroing of the final 
> word in the temporary copy, not in the target buffer.
> 
> Still untested.

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);
+
+			*(unsigned long *)(dest+res) = c;
+
 			return res + find_zero(data);
 		}
+		*(unsigned long *)(dest+res) = c;
+
 		res += sizeof(unsigned long);
 		count -= sizeof(unsigned long);
 		max -= sizeof(unsigned long);