[patch] Avoid using hardcoded values in kernel/sys.c

[patch] Avoid using hardcoded values in kernel/sys.c

[David Rientjes]

The release field of struct new_utsname may always change, so avoid
hardcoded values when setting up a buffer to copy to it.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 kernel/sys.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/kernel/sys.c b/kernel/sys.c
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1169,10 +1169,11 @@ DECLARE_RWSEM(uts_sem);
  * Work around broken programs that cannot handle "Linux 3.0".
  * Instead we map 3.x to 2.6.40+x, so e.g. 3.0 would be 2.6.40
  */
-static int override_release(char __user *release, int len)
+static int override_release(char __user *release)
 {
+	const int len = sizeof(release);
 	int ret = 0;
-	char buf[65];
+	char buf[len];
 
 	if (current->personality & UNAME26) {
 		char *rest = UTS_RELEASE;
@@ -1202,7 +1203,7 @@ SYSCALL_DEFINE1(newuname, struct new_utsname __user *, name)
 		errno = -EFAULT;
 	up_read(&uts_sem);
 
-	if (!errno && override_release(name->release, sizeof(name->release)))
+	if (!errno && override_release(name->release))
 		errno = -EFAULT;
 	if (!errno && override_architecture(name))
 		errno = -EFAULT;
@@ -1225,7 +1226,7 @@ SYSCALL_DEFINE1(uname, struct old_utsname __user *, name)
 		error = -EFAULT;
 	up_read(&uts_sem);
 
-	if (!error && override_release(name->release, sizeof(name->release)))
+	if (!error && override_release(name->release))
 		error = -EFAULT;
 	if (!error && override_architecture(name))
 		error = -EFAULT;
@@ -1261,7 +1262,7 @@ SYSCALL_DEFINE1(olduname, struct oldold_utsname __user *, name)
 
 	if (!error && override_architecture(name))
 		error = -EFAULT;
-	if (!error && override_release(name->release, sizeof(name->release)))
+	if (!error && override_release(name->release))
 		error = -EFAULT;
 	return error ? -EFAULT : 0;
 }

Re: [patch] Avoid using hardcoded values in kernel/sys.c

[Linus Torvalds]

On Mon, Oct 17, 2011 at 5:55 PM, David Rientjes <rientjes@google.com> wrote:
> The release field of struct new_utsname may always change, so avoid
> hardcoded values when setting up a buffer to copy to it.

This patch is totally bogus.

> +static int override_release(char __user *release)
>  {
> +       const int len = sizeof(release);

You just wrote "len = 4" (or 8, for 64-bit architectures) in a very unclear way.

IOW, "len" is now the size of the *pointer*. Which is wrong.

*And* you then re-introduced the variable-length array, which not only
triggered the gcc bug, but which is a BAD IDEA TO BEGIN WITH.

Variably sized arrays generate worse code, and can have subtle
security issues that aren't obvious (like cause our tools that check
for overly big stack usage to quietly fail).

Don't use variable-sized arrays on stack. They are never a good idea
in kernel space. The fact that we do have other code that does use
them too is not an excuse.

But using them in this kind of horribly broken manner with the wrong
length is just *really* bad.

                           Linus

Re: [patch] Avoid using hardcoded values in kernel/sys.c

[David Rientjes]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 597 bytes --]

On Mon, 17 Oct 2011, Linus Torvalds wrote:

> > The release field of struct new_utsname may always change, so avoid
> > hardcoded values when setting up a buffer to copy to it.
> 
> This patch is totally bogus.
> 
> > +static int override_release(char __user *release)
> >  {
> > +       const int len = sizeof(release);
> 

Right, I meant to pass the struct in and use sizeof(release->name), but I 
was under the impression that wouldn't count as a variable length array or 
be compiled as such by ISO99 standards because it's a constant expression 
(as the standard says is not variable length).

Re: [patch] Avoid using hardcoded values in kernel/sys.c

[Linus Torvalds]

On Mon, Oct 17, 2011 at 6:33 PM, David Rientjes <rientjes@google.com> wrote:
>
> Right, I meant to pass the struct in and use sizeof(release->name), but I
> was under the impression that wouldn't count as a variable length array or
> be compiled as such by ISO99 standards because it's a constant expression
> (as the standard says is not variable length).

Yes, if it really were a constant expression of a valid length, that
would be fine. Passing in the struct itself would work, and then you
could do a sizeof of the array, instead of of the pointer to the
character array..

Except in this case it won't work. Because sometimes the struct is the
old one, sometimes it's the new one. So there is no well-defined
struct pointer to pass in. You can only pass in pointer-to-char and
the (variable) size.

Which is why it now just hardcodes it to 65, which is the largest
size. Not exactly "pretty", no. But no worse than using the variable
length array.

Yeah, there are tricks we could play. For example, we could turn that
override_release() into a macro that allocates the array in the
*caller*, where the length is known at compile time, and then pass in
that temporary pointer too. I considered it, but it seemed to be too
much work for too trivial a case. It's not like there are all that
many definitions of utsname, and they are all in <linux/utsname.h>.
It's unlikely that we'd ever add any bigger cases, but if we do, we'd
need to update the constant.

                        Linus

Re: [patch] Avoid using hardcoded values in kernel/sys.c

[Joe Perches]

On Mon, 2011-10-17 at 17:55 -0700, David Rientjes wrote:
> The release field of struct new_utsname may always change, so avoid
> hardcoded values when setting up a buffer to copy to it.

I don't think this is correct.

> diff --git a/kernel/sys.c b/kernel/sys.c
[]
> -static int override_release(char __user *release, int len)
> +static int override_release(char __user *release)
>  {
> +	const int len = sizeof(release);
>  	int ret = 0;
> -	char buf[65];
> +	char buf[len];

sizeof(release) = 4 or 8