Message ID | 20140717080310.GB477@mwanda |
---|---|
State | Superseded, archived |
Delegated to: | David Miller |
Headers | show |
From: Dan Carpenter > 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 > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > diff --git a/drivers/net/wan/x25_asy.c b/drivers/net/wan/x25_asy.c > index 5895f19..f04c8c1 100644 > --- a/drivers/net/wan/x25_asy.c > +++ b/drivers/net/wan/x25_asy.c > @@ -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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- 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 Thu, Jul 17, 2014 at 08:45:58AM +0000, David Laight wrote: > From: Dan Carpenter > > 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. > 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 -- 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 Thu, 2014-07-17 at 11:58 +0300, Dan Carpenter wrote: > On Thu, Jul 17, 2014 at 08:45:58AM +0000, David Laight wrote: > > From: Dan Carpenter > > > 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. > > > > 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...) -- 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/drivers/net/wan/x25_asy.c b/drivers/net/wan/x25_asy.c index 5895f19..f04c8c1 100644 --- a/drivers/net/wan/x25_asy.c +++ b/drivers/net/wan/x25_asy.c @@ -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);
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. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> -- 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