| Message ID | 20170817170931.24545-5-phil@nwl.cc |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | stephen hemminger |
| Headers | show |
From: Phil Sutter > Sent: 17 August 2017 18:10 > The later check for 'k[0] != 0' requires a non-empty filter name, > otherwise NULL pointer dereference in 'q' might happen. > > Signed-off-by: Phil Sutter <phil@nwl.cc> > --- > tc/tc_filter.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/tc/tc_filter.c b/tc/tc_filter.c > index b13fb9185d4fd..a799edb35886d 100644 > --- a/tc/tc_filter.c > +++ b/tc/tc_filter.c > @@ -412,6 +412,9 @@ static int tc_filter_get(int cmd, unsigned int flags, int argc, char **argv) > usage(); > return 0; > } else { > + if (!strlen(*argv)) > + invarg("invalid filter name", *argv); That is nearly as bad as: p[strlen(p)] = 0; > + > strncpy(k, *argv, sizeof(k)-1); > > q = get_filter_kind(k); > -- > 2.13.1
On Fri, Aug 18, 2017 at 09:30:35AM +0000, David Laight wrote: > From: Phil Sutter > > Sent: 17 August 2017 18:10 > > The later check for 'k[0] != 0' requires a non-empty filter name, > > otherwise NULL pointer dereference in 'q' might happen. > > > > Signed-off-by: Phil Sutter <phil@nwl.cc> > > --- > > tc/tc_filter.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/tc/tc_filter.c b/tc/tc_filter.c > > index b13fb9185d4fd..a799edb35886d 100644 > > --- a/tc/tc_filter.c > > +++ b/tc/tc_filter.c > > @@ -412,6 +412,9 @@ static int tc_filter_get(int cmd, unsigned int flags, int argc, char **argv) > > usage(); > > return 0; > > } else { > > + if (!strlen(*argv)) > > + invarg("invalid filter name", *argv); > > That is nearly as bad as: > p[strlen(p)] = 0; Hey, it's not impossible! I could call tc like so: | # tc filter get protocol ip prio 1 "" What's funny about it is that the first call to matches() in tc_filter_get() will catch the empty last parameter: | # tc filter get prio 1 protocol ip "" | Command line is not complete. Try option "help" | # ./tc/tc filter get prio 1 protocol ip "" "" | Error: duplicate "priority": "" is the second value. Cheers, Phil
From: Phil Sutter > Sent: 18 August 2017 12:16 > On Fri, Aug 18, 2017 at 09:30:35AM +0000, David Laight wrote: > > From: Phil Sutter > > > Sent: 17 August 2017 18:10 > > > The later check for 'k[0] != 0' requires a non-empty filter name, > > > otherwise NULL pointer dereference in 'q' might happen. > > > > > > Signed-off-by: Phil Sutter <phil@nwl.cc> > > > --- > > > tc/tc_filter.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/tc/tc_filter.c b/tc/tc_filter.c > > > index b13fb9185d4fd..a799edb35886d 100644 > > > --- a/tc/tc_filter.c > > > +++ b/tc/tc_filter.c > > > @@ -412,6 +412,9 @@ static int tc_filter_get(int cmd, unsigned int flags, int argc, char **argv) > > > usage(); > > > return 0; > > > } else { > > > + if (!strlen(*argv)) > > > + invarg("invalid filter name", *argv); > > > > That is nearly as bad as: > > p[strlen(p)] = 0; > > Hey, it's not impossible! I could call tc like so: > > | # tc filter get protocol ip prio 1 "" You missed the point. Just check **argv there is no need to determine the length just to check it is non-zero. David
On Fri, Aug 18, 2017 at 04:34:44PM +0000, David Laight wrote: > From: Phil Sutter > > Sent: 18 August 2017 12:16 > > On Fri, Aug 18, 2017 at 09:30:35AM +0000, David Laight wrote: > > > From: Phil Sutter > > > > Sent: 17 August 2017 18:10 > > > > The later check for 'k[0] != 0' requires a non-empty filter name, > > > > otherwise NULL pointer dereference in 'q' might happen. > > > > > > > > Signed-off-by: Phil Sutter <phil@nwl.cc> > > > > --- > > > > tc/tc_filter.c | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/tc/tc_filter.c b/tc/tc_filter.c > > > > index b13fb9185d4fd..a799edb35886d 100644 > > > > --- a/tc/tc_filter.c > > > > +++ b/tc/tc_filter.c > > > > @@ -412,6 +412,9 @@ static int tc_filter_get(int cmd, unsigned int flags, int argc, char **argv) > > > > usage(); > > > > return 0; > > > > } else { > > > > + if (!strlen(*argv)) > > > > + invarg("invalid filter name", *argv); > > > > > > That is nearly as bad as: > > > p[strlen(p)] = 0; > > > > Hey, it's not impossible! I could call tc like so: > > > > | # tc filter get protocol ip prio 1 "" > > You missed the point. Just check **argv there is no need to > determine the length just to check it is non-zero. Oh, hehe. Thanks for the short-cut! Thanks, PHil
diff --git a/tc/tc_filter.c b/tc/tc_filter.c index b13fb9185d4fd..a799edb35886d 100644 --- a/tc/tc_filter.c +++ b/tc/tc_filter.c @@ -412,6 +412,9 @@ static int tc_filter_get(int cmd, unsigned int flags, int argc, char **argv) usage(); return 0; } else { + if (!strlen(*argv)) + invarg("invalid filter name", *argv); + strncpy(k, *argv, sizeof(k)-1); q = get_filter_kind(k);
The later check for 'k[0] != 0' requires a non-empty filter name, otherwise NULL pointer dereference in 'q' might happen. Signed-off-by: Phil Sutter <phil@nwl.cc> --- tc/tc_filter.c | 3 +++ 1 file changed, 3 insertions(+)