* [PATCH] sysctl: Check length at deprecated_sysctl_warning.
@ 2007-11-08 2:57 Tetsuo Handa
2007-11-08 3:22 ` Andrew Morton
0 siblings, 1 reply; 9+ messages in thread
From: Tetsuo Handa @ 2007-11-08 2:57 UTC (permalink / raw)
To: akpm; +Cc: ebiederm, linux-kernel
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))
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.
2007-11-08 2:57 [PATCH] sysctl: Check length at deprecated_sysctl_warning Tetsuo Handa
@ 2007-11-08 3:22 ` Andrew Morton
2007-11-08 8:19 ` Tetsuo Handa
0 siblings, 1 reply; 9+ messages in thread
From: Andrew Morton @ 2007-11-08 3:22 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: ebiederm, linux-kernel
> 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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.
2007-11-08 3:22 ` Andrew Morton
@ 2007-11-08 8:19 ` Tetsuo Handa
2007-11-12 9:44 ` Eric W. Biederman
0 siblings, 1 reply; 9+ messages in thread
From: Tetsuo Handa @ 2007-11-08 8:19 UTC (permalink / raw)
To: Andrew Morton; +Cc: ebiederm, linux-kernel, 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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.
2007-11-08 8:19 ` Tetsuo Handa
@ 2007-11-12 9:44 ` Eric W. Biederman
2007-11-12 11:42 ` Tetsuo Handa
2007-11-13 3:07 ` Tetsuo Handa
0 siblings, 2 replies; 9+ messages in thread
From: Eric W. Biederman @ 2007-11-12 9:44 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: Andrew Morton, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.
2007-11-12 9:44 ` Eric W. Biederman
@ 2007-11-12 11:42 ` Tetsuo Handa
2007-11-13 3:07 ` Tetsuo Handa
1 sibling, 0 replies; 9+ messages in thread
From: Tetsuo Handa @ 2007-11-12 11:42 UTC (permalink / raw)
To: ebiederm; +Cc: akpm, linux-kernel
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] sysctl: Check length at deprecated_sysctl_warning.
2007-11-12 9:44 ` Eric W. Biederman
2007-11-12 11:42 ` Tetsuo Handa
@ 2007-11-13 3:07 ` Tetsuo Handa
2007-11-13 3:13 ` Andrew Morton
1 sibling, 1 reply; 9+ messages in thread
From: Tetsuo Handa @ 2007-11-13 3:07 UTC (permalink / raw)
To: akpm; +Cc: ebiederm, linux-kernel
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))
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.
2007-11-13 3:07 ` Tetsuo Handa
@ 2007-11-13 3:13 ` Andrew Morton
2007-11-13 3:50 ` Tetsuo Handa
0 siblings, 1 reply; 9+ messages in thread
From: Andrew Morton @ 2007-11-13 3:13 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: ebiederm, linux-kernel
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))
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.
2007-11-13 3:13 ` Andrew Morton
@ 2007-11-13 3:50 ` Tetsuo Handa
2007-11-13 13:24 ` Eric W. Biederman
0 siblings, 1 reply; 9+ messages in thread
From: Tetsuo Handa @ 2007-11-13 3:50 UTC (permalink / raw)
To: Andrew Morton; +Cc: ebiederm, linux-kernel
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.
2007-11-13 3:50 ` Tetsuo Handa
@ 2007-11-13 13:24 ` Eric W. Biederman
0 siblings, 0 replies; 9+ messages in thread
From: Eric W. Biederman @ 2007-11-13 13:24 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: Andrew Morton, ebiederm, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2007-11-13 13:25 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-11-08 2:57 [PATCH] sysctl: Check length at deprecated_sysctl_warning Tetsuo Handa
2007-11-08 3:22 ` Andrew Morton
2007-11-08 8:19 ` Tetsuo Handa
2007-11-12 9:44 ` Eric W. Biederman
2007-11-12 11:42 ` Tetsuo Handa
2007-11-13 3:07 ` Tetsuo Handa
2007-11-13 3:13 ` Andrew Morton
2007-11-13 3:50 ` Tetsuo Handa
2007-11-13 13:24 ` Eric W. Biederman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).