Re: [patch] wan/x25_asy: integer overflow in x25_asy_change_mtu()

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:

quoted

From: Dan Carpenter

quoted

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