[PATCH v3] usb: gadget: u_serial: Fix the issue that gs_start_io crashed due to accessing null pointer

[PATCH v3] usb: gadget: u_serial: Fix the issue that gs_start_io crashed due to accessing null pointer

[胡连勤]

From: Lianqin Hu <hulianqin@vivo.com>

Considering that in some extreme cases,
when u_serial driver is accessed by multiple threads,
Thread A is executing the open operation and calling the gs_open,
Thread B is executing the disconnect operation and calling the
gserial_disconnect function,The port->port_usb pointer will be set to NULL.

E.g.
    Thread A                                 Thread B

    gs_open()                                gadget_unbind_driver()

    gs_start_io()                            composite_disconnect()

    gs_start_rx()                            gserial_disconnect()
    ...                                      ...
    spin_unlock(&port->port_lock)
    status = usb_ep_queue()                  spin_lock(&port->port_lock)
    spin_lock(&port->port_lock)              port->port_usb = NULL
    gs_free_requests(port->port_usb->in)     spin_unlock(&port->port_lock)
    Crash

This causes thread A to access a null pointer (port->port_usb is null)
when calling the gs_free_requests function, causing a crash.

If port_usb is NULL, the release request will be skipped as it
will be done by gserial_disconnect.

So add a null pointer check to gs_start_io before attempting
to access the value of the pointer port->port_usb.

Unable to handle kernel NULL pointer dereference at
virtual address 00000000000000e8
pc : gs_start_io+0x164/0x25c
lr : gs_start_io+0x238/0x25c
sp : ffffffc08b75ba00
x29: ffffffc08b75ba00 x28: ffffffed8ba01000 x27: 0000000000020902
x26: dead000000000100 x25: ffffff899f43a400 x24: ffffff8862325400
x23: ffffff88623256a4 x22: ffffff8862325690 x21: ffffff88623255ec
x20: ffffff88623255d8 x19: ffffff885e19d700 x18: ffffffed8c45ae40
x17: 00000000d48d30ad x16: 00000000d48d30ad x15: 0010000000000001
x14: ffffffed8c50fcc0 x13: 0000000040000000 x12: 0000000000000001
x11: 0000000080200012 x10: 0000000080200012 x9 : ffffff88623255d8
x8 : 0000000000000000 x7 : 0000000000000000 x6 : 000000000000003f
x5 : ffffffed8ae0b9a4 x4 : fffffffe267d0ea0 x3 : 0000000080200012
x2 : ffffff899f43a400 x1 : 0000000080200013 x0 : ffffff899f43b100
Call trace:
 gs_start_io+0x164/0x25c
 gs_open+0x108/0x13c
 tty_open+0x314/0x638
 chrdev_open+0x1b8/0x258
 do_dentry_open+0x2c4/0x700
 vfs_open+0x2c/0x3c
 path_openat+0xa64/0xc60
 do_filp_open+0xb8/0x164
 do_sys_openat2+0x84/0xf0
 __arm64_sys_openat+0x70/0x9c
 invoke_syscall+0x58/0x114
 el0_svc_common+0x80/0xe0
 do_el0_svc+0x1c/0x28
 el0_svc+0x38/0x68
 el0t_64_sync_handler+0x68/0xbc
 el0t_64_sync+0x1a8/0x1ac
Code: f2fbd5ba eb14013f 540004a1 f940e708 (f9407513)
---[ end trace 0000000000000000 ]---

Suggested-by: Prashanth K <quic_prashk@quicinc.com>
Signed-off-by: Lianqin Hu <hulianqin@vivo.com>

v3:
 - Update patch submission description
 - Link to v2: https://lore.kernel.org/all/TYUPR06MB62178D00DC96CC2702114CF5D2222@TYUPR06MB6217.apcprd06.prod.outlook.com/

v2:
 - Modify patch content and description according to "v1 suggestion"
 - Link to v1: https://lore.kernel.org/all/TYUPR06MB621737D16F68B5ABD6CF5772D2272@TYUPR06MB6217.apcprd06.prod.outlook.com/

 drivers/usb/gadget/function/u_serial.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
index 0a8c05b2746b..53d9fc41acc5 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -579,9 +579,12 @@ static int gs_start_io(struct gs_port *port)
 		 * we didn't in gs_start_tx() */
 		tty_wakeup(port->port.tty);
 	} else {
-		gs_free_requests(ep, head, &port->read_allocated);
-		gs_free_requests(port->port_usb->in, &port->write_pool,
-			&port->write_allocated);
+		/* Free reqs only if we are still connected */
+		if (port->port_usb) {
+			gs_free_requests(ep, head, &port->read_allocated);
+			gs_free_requests(port->port_usb->in, &port->write_pool,
+				&port->write_allocated);
+		}
 		status = -EIO;
 	}
 
-- 
2.39.0

Re: [PATCH v3] usb: gadget: u_serial: Fix the issue that gs_start_io crashed due to accessing null pointer

[Prashanth K]

On 26-11-24 12:30 pm, 胡连勤 wrote:
> From: Lianqin Hu <hulianqin@vivo.com>
> 
> Considering that in some extreme cases,
> when u_serial driver is accessed by multiple threads,
> Thread A is executing the open operation and calling the gs_open,
> Thread B is executing the disconnect operation and calling the
> gserial_disconnect function,The port->port_usb pointer will be set to NULL.
> 
> E.g.
>     Thread A                                 Thread B
> 
>     gs_open()                                gadget_unbind_driver()
> 
>     gs_start_io()                            composite_disconnect()
> 
>     gs_start_rx()                            gserial_disconnect()
>     ...                                      ...
>     spin_unlock(&port->port_lock)
>     status = usb_ep_queue()                  spin_lock(&port->port_lock)
>     spin_lock(&port->port_lock)              port->port_usb = NULL
>     gs_free_requests(port->port_usb->in)     spin_unlock(&port->port_lock)
>     Crash
> 
> This causes thread A to access a null pointer (port->port_usb is null)
> when calling the gs_free_requests function, causing a crash.
> 
> If port_usb is NULL, the release request will be skipped as it
> will be done by gserial_disconnect.
> 
> So add a null pointer check to gs_start_io before attempting
> to access the value of the pointer port->port_usb.
> 
> Unable to handle kernel NULL pointer dereference at
> virtual address 00000000000000e8
> pc : gs_start_io+0x164/0x25c
> lr : gs_start_io+0x238/0x25c
> sp : ffffffc08b75ba00
> x29: ffffffc08b75ba00 x28: ffffffed8ba01000 x27: 0000000000020902
> x26: dead000000000100 x25: ffffff899f43a400 x24: ffffff8862325400
> x23: ffffff88623256a4 x22: ffffff8862325690 x21: ffffff88623255ec
> x20: ffffff88623255d8 x19: ffffff885e19d700 x18: ffffffed8c45ae40
> x17: 00000000d48d30ad x16: 00000000d48d30ad x15: 0010000000000001
> x14: ffffffed8c50fcc0 x13: 0000000040000000 x12: 0000000000000001
> x11: 0000000080200012 x10: 0000000080200012 x9 : ffffff88623255d8
> x8 : 0000000000000000 x7 : 0000000000000000 x6 : 000000000000003f
> x5 : ffffffed8ae0b9a4 x4 : fffffffe267d0ea0 x3 : 0000000080200012
> x2 : ffffff899f43a400 x1 : 0000000080200013 x0 : ffffff899f43b100
> Call trace:
>  gs_start_io+0x164/0x25c
>  gs_open+0x108/0x13c
>  tty_open+0x314/0x638
>  chrdev_open+0x1b8/0x258
>  do_dentry_open+0x2c4/0x700
>  vfs_open+0x2c/0x3c
>  path_openat+0xa64/0xc60
>  do_filp_open+0xb8/0x164
>  do_sys_openat2+0x84/0xf0
>  __arm64_sys_openat+0x70/0x9c
>  invoke_syscall+0x58/0x114
>  el0_svc_common+0x80/0xe0
>  do_el0_svc+0x1c/0x28
>  el0_svc+0x38/0x68
>  el0t_64_sync_handler+0x68/0xbc
>  el0t_64_sync+0x1a8/0x1ac
> Code: f2fbd5ba eb14013f 540004a1 f940e708 (f9407513)
> ---[ end trace 0000000000000000 ]---
> 
> Suggested-by: Prashanth K <quic_prashk@quicinc.com>
> Signed-off-by: Lianqin Hu <hulianqin@vivo.com>
> 
> v3:
>  - Update patch submission description
>  - Link to v2: https://lore.kernel.org/all/TYUPR06MB62178D00DC96CC2702114CF5D2222@TYUPR06MB6217.apcprd06.prod.outlook.com/
> 
> v2:
>  - Modify patch content and description according to "v1 suggestion"
>  - Link to v1: https://lore.kernel.org/all/TYUPR06MB621737D16F68B5ABD6CF5772D2272@TYUPR06MB6217.apcprd06.prod.outlook.com/
> 
>  drivers/usb/gadget/function/u_serial.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
> index 0a8c05b2746b..53d9fc41acc5 100644
> --- a/drivers/usb/gadget/function/u_serial.c
> +++ b/drivers/usb/gadget/function/u_serial.c
> @@ -579,9 +579,12 @@ static int gs_start_io(struct gs_port *port)
>  		 * we didn't in gs_start_tx() */
>  		tty_wakeup(port->port.tty);
>  	} else {
> -		gs_free_requests(ep, head, &port->read_allocated);
> -		gs_free_requests(port->port_usb->in, &port->write_pool,
> -			&port->write_allocated);
> +		/* Free reqs only if we are still connected */
> +		if (port->port_usb) {
> +			gs_free_requests(ep, head, &port->read_allocated);
> +			gs_free_requests(port->port_usb->in, &port->write_pool,
> +				&port->write_allocated);
> +		}
>  		status = -EIO;
>  	}
>  
Had asked to remove some empty newlines and address list from dumpstack
from commit text -
https://lore.kernel.org/all/b0c787ad-55a1-4fb1-b9cd-1f688ea65316@quicinc.com/

And the rest seems fine to me.

Acked-by: Prashanth K <quic_prashk@quicinc.com>

Thanks,
Prashanth K

答复: [PATCH v3] usb: gadget: u_serial: Fix the issue that gs_start_io crashed due to accessing null pointer

[胡连勤]

Hello  Prashanth K:

> > From: Lianqin Hu <hulianqin@vivo.com>
> >
> > Considering that in some extreme cases, when u_serial driver is
> > accessed by multiple threads, Thread A is executing the open operation
> > and calling the gs_open, Thread B is executing the disconnect
> > operation and calling the gserial_disconnect function,The
> > port->port_usb pointer will be set to NULL.
> >
> > E.g.
> >     Thread A                                 Thread B
> >
> >     gs_open()                                gadget_unbind_driver()
> >
> >     gs_start_io()                            composite_disconnect()
> >
> >     gs_start_rx()                            gserial_disconnect()
> >     ...                                      ...
> >     spin_unlock(&port->port_lock)
> >     status = usb_ep_queue()                  spin_lock(&port->port_lock)
> >     spin_lock(&port->port_lock)              port->port_usb = NULL
> >     gs_free_requests(port->port_usb->in)     spin_unlock(&port->port_lock)
> >     Crash
> >
> > This causes thread A to access a null pointer (port->port_usb is null)
> > when calling the gs_free_requests function, causing a crash.
> >
> > If port_usb is NULL, the release request will be skipped as it will be
> > done by gserial_disconnect.
> >
> > So add a null pointer check to gs_start_io before attempting to access
> > the value of the pointer port->port_usb.
> >
> > Unable to handle kernel NULL pointer dereference at virtual address
> > 00000000000000e8 pc : gs_start_io+0x164/0x25c lr :
> > gs_start_io+0x238/0x25c sp : ffffffc08b75ba00
> > x29: ffffffc08b75ba00 x28: ffffffed8ba01000 x27: 0000000000020902
> > x26: dead000000000100 x25: ffffff899f43a400 x24: ffffff8862325400
> > x23: ffffff88623256a4 x22: ffffff8862325690 x21: ffffff88623255ec
> > x20: ffffff88623255d8 x19: ffffff885e19d700 x18: ffffffed8c45ae40
> > x17: 00000000d48d30ad x16: 00000000d48d30ad x15: 0010000000000001
> > x14: ffffffed8c50fcc0 x13: 0000000040000000 x12: 0000000000000001
> > x11: 0000000080200012 x10: 0000000080200012 x9 : ffffff88623255d8
> > x8 : 0000000000000000 x7 : 0000000000000000 x6 : 000000000000003f
> > x5 : ffffffed8ae0b9a4 x4 : fffffffe267d0ea0 x3 : 0000000080200012
> > x2 : ffffff899f43a400 x1 : 0000000080200013 x0 : ffffff899f43b100 Call
> > trace:
> >  gs_start_io+0x164/0x25c
> >  gs_open+0x108/0x13c
> >  tty_open+0x314/0x638
> >  chrdev_open+0x1b8/0x258
> >  do_dentry_open+0x2c4/0x700
> >  vfs_open+0x2c/0x3c
> >  path_openat+0xa64/0xc60
> >  do_filp_open+0xb8/0x164
> >  do_sys_openat2+0x84/0xf0
> >  __arm64_sys_openat+0x70/0x9c
> >  invoke_syscall+0x58/0x114
> >  el0_svc_common+0x80/0xe0
> >  do_el0_svc+0x1c/0x28
> >  el0_svc+0x38/0x68
> >  el0t_64_sync_handler+0x68/0xbc
> >  el0t_64_sync+0x1a8/0x1ac
> > Code: f2fbd5ba eb14013f 540004a1 f940e708 (f9407513) ---[ end trace
> > 0000000000000000 ]---
> >
> > Suggested-by: Prashanth K <quic_prashk@quicinc.com>
> > Signed-off-by: Lianqin Hu <hulianqin@vivo.com>
> >
> > v3:
> >  - Update patch submission description
> >  - Link to v2:
> > https://lore/
> > .kernel.org%2Fall%2FTYUPR06MB62178D00DC96CC2702114CF5D2222%40TY
> UPR06MB
> >
> 6217.apcprd06.prod.outlook.com%2F&data=05%7C02%7Chulianqin%40vivo.c
> om%
> >
> 7C2c27d8875c9940f5010608dd135c7a3e%7C923e42dc48d54cbeb5821a797a64
> 12ed%
> >
> 7C0%7C0%7C638688010184365324%7CUnknown%7CTWFpbGZsb3d8eyJFbXB
> 0eU1hcGkiO
> >
> nRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyf
> Q%
> >
> 3D%3D%7C0%7C%7C%7C&sdata=788E7ThtQFu3VokdzFNulQgRz8KgsMALy40
> gpHdst0Q%3
> > D&reserved=0
> >
> > v2:
> >  - Modify patch content and description according to "v1 suggestion"
> >  - Link to v1:
> > https://lore/
> > .kernel.org%2Fall%2FTYUPR06MB621737D16F68B5ABD6CF5772D2272%40TY
> UPR06MB
> >
> 6217.apcprd06.prod.outlook.com%2F&data=05%7C02%7Chulianqin%40vivo.c
> om%
> >
> 7C2c27d8875c9940f5010608dd135c7a3e%7C923e42dc48d54cbeb5821a797a64
> 12ed%
> >
> 7C0%7C0%7C638688010184383214%7CUnknown%7CTWFpbGZsb3d8eyJFbXB
> 0eU1hcGkiO
> >
> nRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyf
> Q%
> >
> 3D%3D%7C0%7C%7C%7C&sdata=lDOfw5mtN2AWFRaTinoJhIEjIzpj%2FYfmjW
> YfERamtqA
> > %3D&reserved=0
> >
> >  drivers/usb/gadget/function/u_serial.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/function/u_serial.c
> > b/drivers/usb/gadget/function/u_serial.c
> > index 0a8c05b2746b..53d9fc41acc5 100644
> > --- a/drivers/usb/gadget/function/u_serial.c
> > +++ b/drivers/usb/gadget/function/u_serial.c
> > @@ -579,9 +579,12 @@ static int gs_start_io(struct gs_port *port)
> >              * we didn't in gs_start_tx() */
> >             tty_wakeup(port->port.tty);
> >     } else {
> > -           gs_free_requests(ep, head, &port->read_allocated);
> > -           gs_free_requests(port->port_usb->in, &port->write_pool,
> > -                   &port->write_allocated);
> > +           /* Free reqs only if we are still connected */
> > +           if (port->port_usb) {
> > +                   gs_free_requests(ep, head, &port->read_allocated);
> > +                   gs_free_requests(port->port_usb->in, &port-
> >write_pool,
> > +                           &port->write_allocated);
> > +           }
> >             status = -EIO;
> >     }
> >
> Had asked to remove some empty newlines and address list from dumpstack
> from commit text -
> https://lore.k/
> ernel.org%2Fall%2Fb0c787ad-55a1-4fb1-b9cd-
> 1f688ea65316%40quicinc.com%2F&data=05%7C02%7Chulianqin%40vivo.com
> %7C2c27d8875c9940f5010608dd135c7a3e%7C923e42dc48d54cbeb5821a797a6
> 412ed%7C0%7C0%7C638688010184391526%7CUnknown%7CTWFpbGZsb3d8
> eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOI
> joiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=KNsb79lZeuTxGaE
> QAd89GAQ91XJ8nmmOJ1ZbZNFiJgU%3D&reserved=0
>
> And the rest seems fine to me.
>
> Acked-by: Prashanth K <quic_prashk@quicinc.com>

I'm very sorry that I didn't notice some of the previous requirements due to negligence in reading.
I will modify and re-release the version as requested before.
https://lore.kernel.org/all/b0c787ad-55a1-4fb1-b9cd-1f688ea65316@quicinc.com/

Thanks

Re: 答复: [PATCH v3] usb: gadget: u_serial: Fix the issue that gs_start_io crashed due to accessing null pointer

[Prashanth K]

On 03-12-24 12:09 pm, 胡连勤 wrote:
> Hello  Prashanth K:
> 
>>> From: Lianqin Hu <hulianqin@vivo.com>
>>>

[...]
>> And the rest seems fine to me.
>>
>> Acked-by: Prashanth K <quic_prashk@quicinc.com>
> 
> I'm very sorry that I didn't notice some of the previous requirements due to negligence in reading.
> I will modify and re-release the version as requested before.
> https://lore.kernel.org/all/b0c787ad-55a1-4fb1-b9cd-1f688ea65316@quicinc.com/
> 
Yea, Along with those, make sure to add fixes tag and cc stable kernel.
Feel free to add the Acked-by tag after making the above changes.

Regards,