Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider: - [High] drm/tiny: ra8875: sleep in atomic context in display update/enable paths - [High] drm/tiny: ra8875: unaligned memory access in ra8875_pipe_update() - [High] drm/tiny: ra8875: heap buffer overflow in txbuf due to unvalidated device tree videomode - [High] drm/tiny: ra8875: DMA on stack memory via local SPI transfer buffers - [High] drm/tiny: ra8875: Use-After-Free of txbuf due to missing drm_dev_unplug() - [Medium] drm/tiny: ra8875: failure to chunk large SPI transfers in display update - [Medium] drm/tiny: ra8875: silent mode validation failure inside .enable callback --
commit d7d5e9662b2beb4a2ef49392bd30bcd603e994b4 Author: Adam Azuddin <[email protected]> drm/tiny: Add RAiO RA8875 display controller driver Add a DRM tiny driver for the RAiO RA8875 SPI-connected TFT display controller. The driver supports display resolutions up to 800x480 and uses the GEM shmem helper for buffer management. > diff --git a/drivers/gpu/drm/tiny/ra8875.c b/drivers/gpu/drm/tiny/ra8875.c > new file mode 100644 > index 0000000000000..3ed4617ee761d > --- /dev/null > +++ b/drivers/gpu/drm/tiny/ra8875.c [ ... ] > +static int ra8875_write_reg(struct ra8875_device *ra8875, u8 reg, u8 val) > +{ > + int ret; > + u8 cmd[2] = { 0x80, reg }; > + u8 data[2] = { 0x00, val }; > + > + struct spi_transfer t_cmd = { > + .tx_buf = cmd, > + .len = 2, > + .speed_hz = 1000000, > + .cs_change = 1, /* pull CS high after this transfer */ > + }; [Severity: High] Is it safe to use stack-allocated buffers for SPI transfers here? The SPI subsystem often maps these buffers for DMA, and passing stack memory is forbidden in the kernel (especially with CONFIG_VMAP_STACK) because the memory might not be physically contiguous or cache-coherent. This pattern appears to repeat with the rx/tx buffers in ra8875_read_reg() and ra8875_pipe_update(). [ ... ] > +static int ra8875_hw_init(struct ra8875_device *ra8875, > + struct drm_display_mode *mode) > +{ > + u8 hdwr, hndr, hstr, hpwr, vpwr; > + u16 vdhr, vndr, vstr; > + u8 readback; > + int ret; > + > + /* Soft Reset Sequence */ > + ret = ra8875_write_reg(ra8875, RA8875_PWRR, RA8875_PWRR_SOFTRST); > + if (ret) { > + dev_err(ra8875->drm.dev, "Soft Reset failed\n"); > + return ret; > + } > + usleep_range(1000, 2000); [Severity: High] Can this sleep in atomic context? This initialization function is called from ra8875_pipe_enable(), which is invoked by the DRM core during an atomic commit. The display subsystem guidelines state that CRTC enable callbacks run in atomic context where sleeping functions like usleep_range() are forbidden and can cause system hangs or deadlocks. [ ... ] > +static void ra8875_pipe_enable(struct drm_simple_display_pipe *pipe, > + struct drm_crtc_state *crtc_state, > + struct drm_plane_state *plane_state) > +{ > + struct ra8875_device *ra8875 = > + container_of(pipe->crtc.dev, struct ra8875_device, drm); > + struct drm_display_mode *mode = &crtc_state->mode; > + int ret; > + > + if (mode->hdisplay % 8 != 0 || mode->hsync_start % 8 != 0 || > + mode->hsync_end % 8 != 0 || mode->htotal % 8 != 0) { > + drm_err(&ra8875->drm, > + "mode timings must be 8-pixel aligned (hdisplay=%d > hsync_start=%d hsync_end=%d htotal=%d)\n", > + mode->hdisplay, mode->hsync_start, mode->hsync_end, > + mode->htotal); > + return; > + } [Severity: Medium] Does checking the mode validity here leave the pipeline in an inconsistent state if it fails? Since this returns void from the .enable callback, the DRM core cannot catch the error and will assume the CRTC is successfully enabled. Should this validation be moved to the .atomic_check callback instead, so invalid modes are gracefully rejected before the commit? [ ... ] > +static void ra8875_pipe_update(struct drm_simple_display_pipe *pipe, > + struct drm_plane_state *old_plane_state) > +{ [ ... ] > + if (ra8875_write_reg(ra8875, RA8875_CURV1, > + (damage.y1 >> 8) & 0x01)) > + goto exit; > + > + /* MRWC command and pixel data are sent as separate SPI > messages;*/ > + if (spi_write(ra8875->spi, cmd, 2)) > + goto exit; [Severity: High] Does this spi_write() sleep in atomic context? Similar to the hardware initialization path, the plane update callback executes in atomic context where sleeping is forbidden. Using synchronous SPI calls here could lead to kernel warnings. > + > + offset = > + drm_fb_clip_offset(fb->pitches[0], fb->format, &damage); > + > + txbuf[0] = RA8875_DATAWRITE; > + for (r = 0; r < height; r++) { > + src_row = (u16 *)(map->vaddr + offset + > + r * fb->pitches[0]); > + dst_row = (u16 *)(txbuf + 1 + r * width * 2); > + for (p = 0; p < width; p++) > + dst_row[p] = swab16(src_row[p]); > + } [Severity: High] Will this cause an alignment fault on strict-alignment architectures? The expression (txbuf + 1 + r * width * 2) adds an odd offset (1) to the even base address, yielding an odd byte address. Casting this to a u16 pointer and performing a 16-bit store via dst_row[p] = swab16(...) violates C alignment rules and will likely crash platforms without hardware unaligned access support. > + > + memset(&t_data, 0, sizeof(t_data)); > + t_data.tx_buf = txbuf; > + t_data.len = 1 + height * width * 2; > + t_data.speed_hz = ra8875->spi->max_speed_hz; > + > + spi_message_init(&m); > + spi_message_add_tail(&t_data, &m); > + if (spi_sync(ra8875->spi, &m)) > + goto exit; > + } [Severity: Medium] Can this single transfer exceed the SPI controller's maximum transfer size? This sends the entire damage region in one SPI message, which could be quite large. Many SPI controllers enforce much smaller limits (like 4096 or 65535 bytes) and spi_sync() could fail with -EINVAL. Should this be chunked similarly to the logic in ra8875_clear_screen()? [ ... ] > +static int ra8875_probe(struct spi_device *spi) > +{ [ ... ] > + bufSize = 1 + RA8875_MAX_HEIGHT * RA8875_MAX_WIDTH * 2; > + ra8875->txbuf = devm_kzalloc(&spi->dev, bufSize, GFP_KERNEL); > + if (!ra8875->txbuf) > + return -ENOMEM; > + > + ret = of_get_videomode(dev->of_node, &vm, OF_USE_NATIVE_MODE); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to get videomode\n"); > + drm_display_mode_from_videomode(&vm, &ra8875->mode); > + > + ra8875->mode.type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED; > + > + drm_mode_set_name(&ra8875->mode); > + ret = drmm_mode_config_init(drm); > + if (ret) > + return ret; > + > + drm->mode_config.min_width = ra8875->mode.hdisplay; > + drm->mode_config.max_width = ra8875->mode.hdisplay; > + drm->mode_config.min_height = ra8875->mode.vdisplay; > + drm->mode_config.max_height = ra8875->mode.vdisplay; [Severity: High] Could this result in an out-of-bounds write to txbuf if the device tree specifies a large resolution? The txbuf array is allocated with a fixed size based on RA8875_MAX_WIDTH and RA8875_MAX_HEIGHT. However, the display dimensions read from the device tree are not validated against these maximums. If a larger resolution is provided, the DRM core will accept larger damage clips, and ra8875_pipe_update() will overflow the txbuf allocation when writing the pixels. [ ... ] > +static void ra8875_remove(struct spi_device *spi) > +{ > + struct drm_device *drm = spi_get_drvdata(spi); > + struct ra8875_device *ra8875 = > + container_of(drm, struct ra8875_device, drm); > + > + drm_dev_unregister(drm); > + ra8875_display_off(ra8875); > +} [Severity: High] Does this leave the device vulnerable to a use-after-free on unbind? Calling drm_dev_unregister() leaves dev->unplugged set to false. If the device is unbound via sysfs while userspace has the DRM file open, a subsequent display update ioctl will pass the drm_dev_enter() check in ra8875_pipe_update() and attempt to use txbuf after devres has already freed it. Should this use drm_dev_unplug() instead? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
