linux-kernel - Re: [PATCH] usb: gadget: u_serial: Add null pointer check in gserial

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]

Date:   Thu, 9 Feb 2023 16:05:33 -0500
From:   Alan Stern <stern@...land.harvard.edu>
To:     Prashanth K <quic_prashk@...cinc.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        Xiu Jianfeng <xiujianfeng@...wei.com>,
        Pratham Pratap <quic_ppratap@...cinc.com>,
        Jack Pham <quic_jackp@...cinc.com>, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] usb: gadget: u_serial: Add null pointer check in
 gserial_resume

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