| Message ID | 1675864487-18620-1-git-send-email-quic_prashk@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: gadget: u_serial: Add null pointer check in gserial_resume | expand |
On Wed, Feb 08, 2023 at 07:24:47PM +0530, Prashanth K wrote: > Consider a case where gserial_disconnect has already cleared > gser->ioport. And if a wakeup interrupt triggers afterwards, > gserial_resume gets called, which will lead to accessing of > gserial->port and thus causing null pointer dereference.Add > a null pointer check to prevent this. > > Fixes: aba3a8d01d62 (" usb: gadget: u_serial: add suspend resume callbacks") Nit, and our tools will complain, no " " before the "usb:" string here, right? > Signed-off-by: Prashanth K <quic_prashk@quicinc.com> > --- > drivers/usb/gadget/function/u_serial.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c > index 840626e..98be2b8 100644 > --- a/drivers/usb/gadget/function/u_serial.c > +++ b/drivers/usb/gadget/function/u_serial.c > @@ -1428,6 +1428,9 @@ void gserial_resume(struct gserial *gser) > struct gs_port *port = gser->ioport; > unsigned long flags; > > + if (!port) > + return; > + What prevents port from going to NULL right after this check? thanks, greg k-h
On 08-02-23 08:24 pm, Greg Kroah-Hartman wrote: > On Wed, Feb 08, 2023 at 07:24:47PM +0530, Prashanth K wrote: >> Consider a case where gserial_disconnect has already cleared >> gser->ioport. And if a wakeup interrupt triggers afterwards, >> gserial_resume gets called, which will lead to accessing of >> gserial->port and thus causing null pointer dereference.Add >> a null pointer check to prevent this. >> >> Fixes: aba3a8d01d62 (" usb: gadget: u_serial: add suspend resume callbacks") > > Nit, and our tools will complain, no " " before the "usb:" string here, > right? > Will fix it in next patch. > > >> Signed-off-by: Prashanth K <quic_prashk@quicinc.com> >> --- >> drivers/usb/gadget/function/u_serial.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c >> index 840626e..98be2b8 100644 >> --- a/drivers/usb/gadget/function/u_serial.c >> +++ b/drivers/usb/gadget/function/u_serial.c >> @@ -1428,6 +1428,9 @@ void gserial_resume(struct gserial *gser) >> struct gs_port *port = gser->ioport; >> unsigned long flags; >> >> + if (!port) >> + return; >> + > > What prevents port from going to NULL right after this check? In our case we got a null pointer de-reference while performing USB compliance tests, as the gser->port was null. Because in gserial_resume, spinlock_irq_save(&port->port_lock) accesses a null-pointer as port was already marked null by gserial_disconnect. And after gserial_resume acquires the spinlock, gserial_disconnect cant mark it null until the spinlock is released. We need to check if the port->lock is valid before accessing it, otherwise it can lead to the above mentioned scenario Issue Type: kernel panic issue Issue AutoSignature: pc : do_raw_spin_lock lr : _raw_spin_lock_irqsave Call trace: do_raw_spin_lock _raw_spin_lock_irqsave gserial_resume acm_resume composite_resume configfs_composite_resume dwc3_process_event_entry dwc3_process_event_buf dwc3_thread_interrupt irq_thread_fn irq_thread kthread ret_from_fork Thanks in advance, Prashanth > > thanks, > > greg k-h
On Wed, Feb 08, 2023 at 09:15:54PM +0530, Prashanth K wrote: > > > On 08-02-23 08:24 pm, Greg Kroah-Hartman wrote: > > On Wed, Feb 08, 2023 at 07:24:47PM +0530, Prashanth K wrote: > > > Consider a case where gserial_disconnect has already cleared > > > gser->ioport. And if a wakeup interrupt triggers afterwards, > > > gserial_resume gets called, which will lead to accessing of > > > gserial->port and thus causing null pointer dereference.Add > > > a null pointer check to prevent this. > > > > > > Fixes: aba3a8d01d62 (" usb: gadget: u_serial: add suspend resume callbacks") > > > > Nit, and our tools will complain, no " " before the "usb:" string here, > > right? > > > Will fix it in next patch. > > > > > > > Signed-off-by: Prashanth K <quic_prashk@quicinc.com> > > > --- > > > drivers/usb/gadget/function/u_serial.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c > > > index 840626e..98be2b8 100644 > > > --- a/drivers/usb/gadget/function/u_serial.c > > > +++ b/drivers/usb/gadget/function/u_serial.c > > > @@ -1428,6 +1428,9 @@ void gserial_resume(struct gserial *gser) > > > struct gs_port *port = gser->ioport; > > > unsigned long flags; > > > + if (!port) > > > + return; > > > + > > > > What prevents port from going to NULL right after this check? > In our case we got a null pointer de-reference while performing USB > compliance tests, as the gser->port was null. Because in gserial_resume, > spinlock_irq_save(&port->port_lock) accesses a null-pointer as port was > already marked null by gserial_disconnect. > > And after gserial_resume acquires the spinlock, gserial_disconnect cant mark > it null until the spinlock is released. We need to check if the port->lock > is valid before accessing it, otherwise it can lead to the above mentioned > scenario What happens if gserial_disconnect sets gser->port to NULL immediately after your new check occurs, but before spinlock_irq_save(&port->port_lock) gets called? You may need to add a static spinlock to prevent this from happening. Alan Stern
On 09-02-23 01:51 am, Alan Stern wrote: > On Wed, Feb 08, 2023 at 09:15:54PM +0530, Prashanth K wrote: >> >> >> On 08-02-23 08:24 pm, Greg Kroah-Hartman wrote: >>> On Wed, Feb 08, 2023 at 07:24:47PM +0530, Prashanth K wrote: >>>> Consider a case where gserial_disconnect has already cleared >>>> gser->ioport. And if a wakeup interrupt triggers afterwards, >>>> gserial_resume gets called, which will lead to accessing of >>>> gserial->port and thus causing null pointer dereference.Add >>>> a null pointer check to prevent this. >>>> >>>> Fixes: aba3a8d01d62 (" usb: gadget: u_serial: add suspend resume callbacks") >>> >>> Nit, and our tools will complain, no " " before the "usb:" string here, >>> right? >>> >> Will fix it in next patch. >>> >>> >>>> Signed-off-by: Prashanth K <quic_prashk@quicinc.com> >>>> --- >>>> drivers/usb/gadget/function/u_serial.c | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c >>>> index 840626e..98be2b8 100644 >>>> --- a/drivers/usb/gadget/function/u_serial.c >>>> +++ b/drivers/usb/gadget/function/u_serial.c >>>> @@ -1428,6 +1428,9 @@ void gserial_resume(struct gserial *gser) >>>> struct gs_port *port = gser->ioport; >>>> unsigned long flags; >>>> + if (!port) >>>> + return; >>>> + >>> >>> What prevents port from going to NULL right after this check? >> In our case we got a null pointer de-reference while performing USB >> compliance tests, as the gser->port was null. Because in gserial_resume, >> spinlock_irq_save(&port->port_lock) accesses a null-pointer as port was >> already marked null by gserial_disconnect. >> >> And after gserial_resume acquires the spinlock, gserial_disconnect cant mark >> it null until the spinlock is released. We need to check if the port->lock >> is valid before accessing it, otherwise it can lead to the above mentioned >> scenario > > What happens if gserial_disconnect sets gser->port to NULL immediately > after your new check occurs, but before > spinlock_irq_save(&port->port_lock) gets called? > > You may need to add a static spinlock to prevent this from happening. > > Alan Stern In that case i guess we have to make port_lock a global variable and take it out of gs_port structure. + static DEFINE_SPINLOCK(port_lock); struct gs_port { struct tty_port port; - spinlock_t port_lock; This will require us to change all the spinlock(port->port_lock) used in u_serial.c, what do you suggest?
On Thu, Feb 09, 2023 at 10:31:50AM +0530, Prashanth K wrote: > > > On 09-02-23 01:51 am, Alan Stern wrote: > > On Wed, Feb 08, 2023 at 09:15:54PM +0530, Prashanth K wrote: > > > > > > > > > On 08-02-23 08:24 pm, Greg Kroah-Hartman wrote: > > > > On Wed, Feb 08, 2023 at 07:24:47PM +0530, Prashanth K wrote: > > > > > Consider a case where gserial_disconnect has already cleared > > > > > gser->ioport. And if a wakeup interrupt triggers afterwards, > > > > > gserial_resume gets called, which will lead to accessing of > > > > > gserial->port and thus causing null pointer dereference.Add > > > > > a null pointer check to prevent this. > > > > > > > > > > Fixes: aba3a8d01d62 (" usb: gadget: u_serial: add suspend resume callbacks") > > > > > > > > Nit, and our tools will complain, no " " before the "usb:" string here, > > > > right? > > > > > > > Will fix it in next patch. > > > > > > > > > > > > > Signed-off-by: Prashanth K <quic_prashk@quicinc.com> > > > > > --- > > > > > drivers/usb/gadget/function/u_serial.c | 3 +++ > > > > > 1 file changed, 3 insertions(+) > > > > > > > > > > diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c > > > > > index 840626e..98be2b8 100644 > > > > > --- a/drivers/usb/gadget/function/u_serial.c > > > > > +++ b/drivers/usb/gadget/function/u_serial.c > > > > > @@ -1428,6 +1428,9 @@ void gserial_resume(struct gserial *gser) > > > > > struct gs_port *port = gser->ioport; > > > > > unsigned long flags; > > > > > + if (!port) > > > > > + return; > > > > > + > > > > > > > > What prevents port from going to NULL right after this check? > > > In our case we got a null pointer de-reference while performing USB > > > compliance tests, as the gser->port was null. Because in gserial_resume, > > > spinlock_irq_save(&port->port_lock) accesses a null-pointer as port was > > > already marked null by gserial_disconnect. > > > > > > And after gserial_resume acquires the spinlock, gserial_disconnect cant mark > > > it null until the spinlock is released. We need to check if the port->lock > > > is valid before accessing it, otherwise it can lead to the above mentioned > > > scenario > > > > What happens if gserial_disconnect sets gser->port to NULL immediately > > after your new check occurs, but before > > spinlock_irq_save(&port->port_lock) gets called? > > > > You may need to add a static spinlock to prevent this from happening. > > > > Alan Stern > In that case i guess we have to make port_lock a global variable and take it > out of gs_port structure. > > + static DEFINE_SPINLOCK(port_lock); > > struct gs_port { > struct tty_port port; > - spinlock_t port_lock; > > This will require us to change all the spinlock(port->port_lock) used in > u_serial.c, what do you suggest? Yes, that would be the correct thing to do.
On 09-02-23 12:31 pm, Greg Kroah-Hartman wrote: > On Thu, Feb 09, 2023 at 10:31:50AM +0530, Prashanth K wrote: >> >> >> On 09-02-23 01:51 am, Alan Stern wrote: >>> On Wed, Feb 08, 2023 at 09:15:54PM +0530, Prashanth K wrote: >>>> >>>> >>>> On 08-02-23 08:24 pm, Greg Kroah-Hartman wrote: >>>>> On Wed, Feb 08, 2023 at 07:24:47PM +0530, Prashanth K wrote: >>>>>> Consider a case where gserial_disconnect has already cleared >>>>>> gser->ioport. And if a wakeup interrupt triggers afterwards, >>>>>> gserial_resume gets called, which will lead to accessing of >>>>>> gserial->port and thus causing null pointer dereference.Add >>>>>> a null pointer check to prevent this. >>>>>> >>>>>> Fixes: aba3a8d01d62 (" usb: gadget: u_serial: add suspend resume callbacks") >>>>> >>>>> Nit, and our tools will complain, no " " before the "usb:" string here, >>>>> right? >>>>> >>>> Will fix it in next patch. >>>>> >>>>> >>>>>> Signed-off-by: Prashanth K <quic_prashk@quicinc.com> >>>>>> --- >>>>>> drivers/usb/gadget/function/u_serial.c | 3 +++ >>>>>> 1 file changed, 3 insertions(+) >>>>>> >>>>>> diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c >>>>>> index 840626e..98be2b8 100644 >>>>>> --- a/drivers/usb/gadget/function/u_serial.c >>>>>> +++ b/drivers/usb/gadget/function/u_serial.c >>>>>> @@ -1428,6 +1428,9 @@ void gserial_resume(struct gserial *gser) >>>>>> struct gs_port *port = gser->ioport; >>>>>> unsigned long flags; >>>>>> + if (!port) >>>>>> + return; >>>>>> + >>>>> >>>>> What prevents port from going to NULL right after this check? >>>> In our case we got a null pointer de-reference while performing USB >>>> compliance tests, as the gser->port was null. Because in gserial_resume, >>>> spinlock_irq_save(&port->port_lock) accesses a null-pointer as port was >>>> already marked null by gserial_disconnect. >>>> >>>> And after gserial_resume acquires the spinlock, gserial_disconnect cant mark >>>> it null until the spinlock is released. We need to check if the port->lock >>>> is valid before accessing it, otherwise it can lead to the above mentioned >>>> scenario >>> >>> What happens if gserial_disconnect sets gser->port to NULL immediately >>> after your new check occurs, but before >>> spinlock_irq_save(&port->port_lock) gets called? >>> >>> You may need to add a static spinlock to prevent this from happening. >>> >>> Alan Stern >> In that case i guess we have to make port_lock a global variable and take it >> out of gs_port structure. >> >> + static DEFINE_SPINLOCK(port_lock); >> >> struct gs_port { >> struct tty_port port; >> - spinlock_t port_lock; >> >> This will require us to change all the spinlock(port->port_lock) used in >> u_serial.c, what do you suggest? > > Yes, that would be the correct thing to do. will do it and share next patch Thanks for the suggestions Prashanth K
On 09-02-23 12:33 pm, Prashanth K wrote: > > > On 09-02-23 12:31 pm, Greg Kroah-Hartman wrote: >> On Thu, Feb 09, 2023 at 10:31:50AM +0530, Prashanth K wrote: >>> >>> >>> On 09-02-23 01:51 am, Alan Stern wrote: >>>> On Wed, Feb 08, 2023 at 09:15:54PM +0530, Prashanth K wrote: >>>>> >>>>> >>>>> On 08-02-23 08:24 pm, Greg Kroah-Hartman wrote: >>>>>> On Wed, Feb 08, 2023 at 07:24:47PM +0530, Prashanth K wrote: >>>>>>> Consider a case where gserial_disconnect has already cleared >>>>>>> gser->ioport. And if a wakeup interrupt triggers afterwards, >>>>>>> gserial_resume gets called, which will lead to accessing of >>>>>>> gserial->port and thus causing null pointer dereference.Add >>>>>>> a null pointer check to prevent this. >>>>>>> >>>>>>> Fixes: aba3a8d01d62 (" usb: gadget: u_serial: add suspend resume >>>>>>> callbacks") >>>>>> >>>>>> Nit, and our tools will complain, no " " before the "usb:" string >>>>>> here, >>>>>> right? >>>>>> >>>>> Will fix it in next patch. >>>>>> >>>>>> >>>>>>> Signed-off-by: Prashanth K <quic_prashk@quicinc.com> >>>>>>> --- >>>>>>> drivers/usb/gadget/function/u_serial.c | 3 +++ >>>>>>> 1 file changed, 3 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/usb/gadget/function/u_serial.c >>>>>>> b/drivers/usb/gadget/function/u_serial.c >>>>>>> index 840626e..98be2b8 100644 >>>>>>> --- a/drivers/usb/gadget/function/u_serial.c >>>>>>> +++ b/drivers/usb/gadget/function/u_serial.c >>>>>>> @@ -1428,6 +1428,9 @@ void gserial_resume(struct gserial *gser) >>>>>>> struct gs_port *port = gser->ioport; >>>>>>> unsigned long flags; >>>>>>> + if (!port) >>>>>>> + return; >>>>>>> + >>>>>> >>>>>> What prevents port from going to NULL right after this check? >>>>> In our case we got a null pointer de-reference while performing USB >>>>> compliance tests, as the gser->port was null. Because in >>>>> gserial_resume, >>>>> spinlock_irq_save(&port->port_lock) accesses a null-pointer as port >>>>> was >>>>> already marked null by gserial_disconnect. >>>>> >>>>> And after gserial_resume acquires the spinlock, gserial_disconnect >>>>> cant mark >>>>> it null until the spinlock is released. We need to check if the >>>>> port->lock >>>>> is valid before accessing it, otherwise it can lead to the above >>>>> mentioned >>>>> scenario >>>> >>>> What happens if gserial_disconnect sets gser->port to NULL immediately >>>> after your new check occurs, but before >>>> spinlock_irq_save(&port->port_lock) gets called? >>>> >>>> You may need to add a static spinlock to prevent this from happening. >>>> >>>> Alan Stern >>> In that case i guess we have to make port_lock a global variable and >>> take it >>> out of gs_port structure. >>> >>> + static DEFINE_SPINLOCK(port_lock); >>> >>> struct gs_port { >>> struct tty_port port; >>> - spinlock_t port_lock; >>> >>> This will require us to change all the spinlock(port->port_lock) used in >>> u_serial.c, what do you suggest? >> >> Yes, that would be the correct thing to do. Hi Greg/Alan, One general doubt, if we make the spinlock static/global, wouldn't that be a problem when there are multiple instances, and also multiple interfaces can use u_serial at same time. Asking this because u_serial can be used by f_serial (gser) as well as f_acm (acm). Thanks Prahanth K > will do it and share next patch > > Thanks for the suggestions > Prashanth K
On Thu, Feb 09, 2023 at 07:37:01PM +0530, Prashanth K wrote: > > > On 09-02-23 12:33 pm, Prashanth K wrote: > > > > > > On 09-02-23 12:31 pm, Greg Kroah-Hartman wrote: > > > On Thu, Feb 09, 2023 at 10:31:50AM +0530, Prashanth K wrote: > > > > In that case i guess we have to make port_lock a global variable > > > > and take it > > > > out of gs_port structure. > > > > > > > > + static DEFINE_SPINLOCK(port_lock); > > > > > > > > struct gs_port { > > > > struct tty_port port; > > > > - spinlock_t port_lock; > > > > > > > > This will require us to change all the spinlock(port->port_lock) used in > > > > u_serial.c, what do you suggest? > > > > > > Yes, that would be the correct thing to do. > Hi Greg/Alan, One general doubt, if we make the spinlock static/global, > wouldn't that be a problem when there are multiple instances, and also > multiple interfaces can use u_serial at same time. Asking this because > u_serial can be used by f_serial (gser) as well as f_acm (acm). You should consider having _two_ spinlocks: One in the gs_port structure (the way it is now) and a separate global lock. The first would be used in situations where you know you have a valid pointer. The second would be used in situations where you don't know if the pointer is non-NULL or where you are changing the pointer's value. Alan Stern
On 09-02-23 08:39 pm, Alan Stern wrote: > On Thu, Feb 09, 2023 at 07:37:01PM +0530, Prashanth K wrote: >> >> >> On 09-02-23 12:33 pm, Prashanth K wrote: >>> >>> >>> On 09-02-23 12:31 pm, Greg Kroah-Hartman wrote: >>>> On Thu, Feb 09, 2023 at 10:31:50AM +0530, Prashanth K wrote: >>>>> In that case i guess we have to make port_lock a global variable >>>>> and take it >>>>> out of gs_port structure. >>>>> >>>>> + static DEFINE_SPINLOCK(port_lock); >>>>> >>>>> struct gs_port { >>>>> struct tty_port port; >>>>> - spinlock_t port_lock; >>>>> >>>>> This will require us to change all the spinlock(port->port_lock) used in >>>>> u_serial.c, what do you suggest? >>>> >>>> Yes, that would be the correct thing to do. >> Hi Greg/Alan, One general doubt, if we make the spinlock static/global, >> wouldn't that be a problem when there are multiple instances, and also >> multiple interfaces can use u_serial at same time. Asking this because >> u_serial can be used by f_serial (gser) as well as f_acm (acm). > > You should consider having _two_ spinlocks: One in the gs_port structure > (the way it is now) and a separate global lock. The first would be used > in situations where you know you have a valid pointer. The second would > be used in situations where you don't know if the pointer is non-NULL > or where you are changing the pointer's value. Lets say we replaced the existing spinlock in gserial_resume and gserial_disconnect with a new static spinlock, and kept the spinlocks in other functions unchanged. In that case, wouldn't it cause additional race conditions as we are using 2 different locks. Thanks, Prashanth K > > Alan Stern
On Thu, Feb 09, 2023 at 09:13:37PM +0530, Prashanth K wrote: > > > On 09-02-23 08:39 pm, Alan Stern wrote: > > You should consider having _two_ spinlocks: One in the gs_port structure > > (the way it is now) and a separate global lock. The first would be used > > in situations where you know you have a valid pointer. The second would > > be used in situations where you don't know if the pointer is non-NULL > > or where you are changing the pointer's value. > Lets say we replaced the existing spinlock in gserial_resume and > gserial_disconnect with a new static spinlock, and kept the spinlocks in > other functions unchanged. In that case, wouldn't it cause additional race > conditions as we are using 2 different locks. Not race conditions, but possibilities for deadlock. Indeed, you would have to be very careful about avoiding deadlock scenarios. In particular, you would have to ensure that the code never tries to acquire the global spinlock while already holding one of the per-port spinlocks. Alan Stern
On 09-02-23 09:33 pm, Alan Stern wrote: > On Thu, Feb 09, 2023 at 09:13:37PM +0530, Prashanth K wrote: >> >> >> On 09-02-23 08:39 pm, Alan Stern wrote: >>> You should consider having _two_ spinlocks: One in the gs_port structure >>> (the way it is now) and a separate global lock. The first would be used >>> in situations where you know you have a valid pointer. The second would >>> be used in situations where you don't know if the pointer is non-NULL >>> or where you are changing the pointer's value. >> Lets say we replaced the existing spinlock in gserial_resume and >> gserial_disconnect with a new static spinlock, and kept the spinlocks in >> other functions unchanged. In that case, wouldn't it cause additional race >> conditions as we are using 2 different locks. > > Not race conditions, but possibilities for deadlock. > > Indeed, you would have to be very careful about avoiding deadlock > scenarios. In particular, you would have to ensure that the code never > tries to acquire the global spinlock while already holding one of the > per-port spinlocks. > > Alan Stern Hi Alan, instead of doing these and causing potential regressions, can we just have the null pointer check which i suggested in the beginning? The major concern was that port might become null after the null pointer check. We mark gser->ioport as null pointer in gserial_disconnect, and in gserial_resume we copy the gser->ioport to *port in the beginning. struct gs_port *port = gser->ioport; And hence it wont cause null pointer deref after the check as we don't de-reference anything from gser->ioport afterwards. We only use the local pointer *port afterwards. Thanks, Prashanth K
On Thu, Feb 09, 2023 at 11:57:17PM +0530, Prashanth K wrote: > > > On 09-02-23 09:33 pm, Alan Stern wrote: > > On Thu, Feb 09, 2023 at 09:13:37PM +0530, Prashanth K wrote: > > > > > > > > > On 09-02-23 08:39 pm, Alan Stern wrote: > > > > You should consider having _two_ spinlocks: One in the gs_port structure > > > > (the way it is now) and a separate global lock. The first would be used > > > > in situations where you know you have a valid pointer. The second would > > > > be used in situations where you don't know if the pointer is non-NULL > > > > or where you are changing the pointer's value. > > > Lets say we replaced the existing spinlock in gserial_resume and > > > gserial_disconnect with a new static spinlock, and kept the spinlocks in > > > other functions unchanged. In that case, wouldn't it cause additional race > > > conditions as we are using 2 different locks. > > > > Not race conditions, but possibilities for deadlock. > > > > Indeed, you would have to be very careful about avoiding deadlock > > scenarios. In particular, you would have to ensure that the code never > > tries to acquire the global spinlock while already holding one of the > > per-port spinlocks. > > > > Alan Stern > Hi Alan, instead of doing these and causing potential regressions, can we > just have the null pointer check which i suggested in the beginning? The > major concern was that port might become null after the null pointer check. What you are describing is a data race: gserial_disconnect() can write to gser->ioport at the same time that gserial_resume() reads from it. Unless you're working on a fast path -- which this isn't -- you should strive to avoid data races by using proper locking. That means adding the extra spinlock, or finding some other way to make these two accesses be mutually exclusive. With a little care you can ensure there won't be any regressions. Just do what I said above: Make sure the code never tries to acquire the global spinlock while already holding one of the per-port spinlocks. > We mark gser->ioport as null pointer in gserial_disconnect, and in > gserial_resume we copy the gser->ioport to *port in the beginning. > > struct gs_port *port = gser->ioport; > > And hence it wont cause null pointer deref after the check as we don't > de-reference anything from gser->ioport afterwards. We only use the local > pointer *port afterwards. You cannot depend on this to work the way you want. The compiler will optimize your source code, and one of the optimizations might be to eliminate the "port" variable entirely and replace it with gser->ioport. Alan Stern
On 10-02-23 02:35 am, Alan Stern wrote: > On Thu, Feb 09, 2023 at 11:57:17PM +0530, Prashanth K wrote: >> >> >> On 09-02-23 09:33 pm, Alan Stern wrote: >>> On Thu, Feb 09, 2023 at 09:13:37PM +0530, Prashanth K wrote: >>>> >>>> >>>> On 09-02-23 08:39 pm, Alan Stern wrote: >>>>> You should consider having _two_ spinlocks: One in the gs_port structure >>>>> (the way it is now) and a separate global lock. The first would be used >>>>> in situations where you know you have a valid pointer. The second would >>>>> be used in situations where you don't know if the pointer is non-NULL >>>>> or where you are changing the pointer's value. >>>> Lets say we replaced the existing spinlock in gserial_resume and >>>> gserial_disconnect with a new static spinlock, and kept the spinlocks in >>>> other functions unchanged. In that case, wouldn't it cause additional race >>>> conditions as we are using 2 different locks. >>> >>> Not race conditions, but possibilities for deadlock. >>> >>> Indeed, you would have to be very careful about avoiding deadlock >>> scenarios. In particular, you would have to ensure that the code never >>> tries to acquire the global spinlock while already holding one of the >>> per-port spinlocks. >>> >>> Alan Stern >> Hi Alan, instead of doing these and causing potential regressions, can we >> just have the null pointer check which i suggested in the beginning? The >> major concern was that port might become null after the null pointer check. > > What you are describing is a data race: gserial_disconnect() can write > to gser->ioport at the same time that gserial_resume() reads from it. > Unless you're working on a fast path -- which this isn't -- you should > strive to avoid data races by using proper locking. That means adding > the extra spinlock, or finding some other way to make these two accesses > be mutually exclusive. > > With a little care you can ensure there won't be any regressions. Just > do what I said above: Make sure the code never tries to acquire the > global spinlock while already holding one of the per-port spinlocks. > >> We mark gser->ioport as null pointer in gserial_disconnect, and in >> gserial_resume we copy the gser->ioport to *port in the beginning. >> >> struct gs_port *port = gser->ioport; >> >> And hence it wont cause null pointer deref after the check as we don't >> de-reference anything from gser->ioport afterwards. We only use the local >> pointer *port afterwards. > > You cannot depend on this to work the way you want. The compiler will > optimize your source code, and one of the optimizations might be to > eliminate the "port" variable entirely and replace it with gser->ioport. > > Alan Stern Hi Alan, Thanks for the detailed info. I checked and included few cases here. This would cause a deadlock if gserial_disconnect acquires port_lock and gserial_resume acquires static_lock. gserial_disconnect { spin_lock(port) ... spin_lock(static) gser->ioport = NULL; spin_unlock(static) ... spin_unlock(port) } gserial_resume { struct gs_port *port = gser->ioport; spin_lock(static) if (!port) return spin_lock(port) spin_unlock(static) ... spin_unlock(port) } ------------------------------------------------------------------ This would cause additional races when gserial_disconnect releases port_lock and some other functions acquire it. gserial_disconnect { spin_lock(port) ... spin_unlock(port) spin_lock(static) gser->ioport = NULL; spin_unlock(static) spin_lock(port) ... spin_unlock(port) } gserial_resume { struct gs_port *port = gser->ioport; spin_lock(static) if (!port) return spin_lock(port) spin_unlock(static) ... spin_unlock(port) } ------------------------------------------------------------------ And this seems like a viable option to me, what do you suggest? gserial_disconnect { spin_lock(static) spin_lock(port) ... gser->ioport = NULL; ... spin_lock(port) spin_unlock(static) } gserial_resume { struct gs_port *port = gser->ioport; spin_lock(static) if (!port) return spin_lock(port) ... spin_unlock(port) spin_unlock(static) } Thanks, Prashanth K
> And this seems like a viable option to me, what do you suggest? > > gserial_disconnect { > spin_lock(static) > spin_lock(port) > ... > gser->ioport = NULL; > ... > spin_lock(port) > spin_unlock(static) > > } > > gserial_resume { > struct gs_port *port = gser->ioport; > > spin_lock(static) > if (!port) spin_unlock(static) > return > spin_lock(port) > > ... > spin_unlock(port) > spin_unlock(static) > } > > Thanks, > Prashanth K Small correction inlined.
On Fri, Feb 10, 2023 at 12:26:52PM +0530, Prashanth K wrote: > > > > And this seems like a viable option to me, what do you suggest? > > > > gserial_disconnect { > > spin_lock(static) > > spin_lock(port) > > ... > > gser->ioport = NULL; > > ... > > spin_lock(port) > > spin_unlock(static) > > > > } > > > > gserial_resume { > > struct gs_port *port = gser->ioport; > > > > spin_lock(static) > > if (!port) > spin_unlock(static) > > return > > spin_lock(port) If you want, you could move the spin_unlock(static) up to here. It probably doesn't matter. > > > > ... > > spin_unlock(port) > > spin_unlock(static) > > } I agree, that should work fine. Alan Stern
On 10-02-23 09:17 pm, Alan Stern wrote: > > I agree, that should work fine. > > Alan Stern Thanks Alan for the help! will create v2 patch.
diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c index 840626e..98be2b8 100644 --- a/drivers/usb/gadget/function/u_serial.c +++ b/drivers/usb/gadget/function/u_serial.c @@ -1428,6 +1428,9 @@ void gserial_resume(struct gserial *gser) struct gs_port *port = gser->ioport; unsigned long flags; + if (!port) + return; + spin_lock_irqsave(&port->port_lock, flags); port->suspended = false; if (!port->start_delayed) {
Consider a case where gserial_disconnect has already cleared gser->ioport. And if a wakeup interrupt triggers afterwards, gserial_resume gets called, which will lead to accessing of gserial->port and thus causing null pointer dereference.Add a null pointer check to prevent this. 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 | 3 +++ 1 file changed, 3 insertions(+)