linux-kernel - Re: [PATCH v1 1/1] serial: core: Clearing the circular buffer before NULLifying it

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 for Android: free password hash cracker in your pocket

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

Message-ID: <ZhB9M8C9IhXtJIXR@smile.fi.intel.com>
Date: Sat, 6 Apr 2024 01:37:39 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Jiri Slaby <jirislaby@...nel.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
	Yicong Yang <yangyicong@...ilicon.com>,
	Tony Lindgren <tony@...mide.com>,
	kernel test robot <oliver.sang@...el.com>
Subject: Re: [PATCH v1 1/1] serial: core: Clearing the circular buffer before
 NULLifying it

On Fri, Apr 05, 2024 at 06:17:54PM +0300, Andy Shevchenko wrote:
> On Fri, Apr 05, 2024 at 07:25:03AM +0200, Jiri Slaby wrote:
> > On 04. 04. 24, 16:59, Andy Shevchenko wrote:
> > > The circular buffer is NULLified in uart_tty_port_shutdown()
> > > under the spin lock. However, the PM or other timer based callbacks
> > > may still trigger after this event without knowning that buffer pointer
> > > is not valid. Since the serial code is a bit inconsistent in checking
> > > the buffer state (some rely on the head-tail positions, some on the
> > > buffer pointer), it's better to have both aligned, i.e. buffer pointer
> > > to be NULL and head-tail possitions to be the same, meaning it's empty.
> > > This will prevent asynchronous calls to dereference NULL pointer as
> > > reported recently in 8250 case:
> > > 
> > >    BUG: kernel NULL pointer dereference, address: 00000cf5
> > >    Workqueue: pm pm_runtime_work
> > >    EIP: serial8250_tx_chars (drivers/tty/serial/8250/8250_port.c:1809)
> > >    ...
> > >    ? serial8250_tx_chars (drivers/tty/serial/8250/8250_port.c:1809)
> > >    __start_tx (drivers/tty/serial/8250/8250_port.c:1551)
> > >    serial8250_start_tx (drivers/tty/serial/8250/8250_port.c:1654)
> > >    serial_port_runtime_suspend (include/linux/serial_core.h:667 drivers/tty/serial/serial_port.c:63)
> > >    __rpm_callback (drivers/base/power/runtime.c:393)
> > >    ? serial_port_remove (drivers/tty/serial/serial_port.c:50)
> > >    rpm_suspend (drivers/base/power/runtime.c:447)
> > 
> > Yeah, I noticed start_tx() is called repeatedly after shutdown() yesterday
> > too. So thanks for looking into this.
> 
> > And it's pretty weird. I think it's new with the runtime PM (sure, /me reads
> > Fixes: now). I am not sure if it is documented, but most of the code in tty/
> > assumes NO ordinary ->ops (like start_tx()) are called after shutdown().
> > Actually, to me it occurs like serial8250_start_tx() should not be called in
> > the first place. It makes no sense after all.
> > 
> > BTW cannot be x_char en/queued at that time too (the other check in the if)?
> > But again, serial8250_start_tx() should not be called after shutdown().
> 
> Yes, and I have no clue how we can check this as startup can be called again
> and so on. The PM callback is timer based AFAIU, meaning it may happen at any
> time.
> 
> But do you agree that this patch has value on its own?

FWIW, https://lore.kernel.org/all/0000000000009e2dd805ffc595a3@google.com/T/

-- 
With Best Regards,
Andy Shevchenko