[PATCH] sysctl: Check length at deprecated_sysctl_warning.

[PATCH] sysctl: Check length at deprecated_sysctl_warning.

[Tetsuo Handa]

Original patch assumed args->nlen < CTL_MAXNAME, but it can be false.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>


--- linux-2.6.22-rc2.orig/kernel/sysctl.c	2007-11-08 10:38:17.000000000 +0900
+++ linux-2.6.22-rc2/kernel/sysctl.c	2007-11-08 11:24:27.000000000 +0900
@@ -2609,6 +2609,10 @@ static int deprecated_sysctl_warning(str
 	int name[CTL_MAXNAME];
 	int i;
 
+	/* Check args->nlen. */
+	if (args->nlen > CTL_MAXNAME)
+		return -EFAULT;
+
 	/* Read in the sysctl name for better debug message logging */
 	for (i = 0; i < args->nlen; i++)
 		if (get_user(name[i], args->name + i))

Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.

[Andrew Morton]

> On Thu, 08 Nov 2007 11:57:26 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:
> Original patch assumed args->nlen < CTL_MAXNAME, but it can be false.
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> 
> 
> --- linux-2.6.22-rc2.orig/kernel/sysctl.c	2007-11-08 10:38:17.000000000 +0900
> +++ linux-2.6.22-rc2/kernel/sysctl.c	2007-11-08 11:24:27.000000000 +0900
> @@ -2609,6 +2609,10 @@ static int deprecated_sysctl_warning(str
>  	int name[CTL_MAXNAME];
>  	int i;
>  
> +	/* Check args->nlen. */
> +	if (args->nlen > CTL_MAXNAME)
> +		return -EFAULT;
> +
>  	/* Read in the sysctl name for better debug message logging */
>  	for (i = 0; i < args->nlen; i++)
>  		if (get_user(name[i], args->name + i))

Well that would have been a nice roothole for someone.  Thanks.

Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.

[Tetsuo Handa]

Hello.

Thanks for reformatting my patch
and sorry for surprising you with directory name
(I meant to type linux-2.6.24-rc2, not linux-2.6.22-rc2).

According to linux-2.6.23,
it seems that I should return -ENOTDIR
for invalid args->nlen value.

I got a question here regarding interpretation of CTL_MAXNAME .
Is args->nlen == CTL_MAXNAME valid?
It is treated as invalid while the definition says

  /* how many path components do we allow in a
     call to sysctl?   In other words, what is
     the largest acceptable value for the nlen
     member of a struct __sysctl_args to have? */

If "name[CTL_MAXNAME];" is what the author intended,
I think args->nlen == CTL_MAXNAME is valid.

Regards.

Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.

[Eric W. Biederman]

Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> writes:

> Hello.
>
> Thanks for reformatting my patch
> and sorry for surprising you with directory name
> (I meant to type linux-2.6.24-rc2, not linux-2.6.22-rc2).
>
> According to linux-2.6.23,
> it seems that I should return -ENOTDIR
> for invalid args->nlen value.
>
> I got a question here regarding interpretation of CTL_MAXNAME .
> Is args->nlen == CTL_MAXNAME valid?
> It is treated as invalid while the definition says
>
>   /* how many path components do we allow in a
>      call to sysctl?   In other words, what is
>      the largest acceptable value for the nlen
>      member of a struct __sysctl_args to have? */
>
> If "name[CTL_MAXNAME];" is what the author intended,
> I think args->nlen == CTL_MAXNAME is valid.

name[CTL_MAXNAME} is not valid.
name[0...CTL_MAXNAME-1] is valid.


The check that got lost in the refactoring was specfically:

-       if (tmp.nlen <= 0 || tmp.nlen >= CTL_MAXNAME)
-               return -ENOTDIR;

Eric

Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.

[Tetsuo Handa]

Hello.

Eric W. Biederman wrote:
> name[CTL_MAXNAME} is not valid.
> name[0...CTL_MAXNAME-1] is valid.
Yes.

> The check that got lost in the refactoring was specfically:
> 
> -       if (tmp.nlen <= 0 || tmp.nlen >= CTL_MAXNAME)
> -               return -ENOTDIR;
Thus I think tmp.nlen == CTL_MAXNAME should be allowed
because tmp.nlen is used as "for (i = 0; i < tmp.nlen; i++)".
I think
	if (tmp.nlen <= 0 || tmp.nlen > CTL_MAXNAME)
		return -ENOTDIR;
is correct.

Thanks.

[PATCH] sysctl: Check length at deprecated_sysctl_warning.

[Tetsuo Handa]

Andrew, please replace previous patch with this one.
This one returns -ENOTDIR.
----------

Original patch forgot to check args->nlen.
I don't know why args->nlen == CTL_MAXNAME is rejected,
but it has been rejected traditionally.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 kernel/sysctl.c |    4 ++++
 1 file changed, 4 insertions(+)

diff -puN kernel/sysctl.c~sysctl-check-length-at-deprecated_sysctl_warning kernel/sysctl.c
--- a/kernel/sysctl.c~sysctl-check-length-at-deprecated_sysctl_warning
+++ a/kernel/sysctl.c
@@ -2609,6 +2609,10 @@ static int deprecated_sysctl_warning(str
 	int name[CTL_MAXNAME];
 	int i;
 
+	/* Check args->nlen. */
+	if (args->nlen <= 0 || args->nlen >= CTL_MAXNAME)
+		return -ENOTDIR;
+
 	/* Read in the sysctl name for better debug message logging */
 	for (i = 0; i < args->nlen; i++)
 		if (get_user(name[i], args->name + i))

Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.

[Andrew Morton]

On Tue, 13 Nov 2007 12:07:23 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:

> Andrew, please replace previous patch with this one.
> This one returns -ENOTDIR.
> ----------
> 
> Original patch forgot to check args->nlen.
> I don't know why args->nlen == CTL_MAXNAME is rejected,
> but it has been rejected traditionally.
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  kernel/sysctl.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff -puN kernel/sysctl.c~sysctl-check-length-at-deprecated_sysctl_warning kernel/sysctl.c
> --- a/kernel/sysctl.c~sysctl-check-length-at-deprecated_sysctl_warning
> +++ a/kernel/sysctl.c
> @@ -2609,6 +2609,10 @@ static int deprecated_sysctl_warning(str
>  	int name[CTL_MAXNAME];
>  	int i;
>  
> +	/* Check args->nlen. */
> +	if (args->nlen <= 0 || args->nlen >= CTL_MAXNAME)

I believe (args->nlen > CTL_MAXNAME) was correct.

> +		return -ENOTDIR;

Ho hum.  Why does that code return -ENOTDIR on error anyway?

> +
>  	/* Read in the sysctl name for better debug message logging */
>  	for (i = 0; i < args->nlen; i++)
>  		if (get_user(name[i], args->name + i))

Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.

[Tetsuo Handa]

Hello.

Andrew Morton wrote:
> I believe (args->nlen > CTL_MAXNAME) was correct.
I'll leave it to you.
But if you want to allow args->nlen == CTL_MAXNAME,
you also need to update do_sysctl().

int do_sysctl(int __user *name, int nlen, void __user *oldval, size_t __user *oldlenp,
               void __user *newval, size_t newlen)
{
        ...
        if (nlen <= 0 || nlen >= CTL_MAXNAME)
                return -ENOTDIR;
        ...
}

Thanks.

Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.

[Eric W. Biederman]

Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> writes:

> Hello.
>
> Andrew Morton wrote:
>> I believe (args->nlen > CTL_MAXNAME) was correct.
> I'll leave it to you.
> But if you want to allow args->nlen == CTL_MAXNAME,
> you also need to update do_sysctl().

Which has been that way since before I decided to touch it. 2.6.12-rc1.
I haven't tracked it before then as I don't expect to see anything.

And that is why -ENOTDIR.  That is the historical error code from sysctl in
this case.

> int do_sysctl(int __user *name, int nlen, void __user *oldval, size_t __user
> *oldlenp,
>                void __user *newval, size_t newlen)
> {
>         ...
>         if (nlen <= 0 || nlen >= CTL_MAXNAME)
>                 return -ENOTDIR;
>         ...
> }

CTL_MAXNAME is fairly arbitrary, and since the set of binary paths is fixed.
So it feels to me like the code really should read:

        if (nlen <= 0 || nlen > CTL_MAXNAME)
                return -ENOTDIR;

In both places. Just because that is what the comment describes.

I think in reality CTL_MAXNAME is actually 5 but I would have to look a little
more closely to confirm that.

Eric