Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider: - [Critical] drm/tiny: ra8875: heap overflow in ra8875_clear_screen() via unvalidated mode - [High] drm/tiny: ra8875: plane updates run before CRTC enable and screen clear - [High] drm/tiny: ra8875: miscalculation of HNDR and VNDR back porch in ra8875_hw_init() - [High] drm/tiny: ra8875: missing drm_dev_enter() guard in ra8875_crtc_atomic_enable() - [Medium] drm/tiny: ra8875: missing vdd-supply regulator acquisition and enable - [Medium] drm/tiny: ra8875: missing drm_gem_fb_begin_cpu_access() in ra8875_fb_dirty() - [Medium] drm/tiny/ra8875: unaligned u16 stores in ra8875_fb_dirty() via drm_fb_swab() - [Low] drm/tiny: ra8875: missing MMU dependency in Kconfig for DRM_RA8875 --
commit 7ff969ff52235eb3411700feffd794a3e46ceb5a Author: Adam Azuddin <[email protected]> drm/tiny: Add RAiO RA8875 display controller driver This commit adds a DRM tiny driver for the RAiO RA8875 SPI-connected TFT display controller supporting resolutions up to 800x480 with GEM shmem buffer management. > diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig > index f0e72d4b6a470..2f1c49e31dd60 100644 > --- a/drivers/gpu/drm/tiny/Kconfig > +++ b/drivers/gpu/drm/tiny/Kconfig > @@ -98,6 +98,19 @@ config DRM_PIXPAPER > > If M is selected, the module will be built as pixpaper.ko. > > +config DRM_RA8875 > + tristate "DRM support for RA8875 display panels" > + depends on DRM && SPI && OF > + select DRM_CLIENT_SELECTION > + select DRM_KMS_HELPER > + select DRM_GEM_SHMEM_HELPER > + select VIDEOMODE_HELPERS [Severity: Low] In drivers/gpu/drm/tiny/Kconfig, DRM_RA8875 selects DRM_GEM_SHMEM_HELPER, which depends on MMU in drivers/gpu/drm/Kconfig. Without a dependency on MMU, enabling DRM_RA8875 on a !CONFIG_MMU target triggers an unmet direct dependency warning and fails to build in drm_gem_shmem_helper.c. Should DRM_RA8875 depend on MMU as well, matching the other shmem-based drivers in drivers/gpu/drm/tiny/Kconfig? [ ... ] > diff --git a/drivers/gpu/drm/tiny/ra8875.c b/drivers/gpu/drm/tiny/ra8875.c > new file mode 100644 > index 0000000000000..937bb7fdbd910 > --- /dev/null > +++ b/drivers/gpu/drm/tiny/ra8875.c [ ... ] > +static int ra8875_hw_init(struct ra8875_device *ra8875, > + struct drm_display_mode *mode) > +{ [ ... ] > + /* Horizontal timing */ > + hdwr = (mode->hdisplay / 8) - 1; > + hndr = ((mode->htotal - mode->hdisplay) / 8) - 1; > + hstr = ((mode->hsync_start - mode->hdisplay) / 8) - 1; > + hpwr = ((mode->hsync_end - mode->hsync_start) / 8) - 1; > + > + ret = ra8875_write_reg(ra8875, RA8875_HDWR, hdwr); > + if (ret) > + return ret; > + ret = ra8875_write_reg(ra8875, RA8875_HNDFTR, RA8875_HNDFTR_HNDFT(0)); > + if (ret) > + return ret; > + ret = ra8875_write_reg(ra8875, RA8875_HNDR, hndr); > + if (ret) > + return ret; > + ret = ra8875_write_reg(ra8875, RA8875_HSTR, hstr); > + if (ret) > + return ret; > + ret = ra8875_write_reg(ra8875, RA8875_HPWR, hpwr); > + if (ret) > + return ret; > + > + /* Vertical timing */ > + vdhr = mode->vdisplay - 1; > + vndr = mode->vtotal - mode->vdisplay - 1; > + vstr = mode->vsync_start - mode->vdisplay - 1; > + vpwr = mode->vsync_end - mode->vsync_start - 1; [Severity: High] In drivers/gpu/drm/tiny/ra8875.c, are hndr and vndr in ra8875_hw_init() being computed from the total blanking interval rather than the back porch? In drm_display_mode_from_videomode(): dmode->hsync_start = dmode->hdisplay + vm->hfront_porch; dmode->hsync_end = dmode->hsync_start + vm->hsync_len; dmode->htotal = dmode->hsync_end + vm->hback_porch; Since HSTR (and VSTR0/VSTR1) already program the front porch and HPWR (and VPWR) program the sync pulse width, RA8875_HNDR and RA8875_VNDR0/RA8875_VNDR1 program the horizontal and vertical non-display (back porch) periods. Using mode->htotal - mode->hdisplay and mode->vtotal - mode->vdisplay counts the front porch and sync pulse width a second time. Should these be computed from mode->htotal - mode->hsync_end and mode->vtotal - mode->vsync_end instead? [ ... ] > +static int ra8875_clear_screen(struct ra8875_device *ra8875, > + struct drm_display_mode *mode) > +{ [ ... ] > + memset(txbuf, 0, 1 + rows_per_chunk * width * 2); > + txbuf[0] = RA8875_DATAWRITE; > + > + for (y = 0; y < height; y += rows) { > + rows = min(rows_per_chunk, height - y); > + ret = ra8875_write_pixels(ra8875, 1 + rows * width * 2); > + if (ret) > + return ret; > + } > + > + return 0; > +} > + > +static void ra8875_crtc_atomic_enable(struct drm_crtc *crtc, struct > drm_atomic_commit *state) > +{ > + struct ra8875_device *ra8875 = > + container_of(crtc->dev, struct ra8875_device, drm); > + struct drm_crtc_state *crtc_state = > + drm_atomic_get_new_crtc_state(state, crtc); > + struct drm_display_mode *mode = &crtc_state->mode; > + int ret; > + > + ret = ra8875_hw_init(ra8875, mode); > + if (ret) { > + drm_err(&ra8875->drm, "Failed to initialize hardware\n"); > + return; > + } > + > + ret = ra8875_clear_screen(ra8875, mode); > + if (ret) { > + drm_err(&ra8875->drm, "Failed to clear screen\n"); > + return; > + } > +} [Severity: High] In drivers/gpu/drm/tiny/ra8875.c, doesn't the atomic commit order cause ra8875_crtc_atomic_enable() to wipe out the initial plane update on every modeset? In ra8875_probe(), ra8875->rst_gpio is requested with GPIOD_OUT_HIGH and drm->mode_config.helper_private is left unset. When drm_atomic_helper_commit() runs a modeset (including the initial fbdev modeset from drm_client_setup()), it uses the default drm_atomic_helper_commit_tail(), which executes: drm_atomic_helper_commit_tail() drm_atomic_helper_commit_planes(dev, state, 0) ra8875_plane_atomic_update() ra8875_fb_dirty() drm_atomic_helper_commit_modeset_enables(dev, state) ra8875_crtc_atomic_enable() ra8875_hw_init() ra8875_clear_screen() As a result, ra8875_plane_atomic_update() writes the framebuffer while the chip is still held in reset, and then ra8875_crtc_atomic_enable() resets the hardware and zeroes the entire display memory in ra8875_clear_screen(). In addition, ra8875_plane_atomic_check() passes can_update_disabled = true to drm_atomic_helper_check_plane_state(), and ra8875_plane_atomic_update() does not check whether the CRTC is active. Should drm->mode_config.helper_private be configured with .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm (so CRTC enable runs before plane updates, and plane updates are skipped when the CRTC is inactive), and can_update_disabled be set to false in ra8875_plane_atomic_check()? [Severity: High] Also in ra8875_crtc_atomic_enable() in drivers/gpu/drm/tiny/ra8875.c, can a modeset racing with device removal in ra8875_remove() cause a use-after-free on ra8875->txbuf and ra8875->rst_gpio? In ra8875_probe(), ra8875->txbuf and ra8875->rst_gpio are allocated with devm_kzalloc(&spi->dev, ...) and devm_gpiod_get_optional(&spi->dev, ...), so devres frees them as soon as ra8875_remove() returns, even if a DRM file descriptor remains open. Unlike ra8875_plane_atomic_update(), ra8875_crtc_atomic_enable() does not guard its hardware and buffer accesses with drm_dev_enter() and drm_dev_exit(). If an atomic commit enters ra8875_crtc_atomic_enable() concurrently with or after drm_dev_unplug() in ra8875_remove(), drm_dev_unplug() will not synchronize with it and ra8875_clear_screen() can memset() and SPI-transfer from the freed ra8875->txbuf buffer. Should ra8875_crtc_atomic_enable() (and ra8875_crtc_atomic_disable()) be guarded with drm_dev_enter() and drm_dev_exit()? [ ... ] > +static void ra8875_fb_dirty(struct ra8875_device *ra8875, struct iosys_map > *src, > + struct drm_framebuffer *fb, struct drm_rect *rect) > +{ [ ... ] > + for (int y = rect->y1; y < rect->y2; y += rows) { > + struct drm_rect chunk; > + > + rows = min(RA8875_TXBUF_ROWS, rect->y2 - y); > + chunk = DRM_RECT_INIT(rect->x1, y, width, rows); > + > + iosys_map_set_vaddr(&dst, ra8875->txbuf + 1); > + drm_fb_swab(&dst, NULL, src, fb, &chunk, false, &fmtcnv_state); > + > + if (ra8875_write_pixels(ra8875, 1 + rows * width * 2)) > + goto err; > + } [Severity: Medium] In ra8875_fb_dirty() in drivers/gpu/drm/tiny/ra8875.c, drm_fb_swab() reads from the shadow-mapped framebuffer with the CPU, but drm_gem_begin_shadow_fb_access() only vmaps the buffer without calling dma_buf_begin_cpu_access(). When displaying an imported PRIME DMA-BUF on a non-cache-coherent system, can omitting drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE) and drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE) around the CPU read result in stale cache reads and display corruption? [Severity: Medium] Also in ra8875_fb_dirty() in drivers/gpu/drm/tiny/ra8875.c, doesn't passing ra8875->txbuf + 1 as the destination buffer to drm_fb_swab() perform unaligned 16-bit stores on every pixel? In ra8875_probe(), ra8875->txbuf is allocated with devm_kzalloc(), which is even-aligned, making ra8875->txbuf + 1 an odd address. For DRM_FORMAT_RGB565, drm_fb_swab() calls drm_fb_swab16_line(), which casts the destination pointer directly to u16 * and writes *dbuf16++ = swab16(*sbuf16++) without put_unaligned(). Can this trigger alignment faults on architectures requiring natural alignment as well as UBSAN alignment warnings? [ ... ] > +static enum drm_mode_status ra8875_mode_valid(struct drm_crtc *crtc, > + const struct drm_display_mode > *mode) > +{ > + if (mode->hdisplay % 8 != 0 || mode->hsync_start % 8 != 0 || > + mode->hsync_end % 8 != 0 || mode->htotal % 8 != 0) { > + drm_dbg_kms(crtc->dev, > + "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 MODE_H_ILLEGAL; > + } > + return MODE_OK; > +} [Severity: Critical] As reported by Sashiko on v4 [1], can a userspace-supplied display mode overflow ra8875->txbuf in ra8875_clear_screen()? In ra8875_probe() in drivers/gpu/drm/tiny/ra8875.c, ra8875->txbuf is allocated for a fixed maximum width of RA8875_MAX_WIDTH (800): buf_size = 1 + RA8875_TXBUF_ROWS * RA8875_MAX_WIDTH * 2; ra8875->txbuf = devm_kzalloc(&spi->dev, buf_size, GFP_KERNEL); While drm->mode_config.max_width restricts framebuffer sizes, a userspace DRM master can submit a custom CRTC mode via KMS (for example with the primary plane disabled). Because ra8875_mode_valid() only checks that the horizontal timings are multiples of 8 and does not call drm_crtc_helper_mode_valid_fixed(crtc, mode, &ra8875->mode) or bound mode->hdisplay to RA8875_MAX_WIDTH, committing a mode with mode->hdisplay > 800 causes ra8875_crtc_atomic_enable() -> ra8875_clear_screen() to memset() and SPI-write 1 + 16 * mode->hdisplay * 2 bytes past the end of ra8875->txbuf. Additionally, neither ra8875_probe() nor ra8875_mode_valid() verifies that the horizontal and vertical front porch, sync pulse, and back porch lengths are strictly positive, so a zero-length porch or sync pulse will underflow when subtracting 1 in ra8875_hw_init(). Should ra8875_mode_valid() validate the mode with drm_crtc_helper_mode_valid_fixed() and verify that all porch and sync pulse intervals are non-zero? [ ... ] > +static int ra8875_probe(struct spi_device *spi) > +{ [ ... ] > + ra8875->rst_gpio = devm_gpiod_get_optional(dev, "reset", > GPIOD_OUT_HIGH); > + if (IS_ERR(ra8875->rst_gpio)) > + return dev_err_probe(dev, PTR_ERR(ra8875->rst_gpio), > + "failed to get reset GPIO\n"); [Severity: Medium] In ra8875_probe() in drivers/gpu/drm/tiny/ra8875.c, the driver requests the optional reset GPIO, but never acquires or enables the vdd-supply regulator that is marked as required in Documentation/devicetree/bindings/display/panel/raio,ra8875.yaml. If vdd-supply is backed by a switchable regulator that is disabled at boot or turned off by the regulator core as unused, won't the RA8875 remain unpowered when ra8875_hw_init() attempts SPI transfers? Should ra8875_probe() enable the vdd supply (for example via devm_regulator_get_enable(dev, "vdd"))? [1] https://lore.kernel.org/r/[email protected] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
