Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [High] drm/tiny: ra8875: sleeping functions called in atomic context - [High] drm/tiny: ra8875: out-of-bounds DMA read due to double len increment - [High] drm/tiny: ra8875: heap buffer overflow in ra8875_clear_screen - [Medium] drm/tiny: ra8875: unsafe DMA from unaligned struct members - [Low] drm/tiny: ra8875: spurious "Shadow map is null" error on plane disable - [Low] drm/tiny: ra8875: missing error check on ra8875_write_pixels --
commit a60dff365b1d365d54b1d95db3b3bdf06cf4d8be 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. It 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..d990d052e9477 > --- /dev/null > +++ b/drivers/gpu/drm/tiny/ra8875.c > @@ -0,0 +1,713 @@ [ ... ] > +struct ra8875_device { > + struct drm_device drm; > + struct spi_device *spi; > + struct gpio_desc *rst_gpio; > + struct drm_crtc crtc; > + struct drm_plane plane; > + struct drm_encoder encoder; > + struct drm_connector connector; > + struct drm_display_mode mode; > + u8 cmd_buf[RA8875_CMD_DATA_BUF_SIZE]; > + u8 data_buf[RA8875_CMD_DATA_BUF_SIZE]; [Severity: Medium] Is it safe to use these inline arrays for DMA? According to kernel DMA rules, buffers mapped for DMA must not share a cacheline with other variables. Concurrent CPU accesses to adjacent members in the structure (such as userspace reading mode properties) during an ongoing SPI DMA transfer might cause cacheline bouncing and silent data corruption. Should these arrays have cacheline alignment attributes or be allocated separately? > + u8 *txbuf; > +}; [ ... ] > +static int ra8875_write_pixels(struct ra8875_device *ra8875, size_t len) > +{ > + int ret; > + > + ra8875->cmd_buf[0] = RA8875_CMDWRITE; > + ra8875->cmd_buf[1] = RA8875_MRWC; > + ret = spi_write(ra8875->spi, ra8875->cmd_buf, 2); > + if (ret) > + return ret; > + > + return spi_write(ra8875->spi, ra8875->txbuf, 1 + len); [Severity: High] Does this length calculation cause an out-of-bounds DMA read? The callers (ra8875_clear_screen() and ra8875_fb_dirty()) already account for the 1-byte command header when calculating the len argument. For example, they pass '1 + rows * width * 2'. This double-addition appears to cause the requested SPI transfer size to be 1 byte larger than intended, which can exceed the fixed heap allocation of txbuf. > +} > + > +static int ra8875_hw_init(struct ra8875_device *ra8875, > + struct drm_display_mode *mode) > +{ > + u8 hdwr, hndr, hstr, hpwr, vpwr; > + u16 vdhr, vndr, vstr; > + int ret; > + > + if (ra8875->rst_gpio) { > + gpiod_set_value_cansleep(ra8875->rst_gpio, 1); > + usleep_range(1000, 1200); > + gpiod_set_value_cansleep(ra8875->rst_gpio, 0); > + msleep(RA8875_RESET_WAIT_MS); > + } [Severity: High] Can these sleeping functions be called safely here? This hardware initialization routine is invoked by ra8875_crtc_atomic_enable(), which runs in atomic context. Calling msleep(), usleep_range(), or gpiod_set_value_cansleep() in an atomic context causes kernel warnings and potential deadlocks. Note that synchronous spi_write() is also used throughout these atomic callbacks, which similarly sleeps. [ ... ] > +static int ra8875_clear_screen(struct ra8875_device *ra8875, > + struct drm_display_mode *mode) > +{ > + int width = mode->hdisplay; > + int height = mode->vdisplay; > + int y, rows, ret; > + u8 *txbuf = ra8875->txbuf; > + int rows_per_chunk = RA8875_TXBUF_ROWS; [ ... ] > + ret = ra8875_write_reg(ra8875, RA8875_CURV1, 0); > + if (ret) > + return ret; > + > + memset(txbuf, 0, 1 + rows_per_chunk * width * 2); [Severity: High] Does this risk a heap buffer overflow? The width parameter is derived from mode->hdisplay. Since ra8875_mode_valid() does not restrict the maximum width, a maliciously large hdisplay (e.g., 1600) could cause this memset to exceed the fixed txbuf allocation bounds and corrupt kernel memory. > + txbuf[0] = RA8875_DATAWRITE; > + > + for (y = 0; y < height; y += rows) { > + rows = min(rows_per_chunk, height - y); > + ra8875_write_pixels(ra8875, 1 + rows * width * 2); > + } > + > + return 0; > +} [ ... ] > +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); > + > + ra8875_write_pixels(ra8875, 1 + rows * width * 2); [Severity: Low] Is it intentional that the integer return value of ra8875_write_pixels() is ignored here? If an SPI bus error occurs, the driver will blindly continue to iterate and attempt to write subsequent chunks, hiding the failure from the kernel log. > + } > + > + drm_format_conv_state_release(&fmtcnv_state); > + return; [ ... ] > +static void ra8875_plane_atomic_update(struct drm_plane *plane, struct > drm_atomic_commit *state) > +{ > + struct ra8875_device *ra8875 = container_of(plane->dev, struct > ra8875_device, drm); > + struct drm_plane_state *old_plane_state = > drm_atomic_get_old_plane_state(state, plane); > + struct drm_plane_state *plane_state = > drm_atomic_get_new_plane_state(state, plane); > + struct drm_shadow_plane_state *shadow_plane_state = > to_drm_shadow_plane_state(plane_state); > + struct drm_framebuffer *fb = plane_state->fb; > + struct drm_rect rect; > + struct iosys_map *map = &shadow_plane_state->data[0]; > + int idx; > + > + if (!drm_dev_enter(&ra8875->drm, &idx)) > + return; > + > + if (iosys_map_is_null(map)) { > + drm_err(&ra8875->drm, "Shadow map is null\n"); [Severity: Low] Will this unconditionally log a spurious error for standard plane disable operations? Since the driver does not implement the atomic_disable hook, the core falls back to calling atomic_update() with a NULL framebuffer when disabling the plane. > + goto exit; > + } > + if (drm_atomic_helper_damage_merged(old_plane_state, plane_state, > &rect)) > + ra8875_fb_dirty(ra8875, map, fb, &rect); > +exit: > + drm_dev_exit(idx); > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
