2016-04-08 09:08:13

by Chunfeng Yun

[permalink] [raw]
Subject: [PATCH] usb: core: buffer: avoid NULL pointer dereferrence

NULL pointer dereferrence will happen when class driver
wants to allocate zero length buffer and pool_max[0]
can't be used, so skip reserved pool in this case.

Signed-off-by: Chunfeng Yun <[email protected]>
---
drivers/usb/core/buffer.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c
index 2741566..c8f958b 100644
--- a/drivers/usb/core/buffer.c
+++ b/drivers/usb/core/buffer.c
@@ -131,7 +131,7 @@ void *hcd_buffer_alloc(
}

for (i = 0; i < HCD_BUFFER_POOLS; i++) {
- if (size <= pool_max[i])
+ if (pool_max[i] && size <= pool_max[i])
return dma_pool_alloc(hcd->pool[i], mem_flags, dma);
}
return dma_alloc_coherent(hcd->self.controller, size, dma, mem_flags);
@@ -158,7 +158,7 @@ void hcd_buffer_free(
}

for (i = 0; i < HCD_BUFFER_POOLS; i++) {
- if (size <= pool_max[i]) {
+ if (pool_max[i] && size <= pool_max[i]) {
dma_pool_free(hcd->pool[i], addr, dma);
return;
}
--
1.7.9.5


2016-04-08 14:07:06

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH] usb: core: buffer: avoid NULL pointer dereferrence

On Fri, Apr 08, 2016 at 05:08:03PM +0800, Chunfeng Yun wrote:
> NULL pointer dereferrence will happen when class driver
> wants to allocate zero length buffer and pool_max[0]
> can't be used, so skip reserved pool in this case.

Why would a driver want to allocate a 0 length buffer? What driver does
this?

Shouldn't we fix that issue instead?

thanks,

greg k-h

2016-04-08 15:21:09

by Alan Stern

[permalink] [raw]
Subject: Re: [PATCH] usb: core: buffer: avoid NULL pointer dereferrence

On Fri, 8 Apr 2016, Greg Kroah-Hartman wrote:

> On Fri, Apr 08, 2016 at 05:08:03PM +0800, Chunfeng Yun wrote:
> > NULL pointer dereferrence will happen when class driver
> > wants to allocate zero length buffer and pool_max[0]
> > can't be used, so skip reserved pool in this case.
>
> Why would a driver want to allocate a 0 length buffer? What driver does
> this?
>
> Shouldn't we fix that issue instead?

And even if a driver does want to allocate a 0-length buffer, shouldn't
the function simply return early instead of running through all its
calculations?

Alan Stern

2016-04-11 03:01:56

by Chunfeng Yun

[permalink] [raw]
Subject: Re: [PATCH] usb: core: buffer: avoid NULL pointer dereferrence

On Fri, 2016-04-08 at 07:07 -0700, Greg Kroah-Hartman wrote:
> On Fri, Apr 08, 2016 at 05:08:03PM +0800, Chunfeng Yun wrote:
> > NULL pointer dereferrence will happen when class driver
> > wants to allocate zero length buffer and pool_max[0]
> > can't be used, so skip reserved pool in this case.
>
> Why would a driver want to allocate a 0 length buffer? What driver does
> this?
It's misc/usbtest.c
>
> Shouldn't we fix that issue instead?
I don't know which way is better, but it seems simple to fix it up in
buffer.c
>
> thanks,
>
> greg k-h


2016-04-11 05:09:40

by Felipe Balbi

[permalink] [raw]
Subject: Re: [PATCH] usb: core: buffer: avoid NULL pointer dereferrence


Hi,

chunfeng yun <[email protected]> writes:
> On Fri, 2016-04-08 at 07:07 -0700, Greg Kroah-Hartman wrote:
>> On Fri, Apr 08, 2016 at 05:08:03PM +0800, Chunfeng Yun wrote:
>> > NULL pointer dereferrence will happen when class driver
>> > wants to allocate zero length buffer and pool_max[0]
>> > can't be used, so skip reserved pool in this case.
>>
>> Why would a driver want to allocate a 0 length buffer? What driver does
>> this?
> It's misc/usbtest.c

that'll do what you ask it to do with the userspace tool testusb. Are
you trying to pass a size of 0 ?

>> Shouldn't we fix that issue instead?
> I don't know which way is better, but it seems simple to fix it up in
> buffer.c

I think we should, really, avoid a 0-length allocation, but passing a
size of 0 to testusb isn't very good either ;-) How are you calling
testusb ?

--
balbi


Attachments:
signature.asc (818.00 B)

2016-04-11 07:16:24

by Chunfeng Yun

[permalink] [raw]
Subject: Re: [PATCH] usb: core: buffer: avoid NULL pointer dereferrence

On Mon, 2016-04-11 at 08:07 +0300, Felipe Balbi wrote:
> Hi,
>
> chunfeng yun <[email protected]> writes:
> > On Fri, 2016-04-08 at 07:07 -0700, Greg Kroah-Hartman wrote:
> >> On Fri, Apr 08, 2016 at 05:08:03PM +0800, Chunfeng Yun wrote:
> >> > NULL pointer dereferrence will happen when class driver
> >> > wants to allocate zero length buffer and pool_max[0]
> >> > can't be used, so skip reserved pool in this case.
> >>
> >> Why would a driver want to allocate a 0 length buffer? What driver does
> >> this?
> > It's misc/usbtest.c
>
> that'll do what you ask it to do with the userspace tool testusb. Are
> you trying to pass a size of 0 ?
>
No, I just ran "testusb -t10" which called test_ctrl_queue().
In this function, sub-case 8 passed a parameter @len as 0 to
simple_alloc_urb(), and then it tried to allocate a 0-length buffer.

> >> Shouldn't we fix that issue instead?
> > I don't know which way is better, but it seems simple to fix it up in
> > buffer.c
>
> I think we should, really, avoid a 0-length allocation, but passing a
> size of 0 to testusb isn't very good either ;-) How are you calling
> testusb ?
>
As explained above.

2016-04-11 08:26:28

by Felipe Balbi

[permalink] [raw]
Subject: Re: [PATCH] usb: core: buffer: avoid NULL pointer dereferrence


Hi,

chunfeng yun <[email protected]> writes:
>> > On Fri, 2016-04-08 at 07:07 -0700, Greg Kroah-Hartman wrote:
>> >> On Fri, Apr 08, 2016 at 05:08:03PM +0800, Chunfeng Yun wrote:
>> >> > NULL pointer dereferrence will happen when class driver
>> >> > wants to allocate zero length buffer and pool_max[0]
>> >> > can't be used, so skip reserved pool in this case.
>> >>
>> >> Why would a driver want to allocate a 0 length buffer? What driver does
>> >> this?
>> > It's misc/usbtest.c
>>
>> that'll do what you ask it to do with the userspace tool testusb. Are
>> you trying to pass a size of 0 ?
>>
> No, I just ran "testusb -t10" which called test_ctrl_queue().
> In this function, sub-case 8 passed a parameter @len as 0 to
> simple_alloc_urb(), and then it tried to allocate a 0-length buffer.

odd, I have never seen this problem running testusb with the same
arguments.

--
balbi


Attachments:
signature.asc (818.00 B)

2016-04-11 14:44:49

by Alan Stern

[permalink] [raw]
Subject: Re: [PATCH] usb: core: buffer: avoid NULL pointer dereferrence

On Mon, 11 Apr 2016, chunfeng yun wrote:

> On Mon, 2016-04-11 at 08:07 +0300, Felipe Balbi wrote:
> > Hi,
> >
> > chunfeng yun <[email protected]> writes:
> > > On Fri, 2016-04-08 at 07:07 -0700, Greg Kroah-Hartman wrote:
> > >> On Fri, Apr 08, 2016 at 05:08:03PM +0800, Chunfeng Yun wrote:
> > >> > NULL pointer dereferrence will happen when class driver
> > >> > wants to allocate zero length buffer and pool_max[0]
> > >> > can't be used, so skip reserved pool in this case.
> > >>
> > >> Why would a driver want to allocate a 0 length buffer? What driver does
> > >> this?
> > > It's misc/usbtest.c
> >
> > that'll do what you ask it to do with the userspace tool testusb. Are
> > you trying to pass a size of 0 ?
> >
> No, I just ran "testusb -t10" which called test_ctrl_queue().
> In this function, sub-case 8 passed a parameter @len as 0 to
> simple_alloc_urb(), and then it tried to allocate a 0-length buffer.

That should be easy enough to fix. Just skip the calls that allocate
the buffer if the length would be 0.

Alan Stern

2016-04-26 23:11:18

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH] usb: core: buffer: avoid NULL pointer dereferrence

On Fri, Apr 08, 2016 at 11:21:05AM -0400, Alan Stern wrote:
> On Fri, 8 Apr 2016, Greg Kroah-Hartman wrote:
>
> > On Fri, Apr 08, 2016 at 05:08:03PM +0800, Chunfeng Yun wrote:
> > > NULL pointer dereferrence will happen when class driver
> > > wants to allocate zero length buffer and pool_max[0]
> > > can't be used, so skip reserved pool in this case.
> >
> > Why would a driver want to allocate a 0 length buffer? What driver does
> > this?
> >
> > Shouldn't we fix that issue instead?
>
> And even if a driver does want to allocate a 0-length buffer, shouldn't
> the function simply return early instead of running through all its
> calculations?

Yes it should, Chunfeng, can you fix this patch to do that please.

thanks,

greg k-h

2016-04-27 01:32:20

by Chunfeng Yun

[permalink] [raw]
Subject: Re: [PATCH] usb: core: buffer: avoid NULL pointer dereferrence

Hi,

On Tue, 2016-04-26 at 16:11 -0700, Greg Kroah-Hartman wrote:
> On Fri, Apr 08, 2016 at 11:21:05AM -0400, Alan Stern wrote:
> > On Fri, 8 Apr 2016, Greg Kroah-Hartman wrote:
> >
> > > On Fri, Apr 08, 2016 at 05:08:03PM +0800, Chunfeng Yun wrote:
> > > > NULL pointer dereferrence will happen when class driver
> > > > wants to allocate zero length buffer and pool_max[0]
> > > > can't be used, so skip reserved pool in this case.
> > >
> > > Why would a driver want to allocate a 0 length buffer? What driver does
> > > this?
> > >
> > > Shouldn't we fix that issue instead?
> >
> > And even if a driver does want to allocate a 0-length buffer, shouldn't
> > the function simply return early instead of running through all its
> > calculations?
>
> Yes it should, Chunfeng, can you fix this patch to do that please.
>
Ok, I will fix it later

> thanks,
>
> greg k-h