diff mbox

usb: chipidea: udc: fix NULL pointer dereference if udc_start failed

Message ID 20170424123551.2465-1-jszhang@marvell.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jisheng Zhang April 24, 2017, 12:35 p.m. UTC
Fix below NULL pointer dereference. we set ci->roles[CI_ROLE_GADGET]
too early in ci_hdrc_gadget_init(), if udc_start() fails due to some
reason, the ci->roles[CI_ROLE_GADGET] check in  ci_hdrc_gadget_destroy
can't protect us.

We fix this issue by only setting ci->roles[CI_ROLE_GADGET] if
udc_start() succeed.

[    1.398550] Unable to handle kernel NULL pointer dereference at
virtual address 00000000
...
[    1.448600] PC is at dma_pool_free+0xb8/0xf0
[    1.453012] LR is at dma_pool_free+0x28/0xf0
[    2.113369] [<ffffff80081817d8>] dma_pool_free+0xb8/0xf0
[    2.118857] [<ffffff800841209c>] destroy_eps+0x4c/0x68
[    2.124165] [<ffffff8008413770>] ci_hdrc_gadget_destroy+0x28/0x50
[    2.130461] [<ffffff800840fa30>] ci_hdrc_probe+0x588/0x7e8
[    2.136129] [<ffffff8008380fb8>] platform_drv_probe+0x50/0xb8
[    2.142066] [<ffffff800837f494>] driver_probe_device+0x1fc/0x2a8
[    2.148270] [<ffffff800837f68c>] __device_attach_driver+0x9c/0xf8
[    2.154563] [<ffffff800837d570>] bus_for_each_drv+0x58/0x98
[    2.160317] [<ffffff800837f174>] __device_attach+0xc4/0x138
[    2.166072] [<ffffff800837f738>] device_initial_probe+0x10/0x18
[    2.172185] [<ffffff800837e58c>] bus_probe_device+0x94/0xa0
[    2.177940] [<ffffff800837c560>] device_add+0x3f0/0x560
[    2.183337] [<ffffff8008380d20>] platform_device_add+0x180/0x240
[    2.189541] [<ffffff800840f0e8>] ci_hdrc_add_device+0x440/0x4f8
[    2.195654] [<ffffff8008414194>] ci_hdrc_usb2_probe+0x13c/0x2d8
[    2.201769] [<ffffff8008380fb8>] platform_drv_probe+0x50/0xb8
[    2.207705] [<ffffff800837f494>] driver_probe_device+0x1fc/0x2a8
[    2.213910] [<ffffff800837f5ec>] __driver_attach+0xac/0xb0
[    2.219575] [<ffffff800837d4b0>] bus_for_each_dev+0x60/0xa0
[    2.225329] [<ffffff800837ec80>] driver_attach+0x20/0x28
[    2.230816] [<ffffff800837e880>] bus_add_driver+0x1d0/0x238
[    2.236571] [<ffffff800837fdb0>] driver_register+0x60/0xf8
[    2.242237] [<ffffff8008380ef4>] __platform_driver_register+0x44/0x50
[    2.248891] [<ffffff80086fd440>] ci_hdrc_usb2_driver_init+0x18/0x20
[    2.255365] [<ffffff8008082950>] do_one_initcall+0x38/0x128
[    2.261121] [<ffffff80086e0d00>] kernel_init_freeable+0x1ac/0x250
[    2.267414] [<ffffff800852f0b8>] kernel_init+0x10/0x100
[    2.272810] [<ffffff8008082680>] ret_from_fork+0x10/0x50

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/usb/chipidea/udc.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Peter Chen April 25, 2017, 8:29 a.m. UTC | #1
On Mon, Apr 24, 2017 at 12:35:51PM +0000, Jisheng Zhang wrote:
> Fix below NULL pointer dereference. we set ci->roles[CI_ROLE_GADGET]
> too early in ci_hdrc_gadget_init(), if udc_start() fails due to some
> reason, the ci->roles[CI_ROLE_GADGET] check in  ci_hdrc_gadget_destroy
> can't protect us.
> 
> We fix this issue by only setting ci->roles[CI_ROLE_GADGET] if
> udc_start() succeed.
> 
> [    1.398550] Unable to handle kernel NULL pointer dereference at
> virtual address 00000000
> ...
> [    1.448600] PC is at dma_pool_free+0xb8/0xf0
> [    1.453012] LR is at dma_pool_free+0x28/0xf0
> [    2.113369] [<ffffff80081817d8>] dma_pool_free+0xb8/0xf0
> [    2.118857] [<ffffff800841209c>] destroy_eps+0x4c/0x68
> [    2.124165] [<ffffff8008413770>] ci_hdrc_gadget_destroy+0x28/0x50
> [    2.130461] [<ffffff800840fa30>] ci_hdrc_probe+0x588/0x7e8
> [    2.136129] [<ffffff8008380fb8>] platform_drv_probe+0x50/0xb8
> [    2.142066] [<ffffff800837f494>] driver_probe_device+0x1fc/0x2a8
> [    2.148270] [<ffffff800837f68c>] __device_attach_driver+0x9c/0xf8
> [    2.154563] [<ffffff800837d570>] bus_for_each_drv+0x58/0x98
> [    2.160317] [<ffffff800837f174>] __device_attach+0xc4/0x138
> [    2.166072] [<ffffff800837f738>] device_initial_probe+0x10/0x18
> [    2.172185] [<ffffff800837e58c>] bus_probe_device+0x94/0xa0
> [    2.177940] [<ffffff800837c560>] device_add+0x3f0/0x560
> [    2.183337] [<ffffff8008380d20>] platform_device_add+0x180/0x240
> [    2.189541] [<ffffff800840f0e8>] ci_hdrc_add_device+0x440/0x4f8
> [    2.195654] [<ffffff8008414194>] ci_hdrc_usb2_probe+0x13c/0x2d8
> [    2.201769] [<ffffff8008380fb8>] platform_drv_probe+0x50/0xb8
> [    2.207705] [<ffffff800837f494>] driver_probe_device+0x1fc/0x2a8
> [    2.213910] [<ffffff800837f5ec>] __driver_attach+0xac/0xb0
> [    2.219575] [<ffffff800837d4b0>] bus_for_each_dev+0x60/0xa0
> [    2.225329] [<ffffff800837ec80>] driver_attach+0x20/0x28
> [    2.230816] [<ffffff800837e880>] bus_add_driver+0x1d0/0x238
> [    2.236571] [<ffffff800837fdb0>] driver_register+0x60/0xf8
> [    2.242237] [<ffffff8008380ef4>] __platform_driver_register+0x44/0x50
> [    2.248891] [<ffffff80086fd440>] ci_hdrc_usb2_driver_init+0x18/0x20
> [    2.255365] [<ffffff8008082950>] do_one_initcall+0x38/0x128
> [    2.261121] [<ffffff80086e0d00>] kernel_init_freeable+0x1ac/0x250
> [    2.267414] [<ffffff800852f0b8>] kernel_init+0x10/0x100
> [    2.272810] [<ffffff8008082680>] ret_from_fork+0x10/0x50
> 
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>  drivers/usb/chipidea/udc.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> index f88e9157fad0..60a786c87c06 100644
> --- a/drivers/usb/chipidea/udc.c
> +++ b/drivers/usb/chipidea/udc.c
> @@ -1984,6 +1984,7 @@ static void udc_id_switch_for_host(struct ci_hdrc *ci)
>  int ci_hdrc_gadget_init(struct ci_hdrc *ci)
>  {
>  	struct ci_role_driver *rdrv;
> +	int ret;
>  
>  	if (!hw_read(ci, CAP_DCCPARAMS, DCCPARAMS_DC))
>  		return -ENXIO;
> @@ -1996,7 +1997,10 @@ int ci_hdrc_gadget_init(struct ci_hdrc *ci)
>  	rdrv->stop	= udc_id_switch_for_host;
>  	rdrv->irq	= udc_irq;
>  	rdrv->name	= "gadget";
> -	ci->roles[CI_ROLE_GADGET] = rdrv;
>  
> -	return udc_start(ci);
> +	ret = udc_start(ci);
> +	if (!ret)
> +		ci->roles[CI_ROLE_GADGET] = rdrv;
> +
> +	return ret;
>  }
> -- 

Thanks for fixing it. In fact, we'd better return failure if
ret && ret != -ENXIO at probe, it stands for initialization
for host or gadget has failed.
Jisheng Zhang April 25, 2017, 9:11 a.m. UTC | #2
Hi Peter,

On Tue, 25 Apr 2017 16:29:48 +0800 Peter Chen wrote:

> On Mon, Apr 24, 2017 at 12:35:51PM +0000, Jisheng Zhang wrote:
> > Fix below NULL pointer dereference. we set ci->roles[CI_ROLE_GADGET]
> > too early in ci_hdrc_gadget_init(), if udc_start() fails due to some
> > reason, the ci->roles[CI_ROLE_GADGET] check in  ci_hdrc_gadget_destroy
> > can't protect us.
> > 
> > We fix this issue by only setting ci->roles[CI_ROLE_GADGET] if
> > udc_start() succeed.
> > 
> > [    1.398550] Unable to handle kernel NULL pointer dereference at
> > virtual address 00000000
> > ...
> > [    1.448600] PC is at dma_pool_free+0xb8/0xf0
> > [    1.453012] LR is at dma_pool_free+0x28/0xf0
> > [    2.113369] [<ffffff80081817d8>] dma_pool_free+0xb8/0xf0
> > [    2.118857] [<ffffff800841209c>] destroy_eps+0x4c/0x68
> > [    2.124165] [<ffffff8008413770>] ci_hdrc_gadget_destroy+0x28/0x50
> > [    2.130461] [<ffffff800840fa30>] ci_hdrc_probe+0x588/0x7e8
> > [    2.136129] [<ffffff8008380fb8>] platform_drv_probe+0x50/0xb8
> > [    2.142066] [<ffffff800837f494>] driver_probe_device+0x1fc/0x2a8
> > [    2.148270] [<ffffff800837f68c>] __device_attach_driver+0x9c/0xf8
> > [    2.154563] [<ffffff800837d570>] bus_for_each_drv+0x58/0x98
> > [    2.160317] [<ffffff800837f174>] __device_attach+0xc4/0x138
> > [    2.166072] [<ffffff800837f738>] device_initial_probe+0x10/0x18
> > [    2.172185] [<ffffff800837e58c>] bus_probe_device+0x94/0xa0
> > [    2.177940] [<ffffff800837c560>] device_add+0x3f0/0x560
> > [    2.183337] [<ffffff8008380d20>] platform_device_add+0x180/0x240
> > [    2.189541] [<ffffff800840f0e8>] ci_hdrc_add_device+0x440/0x4f8
> > [    2.195654] [<ffffff8008414194>] ci_hdrc_usb2_probe+0x13c/0x2d8
> > [    2.201769] [<ffffff8008380fb8>] platform_drv_probe+0x50/0xb8
> > [    2.207705] [<ffffff800837f494>] driver_probe_device+0x1fc/0x2a8
> > [    2.213910] [<ffffff800837f5ec>] __driver_attach+0xac/0xb0
> > [    2.219575] [<ffffff800837d4b0>] bus_for_each_dev+0x60/0xa0
> > [    2.225329] [<ffffff800837ec80>] driver_attach+0x20/0x28
> > [    2.230816] [<ffffff800837e880>] bus_add_driver+0x1d0/0x238
> > [    2.236571] [<ffffff800837fdb0>] driver_register+0x60/0xf8
> > [    2.242237] [<ffffff8008380ef4>] __platform_driver_register+0x44/0x50
> > [    2.248891] [<ffffff80086fd440>] ci_hdrc_usb2_driver_init+0x18/0x20
> > [    2.255365] [<ffffff8008082950>] do_one_initcall+0x38/0x128
> > [    2.261121] [<ffffff80086e0d00>] kernel_init_freeable+0x1ac/0x250
> > [    2.267414] [<ffffff800852f0b8>] kernel_init+0x10/0x100
> > [    2.272810] [<ffffff8008082680>] ret_from_fork+0x10/0x50
> > 
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > ---
> >  drivers/usb/chipidea/udc.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> > index f88e9157fad0..60a786c87c06 100644
> > --- a/drivers/usb/chipidea/udc.c
> > +++ b/drivers/usb/chipidea/udc.c
> > @@ -1984,6 +1984,7 @@ static void udc_id_switch_for_host(struct ci_hdrc *ci)
> >  int ci_hdrc_gadget_init(struct ci_hdrc *ci)
> >  {
> >  	struct ci_role_driver *rdrv;
> > +	int ret;
> >  
> >  	if (!hw_read(ci, CAP_DCCPARAMS, DCCPARAMS_DC))
> >  		return -ENXIO;
> > @@ -1996,7 +1997,10 @@ int ci_hdrc_gadget_init(struct ci_hdrc *ci)
> >  	rdrv->stop	= udc_id_switch_for_host;
> >  	rdrv->irq	= udc_irq;
> >  	rdrv->name	= "gadget";
> > -	ci->roles[CI_ROLE_GADGET] = rdrv;
> >  
> > -	return udc_start(ci);
> > +	ret = udc_start(ci);
> > +	if (!ret)
> > +		ci->roles[CI_ROLE_GADGET] = rdrv;
> > +
> > +	return ret;
> >  }
> > --   
> 
> Thanks for fixing it. In fact, we'd better return failure if
> ret && ret != -ENXIO at probe, it stands for initialization
> for host or gadget has failed.
> 

I got your meaning. I'll cook v2. I don't have preference, since either
one can fix the issue.

Thanks for your suggestion,
Jisheng
Peter Chen April 25, 2017, 9:20 a.m. UTC | #3
>> > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
>> > index f88e9157fad0..60a786c87c06 100644
>> > --- a/drivers/usb/chipidea/udc.c
>> > +++ b/drivers/usb/chipidea/udc.c
>> > @@ -1984,6 +1984,7 @@ static void udc_id_switch_for_host(struct
>> > ci_hdrc *ci)  int ci_hdrc_gadget_init(struct ci_hdrc *ci)  {
>> >  	struct ci_role_driver *rdrv;
>> > +	int ret;
>> >
>> >  	if (!hw_read(ci, CAP_DCCPARAMS, DCCPARAMS_DC))
>> >  		return -ENXIO;
>> > @@ -1996,7 +1997,10 @@ int ci_hdrc_gadget_init(struct ci_hdrc *ci)
>> >  	rdrv->stop	= udc_id_switch_for_host;
>> >  	rdrv->irq	= udc_irq;
>> >  	rdrv->name	= "gadget";
>> > -	ci->roles[CI_ROLE_GADGET] = rdrv;
>> >
>> > -	return udc_start(ci);
>> > +	ret = udc_start(ci);
>> > +	if (!ret)
>> > +		ci->roles[CI_ROLE_GADGET] = rdrv;
>> > +
>> > +	return ret;
>> >  }
>> > --
>>
>> Thanks for fixing it. In fact, we'd better return failure if ret &&
>> ret != -ENXIO at probe, it stands for initialization for host or
>> gadget has failed.
>>
>
>I got your meaning. I'll cook v2. I don't have preference, since either one can fix the
>issue.
>

Both are needed, you don't need to send this one again. Only a new one, thanks.

Peter
Stefan Wahren April 25, 2017, 3:21 p.m. UTC | #4
Am 25.04.2017 um 11:20 schrieb Peter Chen:
>  
>>>> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
>>>> index f88e9157fad0..60a786c87c06 100644
>>>> --- a/drivers/usb/chipidea/udc.c
>>>> +++ b/drivers/usb/chipidea/udc.c
>>>> @@ -1984,6 +1984,7 @@ static void udc_id_switch_for_host(struct
>>>> ci_hdrc *ci)  int ci_hdrc_gadget_init(struct ci_hdrc *ci)  {
>>>>  	struct ci_role_driver *rdrv;
>>>> +	int ret;
>>>>
>>>>  	if (!hw_read(ci, CAP_DCCPARAMS, DCCPARAMS_DC))
>>>>  		return -ENXIO;
>>>> @@ -1996,7 +1997,10 @@ int ci_hdrc_gadget_init(struct ci_hdrc *ci)
>>>>  	rdrv->stop	= udc_id_switch_for_host;
>>>>  	rdrv->irq	= udc_irq;
>>>>  	rdrv->name	= "gadget";
>>>> -	ci->roles[CI_ROLE_GADGET] = rdrv;
>>>>
>>>> -	return udc_start(ci);
>>>> +	ret = udc_start(ci);
>>>> +	if (!ret)
>>>> +		ci->roles[CI_ROLE_GADGET] = rdrv;
>>>> +
>>>> +	return ret;
>>>>  }
>>>> --
>>> Thanks for fixing it. In fact, we'd better return failure if ret &&
>>> ret != -ENXIO at probe, it stands for initialization for host or
>>> gadget has failed.
>>>
>> I got your meaning. I'll cook v2. I don't have preference, since either one can fix the
>> issue.
>>
> Both are needed, you don't need to send this one again. Only a new one, thanks.

I'm not sure how easy it is to reproduce the issue.

Shouldn't make a Fixes tag sense at least?

>
> Peter
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Jisheng Zhang April 26, 2017, 8:25 a.m. UTC | #5
On Tue, 25 Apr 2017 17:21:59 +0200
Stefan Wahren <stefan.wahren@i2se.com> wrote:

> Am 25.04.2017 um 11:20 schrieb Peter Chen:
> >    
> >>>> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> >>>> index f88e9157fad0..60a786c87c06 100644
> >>>> --- a/drivers/usb/chipidea/udc.c
> >>>> +++ b/drivers/usb/chipidea/udc.c
> >>>> @@ -1984,6 +1984,7 @@ static void udc_id_switch_for_host(struct
> >>>> ci_hdrc *ci)  int ci_hdrc_gadget_init(struct ci_hdrc *ci)  {
> >>>>  	struct ci_role_driver *rdrv;
> >>>> +	int ret;
> >>>>
> >>>>  	if (!hw_read(ci, CAP_DCCPARAMS, DCCPARAMS_DC))
> >>>>  		return -ENXIO;
> >>>> @@ -1996,7 +1997,10 @@ int ci_hdrc_gadget_init(struct ci_hdrc *ci)
> >>>>  	rdrv->stop	= udc_id_switch_for_host;
> >>>>  	rdrv->irq	= udc_irq;
> >>>>  	rdrv->name	= "gadget";
> >>>> -	ci->roles[CI_ROLE_GADGET] = rdrv;
> >>>>
> >>>> -	return udc_start(ci);
> >>>> +	ret = udc_start(ci);
> >>>> +	if (!ret)
> >>>> +		ci->roles[CI_ROLE_GADGET] = rdrv;
> >>>> +
> >>>> +	return ret;
> >>>>  }
> >>>> --  
> >>> Thanks for fixing it. In fact, we'd better return failure if ret &&
> >>> ret != -ENXIO at probe, it stands for initialization for host or
> >>> gadget has failed.
> >>>  
> >> I got your meaning. I'll cook v2. I don't have preference, since either one can fix the
> >> issue.
> >>  
> > Both are needed, you don't need to send this one again. Only a new one, thanks.  
> 
> I'm not sure how easy it is to reproduce the issue.

It's easy to reproduce it (100%) on arm64 platforms after commit
1dccb598df549 ("arm64: simplify dma_get_ops"). This commit could
make all dma related operations failed, then udc_start() would fail
with -ENOMEM.

On other platforms, it's not easy.

> 
> Shouldn't make a Fixes tag sense at least?

maybe 3f124d233e97 ("usb: chipidea: add role init and destroy APIs"
Peter Chen April 26, 2017, 9:05 a.m. UTC | #6
On Wed, Apr 26, 2017 at 04:25:26PM +0800, Jisheng Zhang wrote:
> On Tue, 25 Apr 2017 17:21:59 +0200
> Stefan Wahren <stefan.wahren@i2se.com> wrote:
> 
> > Am 25.04.2017 um 11:20 schrieb Peter Chen:
> > >    
> > >>>> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> > >>>> index f88e9157fad0..60a786c87c06 100644
> > >>>> --- a/drivers/usb/chipidea/udc.c
> > >>>> +++ b/drivers/usb/chipidea/udc.c
> > >>>> @@ -1984,6 +1984,7 @@ static void udc_id_switch_for_host(struct
> > >>>> ci_hdrc *ci)  int ci_hdrc_gadget_init(struct ci_hdrc *ci)  {
> > >>>>  	struct ci_role_driver *rdrv;
> > >>>> +	int ret;
> > >>>>
> > >>>>  	if (!hw_read(ci, CAP_DCCPARAMS, DCCPARAMS_DC))
> > >>>>  		return -ENXIO;
> > >>>> @@ -1996,7 +1997,10 @@ int ci_hdrc_gadget_init(struct ci_hdrc *ci)
> > >>>>  	rdrv->stop	= udc_id_switch_for_host;
> > >>>>  	rdrv->irq	= udc_irq;
> > >>>>  	rdrv->name	= "gadget";
> > >>>> -	ci->roles[CI_ROLE_GADGET] = rdrv;
> > >>>>
> > >>>> -	return udc_start(ci);
> > >>>> +	ret = udc_start(ci);
> > >>>> +	if (!ret)
> > >>>> +		ci->roles[CI_ROLE_GADGET] = rdrv;
> > >>>> +
> > >>>> +	return ret;
> > >>>>  }
> > >>>> --  
> > >>> Thanks for fixing it. In fact, we'd better return failure if ret &&
> > >>> ret != -ENXIO at probe, it stands for initialization for host or
> > >>> gadget has failed.
> > >>>  
> > >> I got your meaning. I'll cook v2. I don't have preference, since either one can fix the
> > >> issue.
> > >>  
> > > Both are needed, you don't need to send this one again. Only a new one, thanks.  
> > 
> > I'm not sure how easy it is to reproduce the issue.
> 
> It's easy to reproduce it (100%) on arm64 platforms after commit
> 1dccb598df549 ("arm64: simplify dma_get_ops"). This commit could
> make all dma related operations failed, then udc_start() would fail
> with -ENOMEM.
> 
> On other platforms, it's not easy.
> 
> > 
> > Shouldn't make a Fixes tag sense at least?
> 
> maybe 3f124d233e97 ("usb: chipidea: add role init and destroy APIs"

I will cc stable, thanks.
diff mbox

Patch

diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index f88e9157fad0..60a786c87c06 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -1984,6 +1984,7 @@  static void udc_id_switch_for_host(struct ci_hdrc *ci)
 int ci_hdrc_gadget_init(struct ci_hdrc *ci)
 {
 	struct ci_role_driver *rdrv;
+	int ret;
 
 	if (!hw_read(ci, CAP_DCCPARAMS, DCCPARAMS_DC))
 		return -ENXIO;
@@ -1996,7 +1997,10 @@  int ci_hdrc_gadget_init(struct ci_hdrc *ci)
 	rdrv->stop	= udc_id_switch_for_host;
 	rdrv->irq	= udc_irq;
 	rdrv->name	= "gadget";
-	ci->roles[CI_ROLE_GADGET] = rdrv;
 
-	return udc_start(ci);
+	ret = udc_start(ci);
+	if (!ret)
+		ci->roles[CI_ROLE_GADGET] = rdrv;
+
+	return ret;
 }