linux-kernel - Re: [RFC PATCH 2/2] serial: sh-sci: always cancel hrtimer when DMA RX is invalidated

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] [day] [month] [year] [list]

Date: Thu, 2 May 2024 20:17:37 +0200
From: Wolfram Sang <wsa+renesas@...g-engineering.com>
To: Geert Uytterhoeven <geert@...ux-m68k.org>
Cc: linux-renesas-soc@...r.kernel.org, Dirk Behme <dirk.behme@...bosch.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jirislaby@...nel.org>,
	Geert Uytterhoeven <geert+renesas@...der.be>,
	Aleksandar Mitev <amitev@...teon.com>, linux-kernel@...r.kernel.org,
	linux-serial@...r.kernel.org
Subject: Re: [RFC PATCH 2/2] serial: sh-sci: always cancel hrtimer when DMA
 RX is invalidated

Hi Geert,

good news, I was able to trigger the DMA rx code path. I dunno what I
did wrong last time. I started from scratch again and it worked easily
by dd-ing random data to the second non-console debug port.

> I do think you need to cancel the timer: even when not restarting
> the timer in sci_dma_rx_complete() due to a DMA failure, the previous
> timer may still be running, and will cause a NULL pointer dereference
> on s->chan_rx on timer expiry.

Taking locking into account, I think this patch is bogus. If we run into
this NULL-pointer, we have a locking problem and cancelling the timer in
sci_dma_rx_chan_invalidate() is not going to fix the underlying locking
problem. sci_dma_rx_chan_invalidate() does not only clear the pointer
but also the cookie_rx-array. sci_dma_rx_timer_fn() bails out via
sci_dma_rx_find_active() if that array is cleared. It does so before
accessing the chan_rx-pointer. So, it looks to me that should work once
all calls to sci_dma_rx_chan_invalidate() are protected. And there is
one path where this is not true, namely via sci_dma_rx_release() during
shutdown. This is why I asked Dirk if the system was about to shutdown.
Currently, I don't see any other problematic code path.

> > -#ifdef CONFIG_SERIAL_SH_SCI_DMA
> > -       if (s->chan_rx_saved) {
> > -               dev_dbg(port->dev, "%s(%d) deleting rx_timer\n", __func__,
> > -                       port->line);
> > -               hrtimer_cancel(&s->rx_timer);
> > -       }
> > -#endif

Also, this chunk needs to stay. I suggested in patch 1 to cancel the
timer on successful dma_rx_complete, so the timer only runs when a DMA
is in progress. Then, of course, we need to cancel it in shutdown.

I hope I am not seeing "no wood for the trees" by now. I am not
convinced that I actually found Dirk's race condition yet...

All the best,

   Wolfram

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)