Re: [PATCH] staging: fbtft: fix unaligned vmem writes when txbuf is byte-offset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Low] Missing `#include <linux/unaligned.h>` in `fbtft-bus.c`.

Pre-existing issues:
- [High] Unaligned memory stores in `define_fbtft_write_reg` macro when `startbyte` is used.
- [High] An integer underflow in `tx_array_size` leads to an out-of-bounds heap write if the `txbuflen` device property is set to a small value.
- [High] Missing NULL check for `par->txbuf.buf` in `fb_ra8875.c` leads to a NULL pointer dereference on Big Endian systems.
--

commit 57041cf8757e7586efa49957106ff7e0611f087c
Author: suryasaimadhu <suryasaimadhu369@gmail.com>

staging: fbtft: fix unaligned vmem writes when txbuf is byte-offset

This commit addresses unaligned memory access in the fbtft driver. It
updates write_vmem16_bus8() in both fb_ra8875.c and fbtft-bus.c to use
put_unaligned() when storing 16-bit pixel data.

> diff --git a/drivers/staging/fbtft/fb_ra8875.c b/drivers/staging/fbtft/fb_ra8875.c
> index 0ab1de6647d07..5b95b00953994 100644
> --- a/drivers/staging/fbtft/fb_ra8875.c
> +++ b/drivers/staging/fbtft/fb_ra8875.c
> @@ -10,6 +10,7 @@
>  #include <linux/delay.h>
>  
>  #include <linux/gpio/consumer.h>
> +#include <linux/unaligned.h>
>  #include "fbtft.h"
>  
>  #define DRVNAME "fb_ra8875"

[Severity: High]
This is a pre-existing issue, but does write_vmem16_bus8() have a potential
null pointer dereference here?

fb_ra8875.c:write_vmem16_bus8() {
    ...
    txbuf16 = par->txbuf.buf + 1;
    tx_array_size -= 2;
    *(u8 *)(par->txbuf.buf) = 0x00;
    ...
}

If the txbuflen device property is 0, par->txbuf.buf remains null on big
endian systems because the fallback allocation is gated by a little endian
check.

Could this cause a crash when assigning 0x00 unconditionally?


[Severity: High]
This is a pre-existing issue, but could tx_array_size underflow and cause
an out-of-bounds heap write?

fb_ra8875.c:write_vmem16_bus8() {
    ...
    tx_array_size = par->txbuf.len / 2;
    txbuf16 = par->txbuf.buf + 1;
    tx_array_size -= 2;
    ...
    while (remain) {
        to_copy = min(tx_array_size, remain);
    ...
}

If par->txbuf.len is set to a small value like 2 via the device tree,
tx_array_size evaluates to 1. Subtracting 2 causes it to underflow to a
huge value.

Would the min() check then just use remain, causing the loop to write past
the end of the allocated heap buffer?

> @@ -262,7 +263,7 @@ static int write_vmem16_bus8(struct fbtft_par *par, size_t offset, size_t len)
>  			to_copy, remain - to_copy);
>  
>  		for (i = 0; i < to_copy; i++)
> -			txbuf16[i] = cpu_to_be16(vmem16[i]);
> +			put_unaligned(cpu_to_be16(vmem16[i]), &txbuf16[i]);
>  
>  		vmem16 = vmem16 + to_copy;
>  		ret = par->fbtftops.write(par, par->txbuf.buf,
> diff --git a/drivers/staging/fbtft/fbtft-bus.c b/drivers/staging/fbtft/fbtft-bus.c
> index 30e436ff19e4c..9e25c19ec9ada 100644
> --- a/drivers/staging/fbtft/fbtft-bus.c
> +++ b/drivers/staging/fbtft/fbtft-bus.c

[Severity: High]
This is a pre-existing issue, but does the define_fbtft_write_reg macro
also suffer from unaligned stores?

fbtft-bus.c:define_fbtft_write_reg() {
    ...
    if (par->startbyte)
        *buf++ = par->startbyte | (((size & 0xF) + 1) << 4);
    ...
    *buf = modifier((data_type)va_arg(args, unsigned int));
    ...
}

If par->startbyte is used, buf is offset by 1 byte. If data_type is a
16-bit type, would this dereference create a misaligned store and fault
on strict-alignment architectures?


[Severity: High]
This is a pre-existing issue, but fbtft_write_vmem16_bus8() shares the same
integer underflow behavior as fb_ra8875.c:

fbtft-bus.c:fbtft_write_vmem16_bus8() {
    ...
    tx_array_size = par->txbuf.len / 2;
    if (par->startbyte) {
        ...
        tx_array_size -= 2;
    ...
    while (remain) {
        to_copy = min(tx_array_size, remain);
    ...
}

Could this similarly overwrite memory beyond the heap buffer if a small
txbuflen is provided?

> @@ -154,7 +154,7 @@ int fbtft_write_vmem16_bus8(struct fbtft_par *par, size_t offset, size_t len)
>  			to_copy, remain - to_copy);
>  
>  		for (i = 0; i < to_copy; i++)
> -			txbuf16[i] = cpu_to_be16(vmem16[i]);
> +			put_unaligned(cpu_to_be16(vmem16[i]), &txbuf16[i]);

[Severity: Low]
Does this file need to include linux/unaligned.h?

Since the header was included in fb_ra8875.c but not added here, could
this cause build failures on architectures where put_unaligned() isn't
transitively included by other headers?

>  
>  		vmem16 = vmem16 + to_copy;
>  		ret = par->fbtftops.write(par, par->txbuf.buf,

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