| Message ID | 1286025469.2582.1806.camel@edumazet-laptop |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Sat, Oct 02, 2010 at 03:17:49PM +0200, Eric Dumazet wrote: >When proc_doulongvec_minmax() is used with an array of longs, >and no min/max check requested (.extra1 or .extra2 being NULL), we >dereference a NULL pointer for the second element of the array. > >Noticed while doing some changes in network stack for the "16TB problem" > >Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> >--- > kernel/sysctl.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > >diff --git a/kernel/sysctl.c b/kernel/sysctl.c >index f88552c..4fba86d 100644 >--- a/kernel/sysctl.c >+++ b/kernel/sysctl.c >@@ -2500,7 +2500,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int > break; > if (neg) > continue; >- if ((min && val < *min) || (max && val > *max)) >+ if ((table->extra1 && val < *min) || >+ (table->extra2 && val > *max)) > continue; Yes? This is wrong for me, min and max are pointers to ->extra{1,2}, they are increased within the for loop, you are only checking the the pointer to the first element of ->extra{1,2}. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Oct 02, 2010 at 03:17:49PM +0200, Eric Dumazet wrote: > When proc_doulongvec_minmax() is used with an array of longs, > and no min/max check requested (.extra1 or .extra2 being NULL), we > dereference a NULL pointer for the second element of the array. > > Noticed while doing some changes in network stack for the "16TB problem" > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> > --- > kernel/sysctl.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/kernel/sysctl.c b/kernel/sysctl.c > index f88552c..4fba86d 100644 > --- a/kernel/sysctl.c > +++ b/kernel/sysctl.c > @@ -2500,7 +2500,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int > break; > if (neg) > continue; > - if ((min && val < *min) || (max && val > *max)) > + if ((table->extra1 && val < *min) || > + (table->extra2 && val > *max)) How about changing: for (; left && vleft--; i++, min++, max++, first=0) { into: for (; left && vleft--; i++, min = min ? min + 1 : NULL, max = max ? max + 1: NULL, first=0) { That would make min and max correct and reduce the chances somebody in the future overlooks the fact they are currently filled with garbage. Robin -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le lundi 04 octobre 2010 à 03:59 -0500, Robin Holt a écrit : > On Sat, Oct 02, 2010 at 03:17:49PM +0200, Eric Dumazet wrote: > > When proc_doulongvec_minmax() is used with an array of longs, > > and no min/max check requested (.extra1 or .extra2 being NULL), we > > dereference a NULL pointer for the second element of the array. > > > > Noticed while doing some changes in network stack for the "16TB problem" > > > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> > > --- > > kernel/sysctl.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/kernel/sysctl.c b/kernel/sysctl.c > > index f88552c..4fba86d 100644 > > --- a/kernel/sysctl.c > > +++ b/kernel/sysctl.c > > @@ -2500,7 +2500,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int > > break; > > if (neg) > > continue; > > - if ((min && val < *min) || (max && val > *max)) > > + if ((table->extra1 && val < *min) || > > + (table->extra2 && val > *max)) > > How about changing: > for (; left && vleft--; i++, min++, max++, first=0) { > into: > for (; left && vleft--; i++, min = min ? min + 1 : NULL, max = max ? max + 1: NULL, first=0) { > > That would make min and max correct and reduce the chances somebody in > the future overlooks the fact they are currently filled with garbage. I prefer my solution, because the check is done only in the 'write' case, while its done also for 'read' in your solution, not counting the for (;;) is really ugly... -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 04, 2010 at 11:04:18AM +0200, Eric Dumazet wrote: >Le lundi 04 octobre 2010 à 03:59 -0500, Robin Holt a écrit : >> On Sat, Oct 02, 2010 at 03:17:49PM +0200, Eric Dumazet wrote: >> > When proc_doulongvec_minmax() is used with an array of longs, >> > and no min/max check requested (.extra1 or .extra2 being NULL), we >> > dereference a NULL pointer for the second element of the array. >> > >> > Noticed while doing some changes in network stack for the "16TB problem" >> > >> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> >> > --- >> > kernel/sysctl.c | 3 ++- >> > 1 file changed, 2 insertions(+), 1 deletion(-) >> > >> > diff --git a/kernel/sysctl.c b/kernel/sysctl.c >> > index f88552c..4fba86d 100644 >> > --- a/kernel/sysctl.c >> > +++ b/kernel/sysctl.c >> > @@ -2500,7 +2500,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int >> > break; >> > if (neg) >> > continue; >> > - if ((min && val < *min) || (max && val > *max)) >> > + if ((table->extra1 && val < *min) || >> > + (table->extra2 && val > *max)) >> >> How about changing: >> for (; left && vleft--; i++, min++, max++, first=0) { >> into: >> for (; left && vleft--; i++, min = min ? min + 1 : NULL, max = max ? max + 1: NULL, first=0) { >> >> That would make min and max correct and reduce the chances somebody in >> the future overlooks the fact they are currently filled with garbage. > >I prefer my solution, because the check is done only in the 'write' >case, while its done also for 'read' in your solution, not counting the >for (;;) is really ugly... > Sorry, I still don't get the point here, min and max are pointers, they are already checked before dereferenced. After your patch, min and max will be still increased, while you are still checking ->extra{1,2} which is wrong. I see no problem with the original code, or I must have missed something? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le lundi 04 octobre 2010 à 17:34 +0800, Américo Wang a écrit : > On Mon, Oct 04, 2010 at 11:04:18AM +0200, Eric Dumazet wrote: > >Le lundi 04 octobre 2010 à 03:59 -0500, Robin Holt a écrit : > >> On Sat, Oct 02, 2010 at 03:17:49PM +0200, Eric Dumazet wrote: > >> > When proc_doulongvec_minmax() is used with an array of longs, > >> > and no min/max check requested (.extra1 or .extra2 being NULL), we > >> > dereference a NULL pointer for the second element of the array. > >> > > >> > Noticed while doing some changes in network stack for the "16TB problem" > >> > > >> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> > >> > --- > >> > kernel/sysctl.c | 3 ++- > >> > 1 file changed, 2 insertions(+), 1 deletion(-) > >> > > >> > diff --git a/kernel/sysctl.c b/kernel/sysctl.c > >> > index f88552c..4fba86d 100644 > >> > --- a/kernel/sysctl.c > >> > +++ b/kernel/sysctl.c > >> > @@ -2500,7 +2500,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int > >> > break; > >> > if (neg) > >> > continue; > >> > - if ((min && val < *min) || (max && val > *max)) > >> > + if ((table->extra1 && val < *min) || > >> > + (table->extra2 && val > *max)) > >> > >> How about changing: > >> for (; left && vleft--; i++, min++, max++, first=0) { > >> into: > >> for (; left && vleft--; i++, min = min ? min + 1 : NULL, max = max ? max + 1: NULL, first=0) { > >> > >> That would make min and max correct and reduce the chances somebody in > >> the future overlooks the fact they are currently filled with garbage. > > > >I prefer my solution, because the check is done only in the 'write' > >case, while its done also for 'read' in your solution, not counting the > >for (;;) is really ugly... > > > > Sorry, I still don't get the point here, min and max > are pointers, they are already checked before dereferenced. > After your patch, min and max will be still increased, while > you are still checking ->extra{1,2} which is wrong. > > I see no problem with the original code, or I must have missed something? Please re-read again. I had crashes, so original code is bugyy. Say you call __do_proc_doulongvec_minmax() with an array of three elements. And .extra1 = NULL, .extra2 = NULL (no range checks, this is a valid use case) First element, min = NULL OK. no problem so far. Second element, min = (long *)(NULL + sizeof(long)) -> BUG Third element, min = (long *)(NULL + 2*sizeof(long)) -> BUG After my patch, min/max increases normally (they are only pointers after all) But they are _dereferenced_ only if they were _not_ NULL at the beginning (extra1 not NULL for *min, extra2 not NULL for *max) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 04, 2010 at 12:10:30PM +0200, Eric Dumazet wrote: >Le lundi 04 octobre 2010 à 17:34 +0800, Américo Wang a écrit : >> On Mon, Oct 04, 2010 at 11:04:18AM +0200, Eric Dumazet wrote: >> >Le lundi 04 octobre 2010 à 03:59 -0500, Robin Holt a écrit : >> >> On Sat, Oct 02, 2010 at 03:17:49PM +0200, Eric Dumazet wrote: >> >> > When proc_doulongvec_minmax() is used with an array of longs, >> >> > and no min/max check requested (.extra1 or .extra2 being NULL), we >> >> > dereference a NULL pointer for the second element of the array. >> >> > >> >> > Noticed while doing some changes in network stack for the "16TB problem" >> >> > >> >> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> >> >> > --- >> >> > kernel/sysctl.c | 3 ++- >> >> > 1 file changed, 2 insertions(+), 1 deletion(-) >> >> > >> >> > diff --git a/kernel/sysctl.c b/kernel/sysctl.c >> >> > index f88552c..4fba86d 100644 >> >> > --- a/kernel/sysctl.c >> >> > +++ b/kernel/sysctl.c >> >> > @@ -2500,7 +2500,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int >> >> > break; >> >> > if (neg) >> >> > continue; >> >> > - if ((min && val < *min) || (max && val > *max)) >> >> > + if ((table->extra1 && val < *min) || >> >> > + (table->extra2 && val > *max)) >> >> >> >> How about changing: >> >> for (; left && vleft--; i++, min++, max++, first=0) { >> >> into: >> >> for (; left && vleft--; i++, min = min ? min + 1 : NULL, max = max ? max + 1: NULL, first=0) { >> >> >> >> That would make min and max correct and reduce the chances somebody in >> >> the future overlooks the fact they are currently filled with garbage. >> > >> >I prefer my solution, because the check is done only in the 'write' >> >case, while its done also for 'read' in your solution, not counting the >> >for (;;) is really ugly... >> > >> >> Sorry, I still don't get the point here, min and max >> are pointers, they are already checked before dereferenced. >> After your patch, min and max will be still increased, while >> you are still checking ->extra{1,2} which is wrong. >> >> I see no problem with the original code, or I must have missed something? > >Please re-read again. I had crashes, so original code is bugyy. > >Say you call __do_proc_doulongvec_minmax() with an array of three >elements. And .extra1 = NULL, .extra2 = NULL (no range checks, this is a >valid use case) > >First element, min = NULL OK. no problem so far. > >Second element, min = (long *)(NULL + sizeof(long)) -> BUG > >Third element, min = (long *)(NULL + 2*sizeof(long)) -> BUG > >After my patch, min/max increases normally (they are only pointers after >all) > >But they are _dereferenced_ only if they were _not_ NULL at the >beginning (extra1 not NULL for *min, extra2 not NULL for *max) > Hmm, I see, thanks for explanation. Your patch does fix the problem, but seems not a good solution, we should skip all min/max checking if ->extra(1|2) is NULL, instead of checking it every time within the loop. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le lundi 04 octobre 2010 à 18:35 +0800, Américo Wang a écrit : > Your patch does fix the problem, but seems not a good solution, > we should skip all min/max checking if ->extra(1|2) is NULL, > instead of checking it every time within the loop. Please do submit a patch, we'll see if you come to a better solution, with no added code size (this is slow path, I dont care for checking it 'every time winthin the loop') -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/kernel/sysctl.c b/kernel/sysctl.c index f88552c..4fba86d 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -2500,7 +2500,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int break; if (neg) continue; - if ((min && val < *min) || (max && val > *max)) + if ((table->extra1 && val < *min) || + (table->extra2 && val > *max)) continue; *i = val; } else {
When proc_doulongvec_minmax() is used with an array of longs, and no min/max check requested (.extra1 or .extra2 being NULL), we dereference a NULL pointer for the second element of the array. Noticed while doing some changes in network stack for the "16TB problem" Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> --- kernel/sysctl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html