Unsafe buffer access in AXI SPI Engine offload

[dlech]

This applies to offload support that hasn't been upstreamed yet.

In spi_engine_offload_load_msg(), the tx buffer of each spi xfer is copied to the SPI engine memory-mapped registers.

linux/drivers/spi/spi-axi-spi-engine.c

Lines 364 to 370 in b8d0e51

	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
	if (!xfer->tx_buf)
	continue;
	buf = xfer->tx_buf;
	for (i = 0; i < xfer->len; i++, j++)
	writel(buf[i], sdo_addr);
	}

According to the SPI framework, xfer->len is the length of xfer->tx_buf in bytes. However the loop is iterating buf which casts xfer->tx_buf (a byte array) to a u32 array. There are two problems with this.

1. If xfer->tx_buf is not aligned to 4 bytes, we get unaligned memory access.
2. It is reading past the end of the buffer by 4x because the for loop is iterating the count in bytes without dividing by 4.

This is further complicated by #2299 which modifies xfer->len, so if we attempt to fix this issue without addressing #2299 first, then we will have a bug that not all of the tx buffer will be copied.

[dlech]

It also looks like there is potentially another bug here. j is written but never used. I'm guessing it is supposed to be writel(buf[i], sdo_addr + j);

[dlech]

Also, if there are multiple xfers, e.g. of len=2, this would make the data in sdo_addr not contiguous between xfers. Is this going to cause a problem? I'm guessing so far consumers of this API are only using a single xfer.

[nunojsa]

According to the SPI framework, xfer->len is the length of xfer->tx_buf in bytes.

Yeah, I think len in the offload context does not really mean the same thing. I think it's basically the number of command transfers to read/write. Of course this loop is just making assumptions the caller got it right... And that tx_buf is properly aligned. Speaking on alignment:

linux/drivers/spi/spi-axi-spi-engine.c

Line 373 in b8d0e51

writel(p->instructions[i], cmd_addr);

This also implicitly casts u16 -> u32

So yeah, the offload support is indeed very messy and and really needs a proper cleanup. As I was saying last week, IMO, we need to have this API's going through the SPI subsystem. Having it "open coded" like this and freely "reinventing" things like xfer->len is just wrong... Ideally, we would have a new specific controller op for spi_engine_offload_load_msg() and the API calling that op, should run __spi_validate() on the message. I'm not sure if __spi_validate() needs any kind of specific support for the kind of devices we support (for example, we have some devices that due to having multiple SDO lines, we end up with bits_per_word of 12 and things like that).

It also looks like there is potentially another bug here. j is written but never used. I'm guessing it is supposed to be writel(buf[i], sdo_addr + j);

Also, if there are multiple xfers, e.g. of len=2, this would make the data in sdo_addr not contiguous between xfers. Is this going to cause a problem? I'm guessing so far consumers of this API are only using a single xfer.

For the above two comments, I think the HW is actually auto incrementing so you just end up writing on the same MMIO address and the HW makes it right. Looking at this:

linux/drivers/spi/spi-axi-spi-engine.c

Line 373 in b8d0e51

writel(p->instructions[i], cmd_addr);

made me wonder how was this working at all... Then, I looked at the hdl code and this seems to be the trick:

https://github.com/analogdevicesinc/hdl/blob/master/library/spi_engine/spi_engine_offload/spi_engine_offload.v#L284

combined with:

https://github.com/analogdevicesinc/hdl/blob/master/library/spi_engine/spi_engine_offload/spi_engine_offload.v#L301

But yeah, my hdl knowledge is very limited so I might be misreading this at all :)

One thing that came to my attention was the SPI_ENGINE_REG_OFFLOAD_SDO_MEM() macro. Seems to imply that we could have multiple SDO banks of memory but we only ever use 0 as argument. So, no idea what was the original intent.

[dlech]

Ideally, we would have a new specific controller op for spi_engine_offload_load_msg() and the API calling that op, should run __spi_validate() on the message.

Agreed. I came to the same conclusion.

One thing that came to my attention was the SPI_ENGINE_REG_OFFLOAD_SDO_MEM() macro. Seems to imply that we could have multiple SDO banks of memory but we only ever use 0 as argument. So, no idea what was the original intent.

The SPI engine wiki pages say that each engine could have more than one offload, so that is probably what it is for. Writing to the same same register for offload number 0 as a sort of FIFO makes sense now.

[nunojsa]

The SPI engine wiki pages say that each engine could have more than one offload, so that is probably what it is for

Hmm that makes sense... I think we don't have any project using more than one offload block so I would likely keep it simple for now.