[PATCH] usb: fix potential integer overflow in usb_sg_init

[PATCH] usb: fix potential integer overflow in usb_sg_init

[Insu Yun]

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.

Signed-off-by: Insu Yun <wuninsu@gmail.com>
---
 drivers/usb/core/message.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 8e641b5..53393d5 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -367,7 +367,8 @@ int usb_sg_init(struct usb_sg_request *io, struct usb_device *dev,
 	if (!io || !dev || !sg
 			|| usb_pipecontrol(pipe)
 			|| usb_pipeisoc(pipe)
-			|| nents <= 0)
+			|| nents <= 0
+			|| nents >= UINT_MAX / sizeof(*io->urbs))
 		return -EINVAL;
 
 	spin_lock_init(&io->lock);
-- 
1.9.1

Re: [PATCH] usb: fix potential integer overflow in usb_sg_init

[Alan Stern]

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.

Alan Stern

Re: [PATCH] usb: fix potential integer overflow in usb_sg_init

[Alan Stern]

On Mon, 18 Jan 2016, Insu Yun wrote:

> On Mon, Jan 18, 2016 at 1:32 PM, Alan Stern <stern@rowland.harvard.edu>
> 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.

Alan Stern

Re: [PATCH] usb: fix potential integer overflow in usb_sg_init

[Alan Stern]

On Tue, 19 Jan 2016, Insu Yun wrote:

> On Tue, Jan 19, 2016 at 9:57 AM, Alan Stern <stern@rowland.harvard.edu>
> wrote:
> 
> > On Mon, 18 Jan 2016, Insu Yun wrote:
> >
> > > On Mon, Jan 18, 2016 at 1:32 PM, Alan Stern <stern@rowland.harvard.edu>
> > > 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 
code.

Alan Stern