mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH] usb: gadget: u_serial: Add null pointer check in gserial_suspend
@ 2023-05-05  9:18 Prashanth K
  2023-05-06 15:45 ` Alan Stern
  0 siblings, 1 reply; 5+ messages in thread
From: Prashanth K @ 2023-05-05  9:18 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Alan Stern
  Cc: Xiu Jianfeng, Christophe JAILLET, linux-usb, linux-kernel, Prashanth K

Consider a case where gserial_disconnect has already cleared
gser->ioport. And if gserial_suspend gets called afterwards,
it will lead to accessing of gser->ioport and thus causing
null pointer dereference.

Avoid this by adding a null pointer check. Added a static
spinlock to prevent gser->ioport from becoming null after
the newly added null pointer check.

Fixes: aba3a8d01d62 ("usb: gadget: u_serial: add suspend resume callbacks")
Signed-off-by: Prashanth K <quic_prashk@quicinc.com>
---
 drivers/usb/gadget/function/u_serial.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
index a0ca47f..e5d522d 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -1420,10 +1420,19 @@ EXPORT_SYMBOL_GPL(gserial_disconnect);
 
 void gserial_suspend(struct gserial *gser)
 {
-	struct gs_port	*port = gser->ioport;
+	struct gs_port	*port;
 	unsigned long	flags;
 
-	spin_lock_irqsave(&port->port_lock, flags);
+	spin_lock_irqsave(&serial_port_lock, flags);
+	port = gser->ioport;
+
+	if (!port) {
+		spin_unlock_irqrestore(&serial_port_lock, flags);
+		return;
+	}
+
+	spin_lock(&port->port_lock);
+	spin_unlock(&serial_port_lock);
 	port->suspended = true;
 	spin_unlock_irqrestore(&port->port_lock, flags);
 }
-- 
2.7.4


^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] usb: gadget: u_serial: Add null pointer check in gserial_suspend
  2023-05-05  9:18 [PATCH] usb: gadget: u_serial: Add null pointer check in gserial_suspend Prashanth K
@ 2023-05-06 15:45 ` Alan Stern
  0 siblings, 0 replies; 5+ messages in thread
From: Alan Stern @ 2023-05-06 15:45 UTC (permalink / raw)
  To: Prashanth K
  Cc: Greg Kroah-Hartman, Xiu Jianfeng, Christophe JAILLET, linux-usb,
	linux-kernel

On Fri, May 05, 2023 at 02:48:37PM +0530, Prashanth K wrote:
> Consider a case where gserial_disconnect has already cleared
> gser->ioport. And if gserial_suspend gets called afterwards,
> it will lead to accessing of gser->ioport and thus causing
> null pointer dereference.
> 
> Avoid this by adding a null pointer check. Added a static
> spinlock to prevent gser->ioport from becoming null after
> the newly added null pointer check.
> 
> Fixes: aba3a8d01d62 ("usb: gadget: u_serial: add suspend resume callbacks")
> Signed-off-by: Prashanth K <quic_prashk@quicinc.com>
> ---
>  drivers/usb/gadget/function/u_serial.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
> index a0ca47f..e5d522d 100644
> --- a/drivers/usb/gadget/function/u_serial.c
> +++ b/drivers/usb/gadget/function/u_serial.c
> @@ -1420,10 +1420,19 @@ EXPORT_SYMBOL_GPL(gserial_disconnect);
>  
>  void gserial_suspend(struct gserial *gser)
>  {
> -	struct gs_port	*port = gser->ioport;
> +	struct gs_port	*port;
>  	unsigned long	flags;
>  
> -	spin_lock_irqsave(&port->port_lock, flags);
> +	spin_lock_irqsave(&serial_port_lock, flags);
> +	port = gser->ioport;
> +
> +	if (!port) {
> +		spin_unlock_irqrestore(&serial_port_lock, flags);
> +		return;
> +	}
> +
> +	spin_lock(&port->port_lock);
> +	spin_unlock(&serial_port_lock);
>  	port->suspended = true;
>  	spin_unlock_irqrestore(&port->port_lock, flags);
>  }

This looks fine to me, but I'm not a serial-gadget maintainer.  
In fact, it looks like we don't have a serial-gadget maintainer.

Alan Stern

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] usb: gadget: u_serial: Add null pointer check in gserial_suspend
  2023-05-22  5:49 ` Prashanth K
@ 2023-05-25  7:00   ` Chunfeng Yun (云春峰)
  0 siblings, 0 replies; 5+ messages in thread
From: Chunfeng Yun (云春峰) @ 2023-05-25  7:00 UTC (permalink / raw)
  To: quic_prashk, gregkh
  Cc: linux-kernel, linux-mediatek, linux-usb, fabrice.gasnier, stable,
	balbi, xiujianfeng, Kewu Chen (陈科伍),
	stern, linux-arm-kernel, matthias.bgg, christophe.jaillet,
	angelogioacchino.delregno

On Mon, 2023-05-22 at 11:19 +0530, Prashanth K wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On 22-05-23 07:51 am, Chunfeng Yun wrote:
> > When gserial_disconnect has already cleared gser->ioport, and the
> > suspend triggers afterwards, gserial_suspend gets called, which
> > will
> > lead to accessing of gser->ioport and thus causing null pointer
> > dereference. Add a null pointer check to prevent it as the bellow
> > patch does:
> > 5ec63fdbca60 ("usb: gadget: u_serial: Add null pointer check in
> > gserial_resume")
> > 
> > Fixes: aba3a8d01d62 ("usb: gadget: u_serial: add suspend resume
> > callbacks")
> > Cc: stable <stable@kernel.org>
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > ---
> >   drivers/usb/gadget/function/u_serial.c | 12 ++++++++++--
> >   1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/usb/gadget/function/u_serial.c
> > b/drivers/usb/gadget/function/u_serial.c
> > index a0ca47fbff0f..40ba220cf6d2 100644
> > --- a/drivers/usb/gadget/function/u_serial.c
> > +++ b/drivers/usb/gadget/function/u_serial.c
> > @@ -1420,10 +1420,18 @@ EXPORT_SYMBOL_GPL(gserial_disconnect);
> > 
> >   void gserial_suspend(struct gserial *gser)
> >   {
> > -     struct gs_port  *port = gser->ioport;
> > +     struct gs_port  *port;
> >       unsigned long   flags;
> > 
> > -     spin_lock_irqsave(&port->port_lock, flags);
> > +     spin_lock_irqsave(&serial_port_lock, flags);
> > +     port = gser->ioport;
> > +     if (!port) {
> > +             spin_unlock_irqrestore(&serial_port_lock, flags);
> > +             return;
> > +     }
> > +
> > +     spin_lock(&port->port_lock);
> > +     spin_unlock(&serial_port_lock);
> >       port->suspended = true;
> >       spin_unlock_irqrestore(&port->port_lock, flags);
> >   }
> 
> Hi Chunfeng,
> 
> This looks same as the following patch.
> 
https://lore.kernel.org/linux-usb/1683278317-11774-1-git-send-email-quic_prashk@quicinc.com/

Yes, it is, please ignore this one, thanks a lot

> 
> 
> Regards

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] usb: gadget: u_serial: Add null pointer check in gserial_suspend
  2023-05-22  2:21 Chunfeng Yun
@ 2023-05-22  5:49 ` Prashanth K
  2023-05-25  7:00   ` Chunfeng Yun (云春峰)
  0 siblings, 1 reply; 5+ messages in thread
From: Prashanth K @ 2023-05-22  5:49 UTC (permalink / raw)
  To: Chunfeng Yun, Greg Kroah-Hartman
  Cc: Matthias Brugger, AngeloGioacchino Del Regno, Alan Stern,
	Xiu Jianfeng, Christophe JAILLET, Fabrice Gasnier, Felipe Balbi,
	linux-usb, linux-kernel, linux-arm-kernel, linux-mediatek,
	Kewu Chen, stable



On 22-05-23 07:51 am, Chunfeng Yun wrote:
> When gserial_disconnect has already cleared gser->ioport, and the
> suspend triggers afterwards, gserial_suspend gets called, which will
> lead to accessing of gser->ioport and thus causing null pointer
> dereference. Add a null pointer check to prevent it as the bellow
> patch does:
> 5ec63fdbca60 ("usb: gadget: u_serial: Add null pointer check in gserial_resume")
> 
> Fixes: aba3a8d01d62 ("usb: gadget: u_serial: add suspend resume callbacks")
> Cc: stable <stable@kernel.org>
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> ---
>   drivers/usb/gadget/function/u_serial.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
> index a0ca47fbff0f..40ba220cf6d2 100644
> --- a/drivers/usb/gadget/function/u_serial.c
> +++ b/drivers/usb/gadget/function/u_serial.c
> @@ -1420,10 +1420,18 @@ EXPORT_SYMBOL_GPL(gserial_disconnect);
>   
>   void gserial_suspend(struct gserial *gser)
>   {
> -	struct gs_port	*port = gser->ioport;
> +	struct gs_port	*port;
>   	unsigned long	flags;
>   
> -	spin_lock_irqsave(&port->port_lock, flags);
> +	spin_lock_irqsave(&serial_port_lock, flags);
> +	port = gser->ioport;
> +	if (!port) {
> +		spin_unlock_irqrestore(&serial_port_lock, flags);
> +		return;
> +	}
> +
> +	spin_lock(&port->port_lock);
> +	spin_unlock(&serial_port_lock);
>   	port->suspended = true;
>   	spin_unlock_irqrestore(&port->port_lock, flags);
>   }
Hi Chunfeng,

This looks same as the following patch.
https://lore.kernel.org/linux-usb/1683278317-11774-1-git-send-email-quic_prashk@quicinc.com/ 


Regards

^ permalink raw reply	[flat|nested] 5+ messages in thread
* [PATCH] usb: gadget: u_serial: Add null pointer check in gserial_suspend
@ 2023-05-22  2:21 Chunfeng Yun
  2023-05-22  5:49 ` Prashanth K
  0 siblings, 1 reply; 5+ messages in thread
From: Chunfeng Yun @ 2023-05-22  2:21 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Matthias Brugger, AngeloGioacchino Del Regno, Alan Stern,
	Chunfeng Yun, Prashanth K, Xiu Jianfeng, Christophe JAILLET,
	Fabrice Gasnier, Felipe Balbi, linux-usb, linux-kernel,
	linux-arm-kernel, linux-mediatek, Kewu Chen, stable

When gserial_disconnect has already cleared gser->ioport, and the
suspend triggers afterwards, gserial_suspend gets called, which will
lead to accessing of gser->ioport and thus causing null pointer
dereference. Add a null pointer check to prevent it as the bellow
patch does:
5ec63fdbca60 ("usb: gadget: u_serial: Add null pointer check in gserial_resume")

Fixes: aba3a8d01d62 ("usb: gadget: u_serial: add suspend resume callbacks")
Cc: stable <stable@kernel.org>
Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 drivers/usb/gadget/function/u_serial.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
index a0ca47fbff0f..40ba220cf6d2 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -1420,10 +1420,18 @@ EXPORT_SYMBOL_GPL(gserial_disconnect);
 
 void gserial_suspend(struct gserial *gser)
 {
-	struct gs_port	*port = gser->ioport;
+	struct gs_port	*port;
 	unsigned long	flags;
 
-	spin_lock_irqsave(&port->port_lock, flags);
+	spin_lock_irqsave(&serial_port_lock, flags);
+	port = gser->ioport;
+	if (!port) {
+		spin_unlock_irqrestore(&serial_port_lock, flags);
+		return;
+	}
+
+	spin_lock(&port->port_lock);
+	spin_unlock(&serial_port_lock);
 	port->suspended = true;
 	spin_unlock_irqrestore(&port->port_lock, flags);
 }
-- 
2.18.0


^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-05-25  7:00 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-05  9:18 [PATCH] usb: gadget: u_serial: Add null pointer check in gserial_suspend Prashanth K
2023-05-06 15:45 ` Alan Stern
2023-05-22  2:21 Chunfeng Yun
2023-05-22  5:49 ` Prashanth K
2023-05-25  7:00   ` Chunfeng Yun (云春峰)

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®