diff mbox series

[v2] usb: gadget: composite: Fix null pointer exception

Message ID 20211105104840.159533-1-huqihang@oppo.com (mailing list archive)
State New, archived
Headers show
Series [v2] usb: gadget: composite: Fix null pointer exception | expand

Commit Message

Qihang Hu Nov. 5, 2021, 10:48 a.m. UTC 
In the config_ep_by_speed_and_alt function, select the corresponding
descriptor through g->speed, but the function driver may not
support the corresponding speed. So, we need to check whether the
function driver provides the corresponding speed descriptor when
selecting the descriptor.

[  237.708146]  android_work: sent uevent USB_STATE=CONNECTED
[  237.712464]  kconfigfs-gadget gadget: super-speed config #1: b
[  237.712487]  kUnable to handle kernel NULL pointer dereference at virtual address 0000000000000000
[  237.712493]  kMem abort info:
[  237.712498]  k  ESR = 0x96000006
[  237.712504]  k  EC = 0x25: DABT (current EL), IL = 32 bits
[  237.712510]  k  SET = 0, FnV = 0
[  237.712515]  k  EA = 0, S1PTW = 0
[  237.712520]  kData abort info:
[  237.712525]  k  ISV = 0, ISS = 0x00000006
[  237.712530]  k  CM = 0, WnR = 0
[  237.712536]  kuser pgtable: 4k pages, 39-bit VAs, pgdp=000000020ef29000
[  237.712541]  k[0000000000000000] pgd=000000020ef2a003, pud=000000020ef2a003, pmd=0000000000000000
[  237.712554]  kInternal error: Oops: 96000006 [#1] PREEMPT SMP
[  237.722067]  kSkip md ftrace buffer dump for: 0x1609e0
[  237.787037]  kWorkqueue: dwc_wq dwc3_bh_work.cfi_jt
[  237.854922]  kpstate: 60c00085 (nZCv daIf +PAN +UAO)
[  237.863165]  kpc : config_ep_by_speed_and_alt+0x90/0x308
[  237.871766]  klr : audio_set_alt+0x54/0x78
[  237.879108]  ksp : ffffffc0104839e0

Signed-off-by: Qihang Hu <huqihang@oppo.com>
---
Changes in v2:
-Add warning message
---
 drivers/usb/gadget/composite.c | 40 +++++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 13 deletions(-)

Comments

Greg Kroah-Hartman Nov. 5, 2021, 12:44 p.m. UTC | #1 
On Fri, Nov 05, 2021 at 06:48:40PM +0800, Qihang Hu wrote:
> In the config_ep_by_speed_and_alt function, select the corresponding
> descriptor through g->speed, but the function driver may not
> support the corresponding speed. So, we need to check whether the
> function driver provides the corresponding speed descriptor when
> selecting the descriptor.
> 
> [  237.708146]  android_work: sent uevent USB_STATE=CONNECTED
> [  237.712464]  kconfigfs-gadget gadget: super-speed config #1: b
> [  237.712487]  kUnable to handle kernel NULL pointer dereference at virtual address 0000000000000000

So this is an invalid driver causing this problem?  Or can this be
triggered by userspace?

> [  237.712493]  kMem abort info:
> [  237.712498]  k  ESR = 0x96000006
> [  237.712504]  k  EC = 0x25: DABT (current EL), IL = 32 bits
> [  237.712510]  k  SET = 0, FnV = 0
> [  237.712515]  k  EA = 0, S1PTW = 0
> [  237.712520]  kData abort info:
> [  237.712525]  k  ISV = 0, ISS = 0x00000006
> [  237.712530]  k  CM = 0, WnR = 0
> [  237.712536]  kuser pgtable: 4k pages, 39-bit VAs, pgdp=000000020ef29000
> [  237.712541]  k[0000000000000000] pgd=000000020ef2a003, pud=000000020ef2a003, pmd=0000000000000000
> [  237.712554]  kInternal error: Oops: 96000006 [#1] PREEMPT SMP
> [  237.722067]  kSkip md ftrace buffer dump for: 0x1609e0
> [  237.787037]  kWorkqueue: dwc_wq dwc3_bh_work.cfi_jt
> [  237.854922]  kpstate: 60c00085 (nZCv daIf +PAN +UAO)
> [  237.863165]  kpc : config_ep_by_speed_and_alt+0x90/0x308
> [  237.871766]  klr : audio_set_alt+0x54/0x78
> [  237.879108]  ksp : ffffffc0104839e0
> 
> Signed-off-by: Qihang Hu <huqihang@oppo.com>

What commit id does this fix?

> ---
> Changes in v2:
> -Add warning message
> ---
>  drivers/usb/gadget/composite.c | 40 +++++++++++++++++++++++-----------
>  1 file changed, 27 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index 72a9797dbbae..746b34cf0310 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -160,6 +160,9 @@ int config_ep_by_speed_and_alt(struct usb_gadget *g,
>  
>  	struct usb_descriptor_header **d_spd; /* cursor for speed desc */
>  

Why the blank line here?

> +	struct usb_composite_dev *cdev;
> +	int incomplete_desc = 0;

Shouldn't this be a bool?


thanks,

greg k-h
Qihang Hu Nov. 8, 2021, 12:42 p.m. UTC | #2 
> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Friday, November 5, 2021 8:45 PM
> To: 胡启航(Nick Hu) <huqihang@oppo.com>
> Cc: balbi@kernel.org; peter.chen@kernel.org; linux-usb@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2] usb: gadget: composite: Fix null pointer exception
> 
> On Fri, Nov 05, 2021 at 06:48:40PM +0800, Qihang Hu wrote:
> > In the config_ep_by_speed_and_alt function, select the corresponding
> > descriptor through g->speed, but the function driver may not
> > support the corresponding speed. So, we need to check whether the
> > function driver provides the corresponding speed descriptor when
> > selecting the descriptor.
> >
> > [  237.708146]  android_work: sent uevent USB_STATE=CONNECTED
> > [  237.712464]  kconfigfs-gadget gadget: super-speed config #1: b
> > [  237.712487]  kUnable to handle kernel NULL pointer dereference at
> virtual address 0000000000000000
> 
> So this is an invalid driver causing this problem?  Or can this be
> triggered by userspace?

Yes, if the kernel is loaded with an interface driver that does not support all
speeds, this problem can be triggered in userspace.

> 
> > [  237.712493]  kMem abort info:
> > [  237.712498]  k  ESR = 0x96000006
> > [  237.712504]  k  EC = 0x25: DABT (current EL), IL = 32 bits
> > [  237.712510]  k  SET = 0, FnV = 0
> > [  237.712515]  k  EA = 0, S1PTW = 0
> > [  237.712520]  kData abort info:
> > [  237.712525]  k  ISV = 0, ISS = 0x00000006
> > [  237.712530]  k  CM = 0, WnR = 0
> > [  237.712536]  kuser pgtable: 4k pages, 39-bit VAs,
> pgdp=000000020ef29000
> > [  237.712541]  k[0000000000000000] pgd=000000020ef2a003,
> pud=000000020ef2a003, pmd=0000000000000000
> > [  237.712554]  kInternal error: Oops: 96000006 [#1] PREEMPT SMP
> > [  237.722067]  kSkip md ftrace buffer dump for: 0x1609e0
> > [  237.787037]  kWorkqueue: dwc_wq dwc3_bh_work.cfi_jt
> > [  237.854922]  kpstate: 60c00085 (nZCv daIf +PAN +UAO)
> > [  237.863165]  kpc : config_ep_by_speed_and_alt+0x90/0x308
> > [  237.871766]  klr : audio_set_alt+0x54/0x78
> > [  237.879108]  ksp : ffffffc0104839e0
> >
> > Signed-off-by: Qihang Hu <huqihang@oppo.com>
> 
> What commit id does this fix?

I have not submitted a BUG.
I will study and submit a BUG as soon as possible.

> 
> > ---
> > Changes in v2:
> > -Add warning message
> > ---
> >  drivers/usb/gadget/composite.c | 40 +++++++++++++++++++++++-----------
> >  1 file changed, 27 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> > index 72a9797dbbae..746b34cf0310 100644
> > --- a/drivers/usb/gadget/composite.c
> > +++ b/drivers/usb/gadget/composite.c
> > @@ -160,6 +160,9 @@ int config_ep_by_speed_and_alt(struct usb_gadget
> *g,
> >
> >  	struct usb_descriptor_header **d_spd; /* cursor for speed desc */
> >
> 
> Why the blank line here?
> 
> > +	struct usb_composite_dev *cdev;
> > +	int incomplete_desc = 0;
> 
> Shouldn't this be a bool?
> 

The bool variable should indeed be used, I will fix this problem in V3

Thanks
Greg Kroah-Hartman Nov. 8, 2021, 12:55 p.m. UTC | #3 
On Mon, Nov 08, 2021 at 12:42:24PM +0000, 胡启航(Nick Hu) wrote:
> > -----Original Message-----
> > From: Greg KH <gregkh@linuxfoundation.org>
> > Sent: Friday, November 5, 2021 8:45 PM
> > To: 胡启航(Nick Hu) <huqihang@oppo.com>
> > Cc: balbi@kernel.org; peter.chen@kernel.org; linux-usb@vger.kernel.org;
> > linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH v2] usb: gadget: composite: Fix null pointer exception
> > 
> > On Fri, Nov 05, 2021 at 06:48:40PM +0800, Qihang Hu wrote:
> > > In the config_ep_by_speed_and_alt function, select the corresponding
> > > descriptor through g->speed, but the function driver may not
> > > support the corresponding speed. So, we need to check whether the
> > > function driver provides the corresponding speed descriptor when
> > > selecting the descriptor.
> > >
> > > [  237.708146]  android_work: sent uevent USB_STATE=CONNECTED
> > > [  237.712464]  kconfigfs-gadget gadget: super-speed config #1: b
> > > [  237.712487]  kUnable to handle kernel NULL pointer dereference at
> > virtual address 0000000000000000
> > 
> > So this is an invalid driver causing this problem?  Or can this be
> > triggered by userspace?
> 
> Yes, if the kernel is loaded with an interface driver that does not support all
> speeds, this problem can be triggered in userspace.

What in-kernel driver does not support all speeds that can trigger this?

Why not fix the drivers?

> > > [  237.712493]  kMem abort info:
> > > [  237.712498]  k  ESR = 0x96000006
> > > [  237.712504]  k  EC = 0x25: DABT (current EL), IL = 32 bits
> > > [  237.712510]  k  SET = 0, FnV = 0
> > > [  237.712515]  k  EA = 0, S1PTW = 0
> > > [  237.712520]  kData abort info:
> > > [  237.712525]  k  ISV = 0, ISS = 0x00000006
> > > [  237.712530]  k  CM = 0, WnR = 0
> > > [  237.712536]  kuser pgtable: 4k pages, 39-bit VAs,
> > pgdp=000000020ef29000
> > > [  237.712541]  k[0000000000000000] pgd=000000020ef2a003,
> > pud=000000020ef2a003, pmd=0000000000000000
> > > [  237.712554]  kInternal error: Oops: 96000006 [#1] PREEMPT SMP
> > > [  237.722067]  kSkip md ftrace buffer dump for: 0x1609e0
> > > [  237.787037]  kWorkqueue: dwc_wq dwc3_bh_work.cfi_jt
> > > [  237.854922]  kpstate: 60c00085 (nZCv daIf +PAN +UAO)
> > > [  237.863165]  kpc : config_ep_by_speed_and_alt+0x90/0x308
> > > [  237.871766]  klr : audio_set_alt+0x54/0x78
> > > [  237.879108]  ksp : ffffffc0104839e0
> > >
> > > Signed-off-by: Qihang Hu <huqihang@oppo.com>
> > 
> > What commit id does this fix?
> 
> I have not submitted a BUG.
> I will study and submit a BUG as soon as possible.

I do not understand what you mean by this.  I am asking what commit
caused this problem so we can mark this one as fixing it.

thnaks,

greg k-h
Qihang Hu Nov. 8, 2021, 1:10 p.m. UTC | #4 
> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Monday, November 8, 2021 8:56 PM
> To: 胡启航(Nick Hu) <huqihang@oppo.com>
> Cc: balbi@kernel.org; peter.chen@kernel.org; linux-usb@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2] usb: gadget: composite: Fix null pointer exception
> 
> On Mon, Nov 08, 2021 at 12:42:24PM +0000, 胡启航(Nick Hu) wrote:
> > > -----Original Message-----
> > > From: Greg KH <gregkh@linuxfoundation.org>
> > > Sent: Friday, November 5, 2021 8:45 PM
> > > To: 胡启航(Nick Hu) <huqihang@oppo.com>
> > > Cc: balbi@kernel.org; peter.chen@kernel.org;
> > > linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH v2] usb: gadget: composite: Fix null pointer
> > > exception
> > >
> > > On Fri, Nov 05, 2021 at 06:48:40PM +0800, Qihang Hu wrote:
> > > > In the config_ep_by_speed_and_alt function, select the
> > > > corresponding descriptor through g->speed, but the function driver
> > > > may not support the corresponding speed. So, we need to check
> > > > whether the function driver provides the corresponding speed
> > > > descriptor when selecting the descriptor.
> > > >
> > > > [  237.708146]  android_work: sent uevent USB_STATE=CONNECTED [
> > > > 237.712464]  kconfigfs-gadget gadget: super-speed config #1: b [
> > > > 237.712487]  kUnable to handle kernel NULL pointer dereference at
> > > virtual address 0000000000000000
> > >
> > > So this is an invalid driver causing this problem?  Or can this be
> > > triggered by userspace?
> >
> > Yes, if the kernel is loaded with an interface driver that does not
> > support all speeds, this problem can be triggered in userspace.
> 
> What in-kernel driver does not support all speeds that can trigger this?
> 
> Why not fix the drivers?

This problem is caused by android f_audio_source.c
I think the core driver code should be improved to make it less susceptible to function-driven influence. 
Of course, repairing the function driver is the fundamental solution to the problem.

> > > > [  237.712493]  kMem abort info:
> > > > [  237.712498]  k  ESR = 0x96000006 [  237.712504]  k  EC = 0x25:
> > > > DABT (current EL), IL = 32 bits [  237.712510]  k  SET = 0, FnV =
> > > > 0 [  237.712515]  k  EA = 0, S1PTW = 0 [  237.712520]  kData abort
> > > > info:
> > > > [  237.712525]  k  ISV = 0, ISS = 0x00000006 [  237.712530]  k  CM
> > > > = 0, WnR = 0 [  237.712536]  kuser pgtable: 4k pages, 39-bit VAs,
> > > pgdp=000000020ef29000
> > > > [  237.712541]  k[0000000000000000] pgd=000000020ef2a003,
> > > pud=000000020ef2a003, pmd=0000000000000000
> > > > [  237.712554]  kInternal error: Oops: 96000006 [#1] PREEMPT SMP [
> > > > 237.722067]  kSkip md ftrace buffer dump for: 0x1609e0 [
> > > > 237.787037]  kWorkqueue: dwc_wq dwc3_bh_work.cfi_jt
> [  237.854922]
> > > > kpstate: 60c00085 (nZCv daIf +PAN +UAO) [  237.863165]  kpc :
> > > > config_ep_by_speed_and_alt+0x90/0x308
> > > > [  237.871766]  klr : audio_set_alt+0x54/0x78 [  237.879108]  ksp
> > > > : ffffffc0104839e0
> > > >
> > > > Signed-off-by: Qihang Hu <huqihang@oppo.com>
> > >
> > > What commit id does this fix?
> >
> > I have not submitted a BUG.
> > I will study and submit a BUG as soon as possible.
> 
> I do not understand what you mean by this.  I am asking what commit caused
> this problem so we can mark this one as fixing it.
> 

This is a problem with Android's driver code.
The kernel does not include it.

Thanks
Greg Kroah-Hartman Nov. 8, 2021, 1:41 p.m. UTC | #5 
On Mon, Nov 08, 2021 at 01:10:14PM +0000, 胡启航(Nick Hu) wrote:
> > -----Original Message-----
> > From: Greg KH <gregkh@linuxfoundation.org>
> > Sent: Monday, November 8, 2021 8:56 PM
> > To: 胡启航(Nick Hu) <huqihang@oppo.com>
> > Cc: balbi@kernel.org; peter.chen@kernel.org; linux-usb@vger.kernel.org;
> > linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH v2] usb: gadget: composite: Fix null pointer exception
> > 
> > On Mon, Nov 08, 2021 at 12:42:24PM +0000, 胡启航(Nick Hu) wrote:
> > > > -----Original Message-----
> > > > From: Greg KH <gregkh@linuxfoundation.org>
> > > > Sent: Friday, November 5, 2021 8:45 PM
> > > > To: 胡启航(Nick Hu) <huqihang@oppo.com>
> > > > Cc: balbi@kernel.org; peter.chen@kernel.org;
> > > > linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org
> > > > Subject: Re: [PATCH v2] usb: gadget: composite: Fix null pointer
> > > > exception
> > > >
> > > > On Fri, Nov 05, 2021 at 06:48:40PM +0800, Qihang Hu wrote:
> > > > > In the config_ep_by_speed_and_alt function, select the
> > > > > corresponding descriptor through g->speed, but the function driver
> > > > > may not support the corresponding speed. So, we need to check
> > > > > whether the function driver provides the corresponding speed
> > > > > descriptor when selecting the descriptor.
> > > > >
> > > > > [  237.708146]  android_work: sent uevent USB_STATE=CONNECTED [
> > > > > 237.712464]  kconfigfs-gadget gadget: super-speed config #1: b [
> > > > > 237.712487]  kUnable to handle kernel NULL pointer dereference at
> > > > virtual address 0000000000000000
> > > >
> > > > So this is an invalid driver causing this problem?  Or can this be
> > > > triggered by userspace?
> > >
> > > Yes, if the kernel is loaded with an interface driver that does not
> > > support all speeds, this problem can be triggered in userspace.
> > 
> > What in-kernel driver does not support all speeds that can trigger this?
> > 
> > Why not fix the drivers?
> 
> This problem is caused by android f_audio_source.c

That file should not be used anymore, please use the proper USB gadget
code for Android instead.  There is a reason that code was never merged
upstream.

> I think the core driver code should be improved to make it less susceptible to function-driven influence. 
> Of course, repairing the function driver is the fundamental solution to the problem.

There is no need to use that code at all, so you are trying to fix an
external driver's bug with a core kernel change.  That's not the best
thing to do here :)

> > > I have not submitted a BUG.
> > > I will study and submit a BUG as soon as possible.
> > 
> > I do not understand what you mean by this.  I am asking what commit caused
> > this problem so we can mark this one as fixing it.
> > 
> 
> This is a problem with Android's driver code.
> The kernel does not include it.

Great, then fix the android driver's code and then do not use it anymore :)

thanks,

greg k-h
diff mbox series

Patch

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 72a9797dbbae..746b34cf0310 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -160,6 +160,9 @@  int config_ep_by_speed_and_alt(struct usb_gadget *g,
 
 	struct usb_descriptor_header **d_spd; /* cursor for speed desc */
 
+	struct usb_composite_dev *cdev;
+	int incomplete_desc = 0;
+
 	if (!g || !f || !_ep)
 		return -EIO;
 
@@ -167,28 +170,43 @@  int config_ep_by_speed_and_alt(struct usb_gadget *g,
 	switch (g->speed) {
 	case USB_SPEED_SUPER_PLUS:
 		if (gadget_is_superspeed_plus(g)) {
-			speed_desc = f->ssp_descriptors;
-			want_comp_desc = 1;
-			break;
+			if (f->ssp_descriptors) {
+				speed_desc = f->ssp_descriptors;
+				want_comp_desc = 1;
+				break;
+			}
+			incomplete_desc = 1;
 		}
 		fallthrough;
 	case USB_SPEED_SUPER:
 		if (gadget_is_superspeed(g)) {
-			speed_desc = f->ss_descriptors;
-			want_comp_desc = 1;
-			break;
+			if (f->ss_descriptors) {
+				speed_desc = f->ss_descriptors;
+				want_comp_desc = 1;
+				break;
+			}
+			incomplete_desc = 1;
 		}
 		fallthrough;
 	case USB_SPEED_HIGH:
 		if (gadget_is_dualspeed(g)) {
-			speed_desc = f->hs_descriptors;
-			break;
+			if (f->hs_descriptors) {
+				speed_desc = f->hs_descriptors;
+				break;
+			}
+			incomplete_desc = 1;
 		}
 		fallthrough;
 	default:
 		speed_desc = f->fs_descriptors;
 	}
 
+	cdev = get_gadget_data(g);
+	if (incomplete_desc != 0)
+		WARNING(cdev,
+			"%s doesn't hold the descriptors for current speed\n",
+			f->name);
+
 	/* find correct alternate setting descriptor */
 	for_each_desc(speed_desc, d_spd, USB_DT_INTERFACE) {
 		int_desc = (struct usb_interface_descriptor *)*d_spd;
@@ -244,12 +262,8 @@  int config_ep_by_speed_and_alt(struct usb_gadget *g,
 			_ep->maxburst = comp_desc->bMaxBurst + 1;
 			break;
 		default:
-			if (comp_desc->bMaxBurst != 0) {
-				struct usb_composite_dev *cdev;
-
-				cdev = get_gadget_data(g);
+			if (comp_desc->bMaxBurst != 0)
 				ERROR(cdev, "ep0 bMaxBurst must be 0\n");
-			}
 			_ep->maxburst = 1;
 			break;
 		}