netdev - RE: [net-next 6/6] can: mcp251xfd: mcp251xfd_regmap_crc_read(): work around broken CRC on TBC register

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:   Tue, 21 Dec 2021 22:24:52 +0000
From:   <Thomas.Kopp@...rochip.com>
To:     <pavel.modilaynen@...vocars.com>, <mkl@...gutronix.de>
CC:     <drew@...gleboard.org>, <linux-can@...r.kernel.org>,
        <menschel.p@...teo.de>, <netdev@...r.kernel.org>,
        <will@...china.cc>
Subject: RE: [net-next 6/6] can: mcp251xfd: mcp251xfd_regmap_crc_read(): work
 around broken CRC on TBC register

Hi Pavel,

> > > We have the similar CRC read errors but
> > > the lowest byte is not 0x00 and 0x80, it's actually 0x0x or 0x8x, e.g.
> > >
> > > mcp251xfd spi0.0 can0: CRC read error at address 0x0010 (length=4,
> > > data=82 d1 fa 6c, CRC=0xd9c2) retrying.
> > >
> > > 0xb0 0x10 0x04 0x82 0xd1 0xfa 0x6c => 0x59FD (not matching)
> > >
> > > but if I flip the first received bit  (highest bit in the lowest byte):
> > > 0xb0 0x10 0x04 0x02 0xd1 0xfa 0x6c => 0xD9C2 (matching!)
> 
> > What settings do you have on your setup? Can you please print the
> dmesg output from the init? I'm especially interested in Sysclk and SPI
> speed.
> 
> mcp251xfd spi0.0 can0: MCP2517FD rev0.0 (-RX_INT +MAB_NO_WARN
> +CRC_REG +CRC_RX +CRC_TX +ECC -HD c:40.00MHz m:10.00MHz
> r:10.00MHz e:10.00MHz) successfully initialized.

Thanks for the data. I've looked into this and it seems that the second bit being set in your case does not depend on the SPI-Rate (or the quirks for that matter) but it seems to be hardware setup related. 
I'm fine with changing the driver so that it ignores set LSBs but would limit it to 2 or 3 bits:
(buf_rx->data[0] == 0x0 || buf_rx->data[0] == 0x80))
becomes
((buf_rx->data[0] & 0xf8) == 0x0 || (buf_rx->data[0] & 0xf8) == 0x80)) {

The action also needs to be changed and the flip back of the bit needs to be removed. In this case the flipped databit that produces a matching CRC is actually  correct (i.e. consistent with the 7 LSBs in that byte.)
A patch could look like this (I'm currently not close to a setup where I can compile/test this.)

diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
index 297491516a26..e5bc897f37e8 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
@@ -332,12 +332,10 @@ mcp251xfd_regmap_crc_read(void *context,
                 *
                 * If the highest bit in the lowest byte is flipped
                 * the transferred CRC matches the calculated one. We
-                * assume for now the CRC calculation in the chip
-                * works on wrong data and the transferred data is
-                * correct.
+                * assume for now the CRC operates on the correct data.
                 */
                if (reg == MCP251XFD_REG_TBC &&
-                   (buf_rx->data[0] == 0x0 || buf_rx->data[0] == 0x80)) {
+                   ((buf_rx->data[0] & 0xF8) == 0x0 || (buf_rx->data[0] & 0xF8) == 0x80)) {
                        /* Flip highest bit in lowest byte of le32 */
                        buf_rx->data[0] ^= 0x80;

@@ -347,10 +345,8 @@ mcp251xfd_regmap_crc_read(void *context,
                                                                  val_len);
                        if (!err) {
                                /* If CRC is now correct, assume
-                                * transferred data was OK, flip bit
-                                * back to original value.
+                                * flipped data was OK.
                                 */
-                               buf_rx->data[0] ^= 0x80;
                                goto out;
                        }
                }

Thanks,
Thomas