netfilter: prevent harmless integer overflow

PATCH In review Dan Carpenter 2 participants 3 messages Jun 18, 2013 – Jun 20, 2013

Subsystems (5)

Networking Network drivers Kernel janitors netfilter +1 more subsystems

lore

Share & tools

lore

Dan Carpenter

author

2013-06-18 07:46 UTC #0 lore raw

PATCH 1/1 netfilter: prevent harmless integer overflow

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 <[email protected]>

net/bridge/netfilter/ebtables.c +2 -0

index 3d110c4..141350e 100644
@@ -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)

Pablo Neira Ayuso

maintainer

2013-06-20 10:23 UTC #1 lore raw

Quoted 13 lines · Dan Carpenter

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 <[email protected]>
net/bridge/netfilter/ebtables.c +0 -0
index 3d110c4..141350e 100644

net/bridge/netfilter/ebtables.c static int do_update_counters(struct net *net, const char *name, · line 1282

Quoted 5 lines

net/bridge/netfilter/ebtables.c +2 -0

@@ -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.

Dan Carpenter

author

2013-06-20 11:09 UTC #2 lore raw

Quoted 15 lines · Pablo Neira Ayuso

Quoted 14 lines · Dan Carpenter
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 <[email protected]>
net/bridge/netfilter/ebtables.c +0 -0
index 3d110c4..141350e 100644
@@ -1278,6 +1278,8 @@ static int do_update_counters(struct net *net, const char *name,
 

Quoted 10 lines

net/bridge/netfilter/ebtables.c static int do_update_counters(struct net *net, const char *name, · line 1282
Quoted 4 lines
net/bridge/netfilter/ebtables.c +2 -0
 	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