netdev - Re: [PATCH] can: hi311x: Acquire SPI lock on ->do_get_berr

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:   Wed, 9 May 2018 13:58:39 -0400
From:   Akshay Bhat <akshay.bhat@...esys.com>
To:     Lukas Wunner <lukas@...ner.de>,
        Marc Kleine-Budde <mkl@...gutronix.de>,
        Wolfgang Grandegger <wg@...ndegger.com>,
        linux-can@...r.kernel.org, netdev@...r.kernel.org
Cc:     Mathias Duckeck <m.duckeck@...bus.de>,
        Casey Fitzpatrick <casey.fitzpatrick@...esys.com>,
        Stef Walter <stefw@...hat.com>, Karel Zak <kzak@...hat.com>
Subject: Re: [PATCH] can: hi311x: Acquire SPI lock on ->do_get_berr_counter



On 05/09/2018 08:38 AM, Lukas Wunner wrote:
> hi3110_get_berr_counter() may run concurrently to the rest of the driver
> but neglects to acquire the lock protecting access to the SPI device.
> As a result, it and the rest of the driver may clobber each other's tx
> and rx buffers.
> 
> We became aware of this issue because transmission of packets with
> "cangen -g 0 -i -x" frequently hung.  It turns out that agetty executes
> ->do_get_berr_counter every few seconds via the following call stack:
> 
>     CPU: 2 PID: 1605 Comm: agetty
>     [<7f3f7500>] (hi3110_get_berr_counter [hi311x])
>     [<7f130204>] (can_fill_info [can_dev])
>     [<80693bc0>] (rtnl_fill_ifinfo)
>     [<806949ec>] (rtnl_dump_ifinfo)
>     [<806b4834>] (netlink_dump)
>     [<806b4bc8>] (netlink_recvmsg)
>     [<8065f180>] (sock_recvmsg)
>     [<80660f90>] (___sys_recvmsg)
>     [<80661e7c>] (__sys_recvmsg)
>     [<80661ec0>] (SyS_recvmsg)
>     [<80108b20>] (ret_fast_syscall+0x0/0x1c)
> 
> agetty listens to netlink messages in order to update the login prompt
> when IP addresses change (if /etc/issue contains \4 or \6 escape codes):
> https://git.kernel.org/pub/scm/utils/util-linux/util-linux.git/commit/?id=e36deb6424e8
> 
> It's a useful feature, though it seems questionable that it causes CAN
> bit error statistics to be queried.
> 
> Be that as it may, if hi3110_get_berr_counter() is invoked while a frame
> is sent by hi3110_hw_tx(), bogus SPI transfers like the following may
> occur:
> 
>     => 12 00             (hi3110_get_berr_counter() wanted to transmit
>                           EC 00 to query the transmit error counter,
>                           but the first byte was overwritten by
>                           hi3110_hw_tx_frame())
> 
>     => EA 00 3E 80 01 FB (hi3110_hw_tx_frame() wanted to transmit a
>                           frame, but the first byte was overwritten by
>                           hi3110_get_berr_counter() because it wanted
>                           to query the receive error counter)
> 
> This sequence hangs the transmission because the driver believes it has
> sent a frame and waits for the interrupt signaling completion, but in
> reality the chip has never sent away the frame since the commands it
> received were malformed.
> 
> Fix by acquiring the SPI lock in hi3110_get_berr_counter().
> 
> I've scrutinized the entire driver for further unlocked SPI accesses but
> found no others.
> 
> Cc: Mathias Duckeck <m.duckeck@...bus.de>
> Cc: Akshay Bhat <akshay.bhat@...esys.com>
> Cc: Casey Fitzpatrick <casey.fitzpatrick@...esys.com>
> Cc: Stef Walter <stefw@...hat.com>
> Cc: Karel Zak <kzak@...hat.com>
> Cc: stable@...r.kernel.org # v4.12+
> Signed-off-by: Lukas Wunner <lukas@...ner.de>
> ---

Reviewed-by: Akshay Bhat <akshay.bhat@...esys.com>