wan/x25_asy: integer overflow in x25_asy_change_mtu()
Subsystems (3)
PATCH 1/1 wan/x25_asy: integer overflow in x25_asy_change_mtu()
If "newmtu * 2 + 4" is too large then it can cause an integer overflow leading to memory corruption. Btw, "newmtu" is not allowed to be a negative number because of the check in dev_set_mtu(), so that's ok.
drivers/net/wan/x25_asy.c +4 -1
@@ -122,8 +122,11 @@ static int x25_asy_change_mtu(struct net_device *dev, int newmtu) {struct x25_asy *sl =netdev_priv (dev);unsigned char *xbuff, *rbuff; -int len =2 * newmtu; +int len; +if (newmtu >INT_MAX /2 -4 ) +return -EINVAL ; + len =2 * newmtu; xbuff =kmalloc (len +4 ,GFP_ATOMIC ); rbuff =kmalloc (len +4 ,GFP_ATOMIC );
From: Dan Carpenter
Quoted 3 lines
If "newmtu * 2 + 4" is too large then it can cause an integer overflow leading to memory corruption. Btw, "newmtu" is not allowed to be a negative number because of the check in
dev_set_mtu(), so that's ok.
This still allows large numbers to be used to allocate almost all of kernel memory - causing massive issues elsewhere.
I'd have thought a 'sanity' limit on the mtu would be more appropriate. I've no idea which mtu is being changed here, and I can't even remember the x.25 protocol well enough if it is an x.25 level 3 limit. But I suspect that a 'sanity' bound to 1MB won't cause any grief.
David
Quoted 24 lines
Signed-off-by Dan Carpenter <[email protected]>drivers/net/wan/x25_asy.c +4
-2@@ -122,8 +122,11 @@ static int x25_asy_change_mtu(struct net_device *dev, int newmtu) {struct x25_asy *sl =netdev_priv (dev);unsigned char *xbuff, *rbuff; -int len =2 * newmtu; +int len; +if (newmtu >INT_MAX /2 -4 ) +return -EINVAL ; + len =2 * newmtu; xbuff =kmalloc (len +4 ,GFP_ATOMIC ); rbuff =kmalloc (len +4 ,GFP_ATOMIC ); --To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 5 lines · David Laight
From: Dan Carpenter
Quoted 3 lines
If "newmtu * 2 + 4" is too large then it can cause an integer overflow leading to memory corruption. Btw, "newmtu" is not allowed to be a negative number because of the check in
dev_set_mtu(), so that's ok.
Quoted 8 lines
This still allows large numbers to be used to allocate almost all of kernel memory - causing massive issues elsewhere.
I'd have thought a 'sanity' limit on the mtu would be more appropriate. I've no idea which mtu is being changed here, and I can't even remember the x.25 protocol well enough if it is an x.25 level 3 limit. But I suspect that a 'sanity' bound to 1MB won't cause any grief.
I agree that a sanity check is probably better but I don't think kmalloc can allocate more than 128k (or something. It's arch dependent as well). So using 1MB is almost no different from my original patch.
What's a better, smaller limit?
regards,
dan carpenter
Quoted 15 lines · Dan Carpenter
Quoted 5 lines · David Laight
From: Dan Carpenter
Quoted 3 lines
If "newmtu * 2 + 4" is too large then it can cause an integer overflow leading to memory corruption. Btw, "newmtu" is not allowed to be a negative number because of the check in
dev_set_mtu(), so that's ok.Quoted 8 lines
This still allows large numbers to be used to allocate almost all of kernel memory - causing massive issues elsewhere.
I'd have thought a 'sanity' limit on the mtu would be more appropriate. I've no idea which mtu is being changed here, and I can't even remember the x.25 protocol well enough if it is an x.25 level 3 limit. But I suspect that a 'sanity' bound to 1MB won't cause any grief.
Quoted 3 lines
I agree that a sanity check is probably better but I don't think kmalloc can allocate more than 128k (or something. It's arch dependent as well). So using 1MB is almost no different from my original patch.
kmalloc() can typically allocate up to 4MB (MAX_ORDER = 11) if you are lucky (enough contiguous memory)
Really, I do not think we should allow more than 65534 MTU, which would allocate two 128K blocks at most.
If some bigger MTU was really needed, we would have switch to vmalloc() a long time ago.
X.25 was limited to 4096 bytes packets if I remember well, and I used 128 and 256 only, that was a long time ago.
( link speeds were limited to 128kbps, it would be quite impractical to use large packets...)
PATCH v2 1/1 wan/x25_asy: integer overflow in x25_asy_change_mtu()
If "newmtu * 2 + 4" is too large then it can cause an integer overflow leading to memory corruption. Eric Dumazet suggests that 65534 is a reasonable upper limit.
Btw, "newmtu" is not allowed to be a negative number because of the check in dev_set_mtu(), so that's ok.
---
v2: Cap it at 65534 instead of just testing for integer overflows.
Thanks David and Eric!
drivers/net/wan/x25_asy.c +5 -1
@@ -122,8 +122,12 @@ static int x25_asy_change_mtu(struct net_device *dev, int newmtu) {struct x25_asy *sl =netdev_priv (dev);unsigned char *xbuff, *rbuff; -int len =2 * newmtu; +int len; +if (newmtu >65534 ) +return -EINVAL ; + + len =2 * newmtu; xbuff =kmalloc (len +4 ,GFP_ATOMIC ); rbuff =kmalloc (len +4 ,GFP_ATOMIC );
From: Dan Carpenter <[email protected]>
Date: Thu, 17 Jul 2014 13:50:45 +0300
Quoted 11 lines
If "newmtu * 2 + 4" is too large then it can cause an integer overflow leading to memory corruption. Eric Dumazet suggests that 65534 is a reasonable upper limit.
Btw, "newmtu" is not allowed to be a negative number because of the check in
dev_set_mtu(), so that's ok.Signed-off-by Dan Carpenter <[email protected]>---
v2: Cap it at 65534 instead of just testing for integer overflows.
Thanks David and Eric!
Applied.