Re: [PATCH] Fix user data corrupted by old value return of sysctl

[Yi Yang <yang.y.yi@gmail.com>]

Linus Torvalds wrote:

>On Fri, 30 Dec 2005, Yi Yang wrote:
>  
>
>>If the user reads a sysctl entry which is of string type
>>by sysctl syscall, this call probably corrupts the user data
>>right after the old value buffer, the issue lies in sysctl_string
>>seting 0 to oldval[len], len is the available buffer size
>>specified by the user, obviously, this will write to the first
>>byte of the user memory place immediate after the old value buffer,
>>the correct way is that sysctl_string doesn't set 0, the user
>>should do it by self in the program.
>>    
>>
>
>Hmm.. I think this patch is incomplete.
>
>We _should_ zero-pad the data, at least if the result fits in the buffer.
>
>So I think the correct fix is to just _copy_ the last zero if it fits in 
>the buffer, rather than do the unconditional "add NUL at the end" thing. 
>The simplest way to do that is to just make "l" be "strlen(str)+1", so 
>that we count the ending NUL in the length (and then, if the buffer isn't 
>big enough, we will truncate it).
>
>In other words, I would instead suggest a patch like the appended.
>
>But even that is questionable: one alternative is to always zero-pad (like 
>we used to), but make sure that the buffer size is sufficient for it (ie 
>instead of adding one to the length of the string, we'd subtract one from 
>the buffer length and make sure that the '\0' fits..
>
>Comments?
>  
>
Yes, you are more complete, I agree with it very much.

>		Linus
>---
>diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>index 9990e10..ad0425a 100644
>--- a/kernel/sysctl.c
>+++ b/kernel/sysctl.c
>@@ -2201,14 +2201,12 @@ int sysctl_string(ctl_table *table, int 
> 		if (get_user(len, oldlenp))
> 			return -EFAULT;
> 		if (len) {
>-			l = strlen(table->data);
>+			l = strlen(table->data)+1;
> 			if (len > l) len = l;
> 			if (len >= table->maxlen)
> 				len = table->maxlen;
> 			if(copy_to_user(oldval, table->data, len))
> 				return -EFAULT;
>-			if(put_user(0, ((char __user *) oldval) + len))
>-				return -EFAULT;
> 			if(put_user(len, oldlenp))
> 				return -EFAULT;
> 		}
>
>  
>