Re: [PATCH] usb: fix potential integer overflow in usb_sg_init
From: Alan Stern
Date: Tue Jan 19 2016 - 17:05:07 EST
On Tue, 19 Jan 2016, Insu Yun wrote:
> On Tue, Jan 19, 2016 at 9:57 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> > On Mon, 18 Jan 2016, Insu Yun wrote:
> > > On Mon, Jan 18, 2016 at 1:32 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> > > wrote:
> > >
> > > > On Mon, 18 Jan 2016, Insu Yun wrote:
> > > >
> > > > > If nents value is sufficient large, e.g 0x40000000,
> > > > > then it can overflow size in kmalloc and heap overflow happesns.
> > > > > Therefore nents value needs to be checked to prevent overflow.
> > > >
> > > > I don't see why. You seem to be assuming that failure with -EINVAL is
> > > > better than failure with a heap overflow. I disagree; a heap overflow
> > > > provides more debugging information to help locate the reason for the
> > > > underlying problem.
> > > >
> > >
> > > I agree that heap overflow gives more information than -EINVAL.
> > > However, I think -EINVAL already gives sufficient information for
> > debugging.
> > Actually it doesn't. -EINVAL return codes occur all over the place.
> > It's not easy to tell exactly what went wrong when one of them pops up.
> > > And I thin crash is bad, so returning -EINVAL seems better.
> > A better solution in this case would be to avoid overflows by changing
> > the kmalloc call to kmalloc_array, instead of duplicating the code.
> But if we use kmalloc_array, then it cannot be distinguished between memory
> pressure and overflow case.
> It will be failed with -ENOMEM.
> I think it is worse in terms of debugging information that you mentioned.
In practice, it doesn't matter whether the failure is caused by memory
pressure or numerical overflow. In both cases the underlying reason is
the same: too many scatterlist entries.
Besides, this is all highly theoretical. Do you have any evidence that
people are encountering cases where nents is larger than (say) 20?
If you want to prevent overflows and crashes, then fine -- use
kmalloc_array. Other than that, I don't see any need to change the