| Message ID | TYUPR06MB6217989A40FA4E18598DC694D28A2@TYUPR06MB6217.apcprd06.prod.outlook.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v7] usb: gadget: u_serial: Add null pointer check in gs_read_complete & gs_write_complete | expand |
On 25-08-24 01:58 pm, 胡连勤 wrote: > From: Lianqin Hu <hulianqin@vivo.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@vger.kernel.org > Signed-off-by: Lianqin Hu <hulianqin@vivo.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,
Hi Prashanth: >> 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. OK, I'll need to submit the description later. >> 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@vger.kernel.org >> Signed-off-by: Lianqin Hu <hulianqin@vivo.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 OK, I will modify the version information according to the above requirements, so that you can see the changes in each version more intuitively. >> --- >No need for this '---' here, just leave an empty line I removed the "---" and changed it to a blank 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) Thanks
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;