linux-kernel - Re: [PATCH v7] usb: gadget: u_serial: Add null pointer check in gs_read_complete & gs_write

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <33fe50c8-cf98-bd04-87b7-06ec07cc9277@quicinc.com>
Date: Tue, 27 Aug 2024 14:45:51 +0530
From: Prashanth K <quic_prashk@...cinc.com>
To: 胡连勤 <hulianqin@...o.com>,
        Michael Nazzareno Trimarchi
	<michael@...rulasolutions.com>,
        "gregkh@...uxfoundation.org"
	<gregkh@...uxfoundation.org>
CC: "quic_jjohnson@...cinc.com" <quic_jjohnson@...cinc.com>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        opensource.kernel <opensource.kernel@...o.com>,
        "akpm@...ux-foundation.org"
	<akpm@...ux-foundation.org>
Subject: Re: [PATCH v7] usb: gadget: u_serial: Add null pointer check in
 gs_read_complete & gs_write_complete



On 25-08-24 01:58 pm, 胡连勤 wrote:
> From: Lianqin Hu <hulianqin@...o.com>
> 
> Considering that in some extreme cases, when the unbind operation
> is being executed, gserial_disconnect has already cleared gser->ioport,
> and the controller has not stopped & pullup 0, sys.usb.config is reset

As mentioned by Michael earlier, avoid android specific (sys.usb.config)
operations.

> and the bind operation will be re-executed, calling gs_read_complete,
> which will result in accessing gser->iport, resulting in a null pointer
> dereference, add a null pointer check to prevent this situation.
> 
> Added a static spinlock to prevent gser->ioport from becoming
> null after the newly added check.
> 
> Unable to handle kernel NULL pointer dereference at

[...]
> 
> Fixes: c1dca562be8a ("usb gadget: split out serial core")
> Cc: stable@...r.kernel.org
> Signed-off-by: Lianqin Hu <hulianqin@...o.com>
> ---
> v7:
>   - Remove code comments
>   - Update the commit text
>   - Add the Fixes tag
>   - CC stable kernel
>   - Add serial_port_lock protection when checking port pointer
>   - Optimize code comments
>   - Delete log printing

We usually indicate what had changed in each versions.
v7: Remove code comments
v6: xx
v5: xx
v4: xx
v3: xx
v2: Delete log printing

> ---
No need for this '---' here, just leave an empty line
>  drivers/usb/gadget/function/u_serial.c | 33 ++++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
> index b394105e55d6..e43d8065f7ec 100644
> --- a/drivers/usb/gadget/function/u_serial.c
> +++ b/drivers/usb/gadget/function/u_serial.c
> @@ -452,20 +452,43 @@ static void gs_rx_push(struct work_struct *work)
>  
>  static void gs_read_complete(struct usb_ep *ep, struct usb_request *req)
>  {
> -	struct gs_port	*port = ep->driver_data;
> +	struct gs_port	*port;
> +	unsigned long  flags;
> +
> +	spin_lock_irqsave(&serial_port_lock, flags);
> +	port = ep->driver_data;
> +
> +	if (!port) {
> +		spin_unlock_irqrestore(&serial_port_lock, flags);
> +		return;
> +	}
>  
> -	/* Queue all received data until the tty layer is ready for it. */
>  	spin_lock(&port->port_lock);
> +	spin_unlock(&serial_port_lock);
> +
> +	/* Queue all received data until the tty layer is ready for it. */
>  	list_add_tail(&req->list, &port->read_queue);
>  	schedule_delayed_work(&port->push, 0);
> -	spin_unlock(&port->port_lock);
> +	spin_unlock_irqrestore(&port->port_lock, flags);
>  }
>  
>  static void gs_write_complete(struct usb_ep *ep, struct usb_request *req)
>  {
> -	struct gs_port	*port = ep->driver_data;
> +	struct gs_port	*port;
> +	unsigned long  flags;
> +
> +	spin_lock_irqsave(&serial_port_lock, flags);
> +	port = ep->driver_data;
> +
> +	if (!port) {
> +		spin_unlock_irqrestore(&serial_port_lock, flags);
> +		return;
> +	}
>  
>  	spin_lock(&port->port_lock);
> +	spin_unlock(&serial_port_lock);
>  	list_add(&req->list, &port->write_pool);
>  	port->write_started--;
>  
> @@ -486,7 +509,7 @@ static void gs_write_complete(struct usb_ep *ep, struct usb_request *req)
>  		break;
>  	}
>  
> -	spin_unlock(&port->port_lock);
> +	spin_unlock_irqrestore(&port->port_lock, flags);
>  }
>  
>  static void gs_free_requests(struct usb_ep *ep, struct list_head *head,
Regards,