Home » Mailing list archives » linux-kernel » 2007 » November » 13

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

Previous thread: [PATCH 0/3] virtio PCI driver by Anthony Liguori on Wednesday, November 7, 2007 - 10:46 pm. (33 messages)

Next thread: Re: Fwd: same problem with 2.6.24-rc2 by Randy Dunlap on Thursday, November 8, 2007 - 3:05 am. (2 messages)
[view original message]
From: Tetsuo Handa <penguin-kernel@...>
To: <akpm@...>
Cc: <ebiederm@...>, <linux-kernel@...>
Subject: [PATCH] sysctl: Check length at deprecated_sysctl_warning.
Date: Wednesday, November 7, 2007 - 10:57 pm

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))

-
[view original message]
From: Andrew Morton <akpm@...>
To: Tetsuo Handa <penguin-kernel@...>
Cc: <ebiederm@...>, <linux-kernel@...>
Subject: Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.
Date: Wednesday, November 7, 2007 - 11:22 pm

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

-
[view original message]
From: Tetsuo Handa <penguin-kernel@...>
To: Andrew Morton <akpm@...>
Cc: <ebiederm@...>, <linux-kernel@...>, Tetsuo Handa <penguin-kernel@...>
Subject: Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.
Date: Thursday, November 8, 2007 - 4:19 am

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.

-
[view original message]
From: Eric W. Biederman <ebiederm@...>
To: Tetsuo Handa <penguin-kernel@...>
Cc: Andrew Morton <akpm@...>, <linux-kernel@...>
Subject: Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.
Date: Monday, November 12, 2007 - 5:44 am

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


-
[view original message]
From: Tetsuo Handa <penguin-kernel@...>
To: <akpm@...>
Cc: <ebiederm@...>, <linux-kernel@...>
Subject: [PATCH] sysctl: Check length at deprecated_sysctl_warning.
Date: Monday, November 12, 2007 - 11:07 pm

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))

-
[view original message]
From: Andrew Morton <akpm@...>
To: Tetsuo Handa <penguin-kernel@...>
Cc: <ebiederm@...>, <linux-kernel@...>
Subject: Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.
Date: Monday, November 12, 2007 - 11:13 pm

-
[view original message]
From: Tetsuo Handa <penguin-kernel@...>
To: Andrew Morton <akpm@...>
Cc: <ebiederm@...>, <linux-kernel@...>
Subject: Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.
Date: Monday, November 12, 2007 - 11:50 pm

Hello.


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.

-
[view original message]
From: Eric W. Biederman <ebiederm@...>
To: Tetsuo Handa <penguin-kernel@...>
Cc: Andrew Morton <akpm@...>, <ebiederm@...>, <linux-kernel@...>
Subject: Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.
Date: Tuesday, November 13, 2007 - 9:24 am

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


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

-
[view original message]
From: Tetsuo Handa <penguin-kernel@...>
To: <ebiederm@...>
Cc: <akpm@...>, <linux-kernel@...>
Subject: Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.
Date: Monday, November 12, 2007 - 7:42 am

Hello.


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.


-
Previous thread: [PATCH 0/3] virtio PCI driver by Anthony Liguori on Wednesday, November 7, 2007 - 10:46 pm. (33 messages)

Next thread: Re: Fwd: same problem with 2.6.24-rc2 by Randy Dunlap on Thursday, November 8, 2007 - 3:05 am. (2 messages)