Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Incorrect transfer size calculation in `define_fbtft_write_reg` causes an out-of-bounds read of `par->buf` and transmits uninitialized memory. - [High] Unaligned pointer dereference in `define_fbtft_write_reg` when `startbyte` is used with 16-bit data types. -- commit fc3573259bc5556dbb41e0e35027f130ca8f6b63 Author: Anshika Jain <[email protected]> staging: fbtft: fix len<=0 buffer overflow in define_fbtft_write_reg() If len is 0 or negative when a define_fbtft_write_reg()-generated function is called, len-- underflows to -1, causing the subsequent while (i--) loop to run approximately 2^31 times and write far past the end of buf. Link: https://sashiko.dev/#/patchset/[email protected]?part=1 > diff --git a/drivers/staging/fbtft/fbtft-bus.c > b/drivers/staging/fbtft/fbtft-bus.c > index 30e436ff19e4c..0927877249e74 100644 > --- a/drivers/staging/fbtft/fbtft-bus.c > +++ b/drivers/staging/fbtft/fbtft-bus.c > @@ -19,6 +19,9 @@ void func(struct fbtft_par *par, int len, ...) > \ > int offset = 0; \ > buffer_type *buf = (buffer_type *)par->buf; \ > \ > + if (len <= 0) \ > + return; \ > + \ [Severity: High] This is a pre-existing issue, but I noticed an unaligned pointer dereference when startbyte is used with 16-bit data types in define_fbtft_write_reg(). When par->startbyte is true, buf is assigned an offset address: if (par->startbyte) { *(u8 *)par->buf = par->startbyte; buf = (buffer_type *)(par->buf + 1); offset = 1; } Since par->buf is an aligned allocation, adding 1 byte produces an odd (unaligned) memory address. When buffer_type is a 16-bit type (e.g., __be16), operations later in the function perform unaligned 16-bit stores: *buf = modifier((data_type)va_arg(args, unsigned int)); ... *buf++ = modifier((data_type)va_arg(args, unsigned int)); On strict-alignment architectures, this could trigger an alignment fault and lead to a kernel panic. Should this use the put_unaligned() family of helpers instead? [Severity: High] This is also a pre-existing issue, but is the transfer size calculation in define_fbtft_write_reg() causing an out-of-bounds read of par->buf? The macro computes the buffer size to transmit via fbtft_write_buf_dc() as: fbtft_write_buf_dc(par, par->buf, len * (sizeof(data_type) + offset), 1); Because of the parentheses, this incorrectly multiplies the offset by the remaining length instead of calculating len * sizeof(data_type) + offset. For example, if the device tree enables the startbyte property for a display using a 16-bit write_reg macro, len can reach 64 during fbtft_init_display(). After decrementing, len = 63, offset = 1, and sizeof(u16) = 2. This results in calculating an outbound transmission of 189 bytes. Since par->buf is allocated as exactly 128 bytes, this reads past the end of the buffer. Could this leak adjacent SLUB objects to the SPI hardware and corrupt the display initialization sequence with garbage data? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
