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

On Fri, Dec 30, 2005 at 09:25:35AM -0800, 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?

Always do zero-pad please. I'd feel more comfortable with C strings (NULL
terminated). Don't you?

Poor code, also a small cleanup attached.

Signed-off-by: Coywolf Qi Hunt <qiyong@xxxxxxxxx>
---

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index e5102ea..9960a26 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2203,7 +2203,7 @@ int sysctl_string(ctl_table *table, int
if (len) {
l = strlen(table->data)+1;
if (len > l) len = l;
- if (len >= table->maxlen)
+ if (len > table->maxlen)
len = table->maxlen;
if(copy_to_user(oldval, table->data, len))
return -EFAULT;
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages

  • Re: Discovering variable types...
    ... >- but I suppose MS expect us to use wrappers ... memory allocations for your variables from disk as well. ... >They most certainly are of fixed size, changing the size of a String ... >>me to keep buffer size and current postion right in the memory block. ...
    (comp.lang.pascal.delphi.misc)
  • Re: Secure C library
    ... I read much of the new "security TR", and gee, I don't know. ... the buffer from the buffer size. ... It is not hard to design a better form of buffer and string handling. ... but this is just one example of how thoughtful interface design can ...
    (comp.std.c)
  • Re: Secure C library
    ... >> string functions don't make much sense once you add bounds-checking ... >> designing an interface just for the purpose of reducing the frequency ... > make buffer size decisions more visible, ... Bstrlib is also very interoperable with char *'s, ...
    (comp.std.c)
  • Re: why I can not write to the file after initialize the MFC in a service program
    ... you check EVERY return from a call that can fail, ... Why do you need an intermedate buffer to write literal strings anyway? ... For example, if AfxWinInit fails, you copy a 45-character string into a ... So you are going to try to initialize MFC EACH TIME THROUGH THE LOOP? ...
    (microsoft.public.vc.mfc)
  • Re: Calling dll functions from vb.net with pointer returns!
    ... (ByRef pulLen As Integer, ByVal pszFilter As String, ByVal ulFlags As ... OUT PTCHAR Buffer, ... Address of a buffer to receive a set of NULL-terminated device instance ... pszFilter must specify the name of a device ...
    (microsoft.public.dotnet.languages.vb)