Re: [PATCH] staging: fbtft: fix unaligned access and txbuf safety issues

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Hardware protocol violation in unbuffered SPI write fallback paths.
--

commit 3d08e05dccbf9cbe108bf3c042c710ace7899fe1
Author: suryasaimadhu <suryasaimadhu369@gmail.com>

staging: fbtft: fix unaligned access and txbuf safety issues

This commit fixes unaligned access issues during register writes and pixel
data copies in the fbtft driver. It also adds bounds checking and fallbacks
for when the transmit buffer size underflows or is missing.

> diff --git a/drivers/staging/fbtft/fb_ra8875.c b/drivers/staging/fbtft/fb_ra8875.c
> index 0ab1de6647d07..06f650aacc204 100644
> --- a/drivers/staging/fbtft/fb_ra8875.c
> +++ b/drivers/staging/fbtft/fb_ra8875.c
> @@ -250,7 +251,14 @@ static int write_vmem16_bus8(struct fbtft_par *par, size_t offset, size_t len)
>  
>  	remain = len / 2;
>  	vmem16 = (u16 *)(par->info->screen_buffer + offset);
> +
> +	if (!par->txbuf.buf)
> +		return par->fbtftops.write(par, vmem16, len);
> +
>  	tx_array_size = par->txbuf.len / 2;
> +	if (tx_array_size <= 2)
> +		return par->fbtftops.write(par, vmem16, len);
> +

[Severity: High]
Does this fallback bypass the 0x00 command prefix required by the RA8875
hardware?

Since vmem16 points directly to the framebuffer memory, this direct write
will omit the prefix, which might cause the display controller to
misinterpret the first pixel as a command.

Additionally, could this skip the cpu_to_be16() conversion on
little-endian CPUs, sending pixels in the wrong byte order?

Would it be safer to return an error if the required transmission buffer
is unavailable or too small to prepend the hardware protocol prefix?

>  	txbuf16 = par->txbuf.buf + 1;
>  	tx_array_size -= 2;
>  	*(u8 *)(par->txbuf.buf) = 0x00;

[ ... ]

> diff --git a/drivers/staging/fbtft/fbtft-bus.c b/drivers/staging/fbtft/fbtft-bus.c
> index 30e436ff19e4c..52a0c5c0cbaed 100644
> --- a/drivers/staging/fbtft/fbtft-bus.c
> +++ b/drivers/staging/fbtft/fbtft-bus.c

[ ... ]

> @@ -142,19 +146,24 @@ int fbtft_write_vmem16_bus8(struct fbtft_par *par, size_t offset, size_t len)
>  	tx_array_size = par->txbuf.len / 2;
>  
>  	if (par->startbyte) {
> +		if (tx_array_size <= 2)
> +			return par->fbtftops.write(par, vmem16, len);

[Severity: High]
Does this skip prepending the par->startbyte prefix when the buffer is
too small, despite the prefix being explicitly required?

Like the RA8875 case above, this fallback bypasses both the command
prefix and endianness conversions.

>  		txbuf16 = par->txbuf.buf + 1;
>  		tx_array_size -= 2;
>  		*(u8 *)(par->txbuf.buf) = par->startbyte | 0x2;
>  		startbyte_size = 1;
>  	}
>  
> +	if (!tx_array_size)
> +		return par->fbtftops.write(par, vmem16, len);
> +

[Severity: High]
Could this result in the same issue of bypassing necessary prefixes and
endianness conversions if tx_array_size evaluates to zero?

Should we return an error instead of silently sending incorrectly
formatted data?

>  	while (remain) {
>  		to_copy = min(tx_array_size, remain);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625114215.325973-1-suryasaimadhu369@gmail.com?part=1