Re: [RFC PATCH v3 1/5] HID: spi-hid: isolate DMA transfers from protocol buffers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] HID: spi-hid: TX/RX overlap in spi_hid_input_sync() if ARCH_DMA_MINALIGN < 5
--

commit 62a7cf26838a2cb27b111ff47187ee064977f44a
Author: Jizhou Tong <fqwqf@fqwqf.xyz>

HID: spi-hid: isolate DMA transfers from protocol buffers

This commit allocates a separate DMA-aligned transfer buffer in struct
spi_hid and copies TX and RX protocol data through it under io_lock to
avoid DMA mapping unaligned or adjacent protocol buffers.

> diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
> index f2b21066003cb..db0432251732e 100644
> --- a/drivers/hid/spi-hid/spi-hid-core.c
> +++ b/drivers/hid/spi-hid/spi-hid-core.c
> @@ -193,14 +193,19 @@ static void spi_hid_populate_output_header(u8 *buf,
>  static int spi_hid_input_sync(struct spi_hid *shid, void *buf, u16 length,
>  			      bool is_header)
>  {
> +	u8 *rx = shid->xfer_buf + ARCH_DMA_MINALIGN;
>  	int error;
>  
> -	shid->input_transfer[0].tx_buf = is_header ?
> -					 shid->read_approval_header :
> -					 shid->read_approval_body;
> +	if (length > shid->xfer_size - ARCH_DMA_MINALIGN)
> +		return -EMSGSIZE;
> +
> +	memcpy(shid->xfer_buf, is_header ? shid->read_approval_header :
> +					 shid->read_approval_body,
> +	       SPI_HID_READ_APPROVAL_LEN);
> +	shid->input_transfer[0].tx_buf = shid->xfer_buf;
>  	shid->input_transfer[0].len = SPI_HID_READ_APPROVAL_LEN;
>  
> -	shid->input_transfer[1].rx_buf = buf;
> +	shid->input_transfer[1].rx_buf = rx;

[Severity: Medium]
Can shid->input_transfer[0].tx_buf and shid->input_transfer[1].rx_buf
overlap in spi_hid_input_sync() when ARCH_DMA_MINALIGN is smaller than
SPI_HID_READ_APPROVAL_LEN?

In drivers/hid/spi-hid/spi-hid-core.h, SPI_HID_READ_APPROVAL_LEN is 5:

#define SPI_HID_READ_APPROVAL_LEN		5

In include/linux/cache.h, architectures that do not define
ARCH_DMA_MINALIGN fall back to __alignof__(unsigned long long):

#ifdef ARCH_DMA_MINALIGN
#define ARCH_HAS_DMA_MINALIGN
#else
#define ARCH_DMA_MINALIGN __alignof__(unsigned long long)
#endif

On 32-bit architectures where 64-bit integers have 4-byte or 2-byte
alignment (such as OpenRISC or m68k), ARCH_DMA_MINALIGN evaluates to 4 or
2.

When spi_hid_dev_irq() calls spi_hid_input_sync(), the 5-byte read
approval is copied to shid->xfer_buf[0..4] for input_transfer[0].tx_buf,
while rx for input_transfer[1].rx_buf is placed at shid->xfer_buf +
ARCH_DMA_MINALIGN. If ARCH_DMA_MINALIGN is 4, both transfers in the same
spi_message share shid->xfer_buf[4] (and spi_hid_alloc_buffers() only
reserves a single ARCH_DMA_MINALIGN unit for the TX region in
shid->xfer_size).

Should spi_hid_alloc_buffers() and spi_hid_input_sync() use
ALIGN(SPI_HID_READ_APPROVAL_LEN, ARCH_DMA_MINALIGN) for the TX region size
and the rx offset instead of bare ARCH_DMA_MINALIGN?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/010601a1ad274a9c-ca2fc8f5-d3fb-435d-8572-c0d3eb701649-000000@ap-northeast-1.amazonses.com?part=1