On Fri, 20 Oct 2006 16:43:39 +0400
Michael Tokarev <[EMAIL PROTECTED]> wrote:
> I were debugging a weird problem with busybox, and come across
> this chunk of strace output:
>
> open("/proc/sys/kernel/osrelease", O_RDONLY) = 3
> read(3, "2", 1) = 1
> read(3, "", 1) = 0
> close(3) = 0
>
> As you can see, after reading one byte from /proc/sys/kernel/osrelease,
> next read() returns 0, which is treated as end-of-file by an application.
>
> Why busybox does this single-byte reads is another question (many
> shells does that, in order to be able to stop reading at newline).
>
> But this is definitely a bug in kernel, and should be fixed....
>
> It exists in 2.6.17 and 2.6.18
>
Well this nearly killed me. kernel-side proc handlers are ghastly things.
Could I have this reviewed please? It surely has a hole in it somewhere.
From: Andrew Morton <[EMAIL PROTECTED]>
If you try to read things like /proc/sys/kernel/osrelease with single-byte
reads, you get just one byte and then EOF. This is because _proc_do_string()
assumes that the caller is read()ing into a buffer which is large enough to
fit the whole string in a single hit.
Fix.
Cc: "Eric W. Biederman" <[EMAIL PROTECTED]>
Cc: Oleg Nesterov <[EMAIL PROTECTED]>
Signed-off-by: Andrew Morton <[EMAIL PROTECTED]>
---
kernel/sysctl.c | 37 ++++++++++++++++++++++++++++---------
1 file changed, 28 insertions(+), 9 deletions(-)
diff -puN kernel/sysctl.c~_proc_do_string-fix-short-reads kernel/sysctl.c
--- a/kernel/sysctl.c~_proc_do_string-fix-short-reads
+++ a/kernel/sysctl.c
@@ -1682,8 +1682,7 @@ static int _proc_do_string(void* data, i
char __user *p;
char c;
- if (!data || !maxlen || !*lenp ||
- (*ppos && !write)) {
+ if (!data || !maxlen || !*lenp) {
*lenp = 0;
return 0;
}
@@ -1705,18 +1704,38 @@ static int _proc_do_string(void* data, i
((char *) data)[len] = 0;
*ppos += *lenp;
} else {
- len = strlen(data);
+ loff_t pos = *ppos;
+ const size_t slen = strlen(data);
+
+ /*
+ * len is the amount of data to copy, and becomes the amount of
+ * data which was copied
+ */
+ len = slen;
+ if (pos > len) {
+ *lenp = 0;
+ return 0;
+ }
if (len > maxlen)
len = maxlen;
if (len > *lenp)
len = *lenp;
- if (len)
- if(copy_to_user(buffer, data, len))
- return -EFAULT;
- if (len < *lenp) {
- if(put_user('\n', ((char __user *) buffer) + len))
+ /* Don't copy past the end of the string */
+ if (len > slen - pos)
+ len = slen - pos;
+ data += pos;
+ /* Copy as much of the string as we can */
+ if (len) {
+ if (copy_to_user(buffer, data, len))
return -EFAULT;
- len++;
+ }
+ /* If we copied the whole string, now write a \n */
+ if (len + pos == slen) {
+ if (len + pos < maxlen) {
+ if (put_user('\n', (char __user *)buffer + len))
+ return -EFAULT;
+ len++;
+ }
}
*lenp = len;
*ppos += len;
_
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
