| Message ID | 20130618074603.GF12329@elgon.mountain |
|---|---|
| State | Not Applicable |
| Headers | show |
On Tue, Jun 18, 2013 at 10:46:03AM +0300, Dan Carpenter wrote: > This overflow is harmless because a few lines later we check: > > if (num_counters != t->private->nentries) { > > But it still upsets the static checkers. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c > index 3d110c4..141350e 100644 > --- a/net/bridge/netfilter/ebtables.c > +++ b/net/bridge/netfilter/ebtables.c > @@ -1278,6 +1278,8 @@ static int do_update_counters(struct net *net, const char *name, > > if (num_counters == 0) > return -EINVAL; > + if (num_counters > INT_MAX / sizeof(*tmp)) > + return -ENOMEM; This is artificially limiting to INT_MAX / sizeof(struct counters). Before this patch, the limit is UINT_MAX / sizeof(struct counters). I think it's very unlikely to hit such a limit though, but as you mentioned we cover the overflow already. Adding it to calm down a static checker sound a bit too much for me. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 20, 2013 at 12:23:52PM +0200, Pablo Neira Ayuso wrote: > On Tue, Jun 18, 2013 at 10:46:03AM +0300, Dan Carpenter wrote: > > This overflow is harmless because a few lines later we check: > > > > if (num_counters != t->private->nentries) { > > > > But it still upsets the static checkers. > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c > > index 3d110c4..141350e 100644 > > --- a/net/bridge/netfilter/ebtables.c > > +++ b/net/bridge/netfilter/ebtables.c > > @@ -1278,6 +1278,8 @@ static int do_update_counters(struct net *net, const char *name, > > > > if (num_counters == 0) > > return -EINVAL; > > + if (num_counters > INT_MAX / sizeof(*tmp)) > > + return -ENOMEM; > > This is artificially limiting to INT_MAX / sizeof(struct counters). > Before this patch, the limit is UINT_MAX / sizeof(struct counters). I > think it's very unlikely to hit such a limit though, but as you > mentioned we cover the overflow already. Adding it to calm down a > static checker sound a bit too much for me. I think we may be talking about different things? "num_counters" comes from the user in update_counters() and we can definitely overflow. I just copied the checks from do_replace() so that's why it uses INT_MAX instead of UINT_MAX. Like I said, the overflow is not harmful because later in the function we check "(num_counters != t->private->nentries)" and return an error before using "tmp". So I don't feel strongly about this patch either way. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" 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/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c index 3d110c4..141350e 100644 --- a/net/bridge/netfilter/ebtables.c +++ b/net/bridge/netfilter/ebtables.c @@ -1278,6 +1278,8 @@ static int do_update_counters(struct net *net, const char *name, if (num_counters == 0) return -EINVAL; + if (num_counters > INT_MAX / sizeof(*tmp)) + return -ENOMEM; tmp = vmalloc(num_counters * sizeof(*tmp)); if (!tmp)
This overflow is harmless because a few lines later we check: if (num_counters != t->private->nentries) { But it still upsets the static checkers. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html