Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Incorrect buffer size calculation causes out-of-bounds read and information leak over SPI when `startbyte` is used. - [High] Pointer arithmetic creates an unaligned pointer for 16-bit register writes when `startbyte` is used, causing an unaligned memory access. -- commit 5efb24fc5a4ff6b0f2481a7887082d742684defc Author: suryasaimadhu <[email protected]> staging: fbtft: replace empty modifier argument with no-op macro This commit defines an identity function fbtft_write_reg_no_modifier() and uses it to replace empty modifier arguments in define_fbtft_write_reg() macro calls to fix checkpatch errors. > diff --git a/drivers/staging/fbtft/fbtft-bus.c > b/drivers/staging/fbtft/fbtft-bus.c > index 30e436ff19e4c..2169f8d1d1b83 100644 > --- a/drivers/staging/fbtft/fbtft-bus.c > +++ b/drivers/staging/fbtft/fbtft-bus.c > @@ -11,6 +11,7 @@ > * > > *****************************************************************************/ > > +#define fbtft_write_reg_no_modifier(x) (x) > #define define_fbtft_write_reg(func, buffer_type, data_type, modifier) > \ > void func(struct fbtft_par *par, int len, ...) > \ > { > \ [Severity: High] This is a pre-existing issue, but does this pointer arithmetic create an unaligned memory access on strict-alignment architectures? When par->startbyte is non-zero, buf is advanced by one byte in define_fbtft_write_reg(): if (par->startbyte) { *(u8 *)par->buf = par->startbyte; buf = (buffer_type *)(par->buf + 1); offset = 1; } Since buffer_type can be a 16-bit type like u16 or __be16 (for example, in fbtft_write_reg16_bus8 or fbtft_write_reg16_bus16), adding 1 byte to the aligned par->buf creates an odd address. The code then dereferences this unaligned pointer: *buf = modifier((data_type)va_arg(args, unsigned int)); Could this trigger an alignment fault resulting in a kernel panic on architectures that require strict alignment? [ ... ] > @@ -62,9 +63,9 @@ out: > \ > } > \ > EXPORT_SYMBOL(func); > > -define_fbtft_write_reg(fbtft_write_reg8_bus8, u8, u8, ) > +define_fbtft_write_reg(fbtft_write_reg8_bus8, u8, u8, > fbtft_write_reg_no_modifier) > define_fbtft_write_reg(fbtft_write_reg16_bus8, __be16, u16, cpu_to_be16) > -define_fbtft_write_reg(fbtft_write_reg16_bus16, u16, u16, ) > +define_fbtft_write_reg(fbtft_write_reg16_bus16, u16, u16, > fbtft_write_reg_no_modifier) [Severity: High] This is a pre-existing issue, but is there an out-of-bounds read when calculating the length to send over SPI? When startbyte is configured, offset is set to 1. The macro define_fbtft_write_reg() uses the following size calculation for the final transfer: fbtft_write_buf_dc(par, par->buf, len * (sizeof(data_type) + offset), 1); Because the parenthesis includes both sizeof(data_type) and offset, the size grows too quickly. For example, with a len of 63, a 16-bit data_type, and an offset of 1, the size evaluates to 63 * (2 + 1) = 189 bytes. Does this result in reading past the end of par->buf and sending uninitialized kernel heap memory to the display controller, potentially causing a kernel panic if it crosses an unmapped page boundary? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
