Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.
Tetsuo Handa <[EMAIL PROTECTED]> 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.
Tetsuo Handa [EMAIL PROTECTED] 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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.
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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.
On Tue, 13 Nov 2007 12:07:23 +0900 Tetsuo Handa <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> > Cc: "Eric W. Biederman" <[EMAIL PROTECTED]> > Signed-off-by: Andrew Morton <[EMAIL PROTECTED]> > --- > > 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)) - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.
Tetsuo Handa <[EMAIL PROTECTED]> 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.
Tetsuo Handa [EMAIL PROTECTED] 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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.
On Tue, 13 Nov 2007 12:07:23 +0900 Tetsuo Handa [EMAIL PROTECTED] 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 [EMAIL PROTECTED] Cc: Eric W. Biederman [EMAIL PROTECTED] Signed-off-by: Andrew Morton [EMAIL PROTECTED] --- 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)) - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.
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.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.
> On Thu, 08 Nov 2007 11:57:26 +0900 Tetsuo Handa <[EMAIL PROTECTED]> wrote: > Original patch assumed args->nlen < CTL_MAXNAME, but it can be false. > > Signed-off-by: Tetsuo Handa <[EMAIL PROTECTED]> > > > --- linux-2.6.22-rc2.orig/kernel/sysctl.c 2007-11-08 10:38:17.0 > +0900 > +++ linux-2.6.22-rc2/kernel/sysctl.c 2007-11-08 11:24:27.0 +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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sysctl: Check length at deprecated_sysctl_warning.
On Thu, 08 Nov 2007 11:57:26 +0900 Tetsuo Handa [EMAIL PROTECTED] wrote: Original patch assumed args-nlen CTL_MAXNAME, but it can be false. Signed-off-by: Tetsuo Handa [EMAIL PROTECTED] --- linux-2.6.22-rc2.orig/kernel/sysctl.c 2007-11-08 10:38:17.0 +0900 +++ linux-2.6.22-rc2/kernel/sysctl.c 2007-11-08 11:24:27.0 +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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
