Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [Critical] drm/solomon: heap buffer overflow of tx_buf in ssd16xx_fb_dirty() - [Medium] drm/solomon: bit-reversed pixels in ssd16xx_convert_fb_to_1bpp() - [Medium] drm/solomon: ssd16xx_convert_fb_to_3color() ignores orientation - [Medium] drm/solomon: init_refresh_pending cleared on error in atomic_update - [Medium] drm/solomon: ssd16xx: use-after-free of devm-allocated mode after unbind - [Low] drm/solomon: broken portrait check in ssd16xx_connector_get_modes() --
commit 2fb6265a2051abb85583fcfb0ff873962f7bd4d7 Author: Devarsh Thakkar <[email protected]> drm/solomon: Add DRM driver for Solomon SSD16xx e-paper display controllers This commit adds a DRM driver for the Solomon Systech SSD16xx family of e-paper display controllers over SPI, starting with support for the Gooddisplay GDEY042T81 400x300 black/white panel using the SSD1683 controller. [1] : https://www.crystalfontz.com/controllers/SolomonSystech/SSD1683 [2] : https://files.seeedstudio.com/wiki/Other_Display/42-epaper/GDEY042T81.pdf [3] : https://www.crystalfontz.com/controllers/SolomonSystech/SSD1673 [4] : https://www.crystalfontz.com/controllers/SolomonSystech/SSD1680 [5] : https://www.crystalfontz.com/controllers/SolomonSystech/SSD1681 > diff --git a/drivers/gpu/drm/solomon/ssd16xx.c > b/drivers/gpu/drm/solomon/ssd16xx.c > new file mode 100644 > index 0000000000000..ca6550b5dd5bd > --- /dev/null > +++ b/drivers/gpu/drm/solomon/ssd16xx.c > @@ -0,0 +1,1801 @@ [ ... ] > +static int ssd16xx_hw_init(struct ssd16xx_device *device) > +{ [ ... ] > + switch (device->orientation) { > + case 90: > + case 180: > + data_entry_mode = SSD16XX_DATA_ENTRY_XDEC_YDEC; > + break; > + default: /* 0°/270° */ > + data_entry_mode = SSD16XX_DATA_ENTRY_XINC_YINC; > + break; > + } [ ... ] > +static void ssd16xx_convert_fb_to_3color(u8 *bw_dst, u8 *red_dst, > + struct iosys_map *src, > + struct drm_framebuffer *fb, > + struct drm_rect *rect) > +{ > + unsigned int x, y; > + u8 bw_byte = 0, red_byte = 0; > + unsigned int bit_pos = 0; > + unsigned int dst_idx = 0; > + > + if (fb->format->format == DRM_FORMAT_R1) { > + unsigned int src_pitch = fb->pitches[0]; > + unsigned int width_bytes = drm_rect_width(rect) / 8; > + unsigned int data_size = width_bytes * drm_rect_height(rect); > + > + /* White background — no black pixels, only red ink shows */ > + memset(bw_dst, 0xFF, data_size); > + /* RED RAM: copy R1 data directly (1=red ink, 0=no red) */ > + for (y = rect->y1; y < rect->y2; y++) { > + u8 *line = src->vaddr + y * src_pitch + (rect->x1 / 8); > + > + memcpy(red_dst + dst_idx, line, width_bytes); > + dst_idx += width_bytes; > + } > + return; > + } > + > + /* XRGB8888 */ > + for (y = rect->y1; y < rect->y2; y++) { > + for (x = rect->x1; x < rect->x2; x++) { > + bool is_red = ssd16xx_pixel_is_red(src, fb, x, y); > + > + if (is_red) > + red_byte |= (1 << (7 - bit_pos)); > + else if (ssd16xx_pixel_is_white(src, fb, x, y)) > + bw_byte |= (1 << (7 - bit_pos)); > + if (++bit_pos == 8) { > + bw_dst[dst_idx] = bw_byte; > + red_dst[dst_idx] = red_byte; > + dst_idx++; > + bw_byte = 0; > + red_byte = 0; > + bit_pos = 0; > + } > + } > + if (bit_pos > 0) { > + bw_dst[dst_idx] = bw_byte; > + red_dst[dst_idx] = red_byte; > + dst_idx++; > + bw_byte = 0; > + red_byte = 0; > + bit_pos = 0; > + } > + } > +} [Severity: Medium] Should ssd16xx_convert_fb_to_3color() also handle device->orientation like ssd16xx_convert_fb_to_1bpp() does? When ssd16xx_fb_dirty() runs with device->color_mode == SSD16XX_COLOR_MODE_3COLOR and FULL or FAST refresh, it calls ssd16xx_convert_fb_to_3color() without passing device->orientation. Because ssd16xx_convert_fb_to_3color() always packs in 0-degree landscape row-major order, 90, 180, and 270 degree rotations are ignored. In portrait orientations (90 or 270 degrees) where fb->width is 300 and fb->height is 400, the XRGB8888 path packs 38 bytes per row across 400 rows (15,200 bytes in row-major order instead of 300 column-major rows of 50 bytes, while ssd16xx_fb_dirty() only sends data_size = 15,000 bytes), and the DRM_FORMAT_R1 path computes width_bytes = 300 / 8 = 37 bytes per row (14,800 bytes), truncating 4 pixels per row without transposing for portrait orientation. > + > +/* > + * Convert framebuffer to 1-bit monochrome for e-paper display. > + * > + * Supports XRGB8888 (thresholded via ITU-R BT.601 luma at 127) and R1 > + * (native 1bpp, zero-copy fast path for aligned landscape frames). > + * > + * Output layout: > + * 0°/180° landscape: row-major, left-to-right, top-to-bottom > + * 90°/270° CW portrait: column-major, rightmost column first > + */ > +static void ssd16xx_convert_fb_to_1bpp(u8 *dst, struct iosys_map *src, > + struct drm_framebuffer *fb, > + struct drm_rect *rect, > + unsigned int orientation) > +{ [ ... ] > + switch (orientation) { > + case 90: > + case 270: > + /* > + * Portrait (90° or 270°): column-major packing. > + * Each portrait source column becomes one physical RAM row. > + * The data entry mode and cursor position control scan > direction. > + */ > + for (x = rect->x2 - 1; x >= (int)rect->x1; x--) { > + for (y = rect->y1; y < rect->y2; y++) { > + if (ssd16xx_pixel_is_white(src, fb, x, y)) > + byte |= (1 << (7 - bit_pos)); > + if (++bit_pos == 8) { > + dst[dst_idx++] = byte; > + byte = 0; > + bit_pos = 0; > + } > + } > + if (bit_pos > 0) { > + dst[dst_idx++] = byte; > + byte = 0; > + bit_pos = 0; > + } > + } > + break; > + > + case 0: > + case 180: > + default: > + /* > + * Landscape (0° or 180°): row-major packing. > + * Each landscape source row becomes one physical RAM row. > + * The data entry mode and cursor position control scan > direction. > + */ > + for (y = rect->y1; y < rect->y2; y++) { > + for (x = rect->x1; x < rect->x2; x++) { > + if (ssd16xx_pixel_is_white(src, fb, x, y)) > + byte |= (1 << (7 - bit_pos)); > + if (++bit_pos == 8) { > + dst[dst_idx++] = byte; > + byte = 0; > + bit_pos = 0; > + } > + } > + if (bit_pos > 0) { > + dst[dst_idx++] = byte; > + byte = 0; > + bit_pos = 0; > + } > + } > + break; > + } > +} [Severity: Medium] Are pixels within each byte bit-reversed when orientation is 90 or 180 degrees? When device->orientation is 90 or 180, ssd16xx_hw_init() configures Data Entry Mode (command 0x11) to SSD16XX_DATA_ENTRY_XDEC_YDEC (0x00), and ssd16xx_fb_dirty() sets the RAM X address counter to ram_x_end (49) so that it decrements down to 0 across each scanline. In SSD16xx byte-addressed RAM, each byte represents 8 horizontal pixels along the physical X axis where bit 7 (MSB) drives source line 8 * X_addr + 0 and bit 0 (LSB) drives source line 8 * X_addr + 7. Decrementing the X byte counter reverses the byte order across a scanline (49, 48, ..., 0), but does not reverse the bit order within each byte. Because ssd16xx_convert_fb_to_1bpp() uses the same MSB-first packing expression: byte |= (1 << (7 - bit_pos)); for 90 and 180 degrees as it does for 270 and 0 degrees, pixel 0 is placed at physical pixel 392 and pixel 7 at physical pixel 399 in X_addr = 49, followed by pixel 8 at physical pixel 384 and pixel 15 at physical pixel 391 in X_addr = 48, mirroring each 8-pixel group horizontally. > + > +static int ssd16xx_fb_dirty(struct drm_framebuffer *fb, struct drm_rect > *rect, > + struct ssd16xx_device *device, > + const struct iosys_map *src_map) > +{ > + const u8 *ctrl2_tbl = device->controller_cfg->ctrl2_refresh; > + struct iosys_map map = *src_map; > + int err = 0; > + unsigned int data_size = (device->width * device->height) / 8; > + u8 *mono_buffer = NULL; > + u8 *red_buffer = NULL; > + u16 ram_x_start, ram_x_end, ram_y_start, ram_y_end; > + > + /* > + * Process full display area. The rect for convert_fb_to_1bpp uses > + * the framebuffer (logical) dimensions - the pixel iteration walks > + * the source fb coordinate space. RAM window registers below use > + * the physical panel dimensions (device->width/height). > + */ > + rect->x1 = 0; > + rect->y1 = 0; > + rect->x2 = fb->width; > + rect->y2 = fb->height; [Severity: Critical] Can setting rect->x2 = fb->width and rect->y2 = fb->height here overflow device->tx_buf and device->tx_red_buf? In ssd16xx_probe(), drm->mode_config is configured with: drm->mode_config.min_width = min(device->width, device->height); drm->mode_config.max_width = max(device->width, device->height); drm->mode_config.min_height = min(device->width, device->height); drm->mode_config.max_height = max(device->width, device->height); This allows userspace via DRM_IOCTL_MODE_ADDFB2 to create a 400x400 framebuffer and attach it to the primary plane with a 400x300 or 300x400 source rectangle matching the CRTC mode, which passes both drm_atomic_plane_check() and ssd16xx_plane_atomic_check(). However, ssd16xx_alloc_tx_bufs() allocates device->tx_buf and device->tx_red_buf with: size_t frame_size = max(DIV_ROUND_UP(device->width, 8) * device->height, DIV_ROUND_UP(device->height, 8) * device->width); which is 15,200 bytes for a 400x300 panel. Because ssd16xx_fb_dirty() overwrites rect with fb->width and fb->height (400x400) rather than the active mode/clipped plane dimensions, ssd16xx_convert_fb_to_1bpp() and ssd16xx_convert_fb_to_3color() will write (400 / 8) * 400 = 20,000 bytes into the 15,200-byte heap buffer. In addition, ssd16xx_crtc_mode_valid() accepts both 400x300 and 300x400 modes regardless of the static device->orientation configured in ssd16xx_probe(): if ((mode->hdisplay == device->mode->hdisplay && mode->vdisplay == device->mode->vdisplay) || (mode->hdisplay == device->mode->vdisplay && mode->vdisplay == device->mode->hdisplay)) return MODE_OK; If userspace commits a 300x400 mode when device->orientation is 0 or 180 (or 400x300 when orientation is 90 or 270), ssd16xx_convert_fb_to_1bpp() packs 38-byte padded rows across 400 rows (15,200 bytes, or 37-byte rows totaling 14,800 bytes on the DRM_FORMAT_R1 fast path), while ssd16xx_fb_dirty() only zeroes and transmits data_size = 15,000 bytes to a hardware RAM window programmed for 50-byte rows. [ ... ] > +static void ssd16xx_plane_atomic_update(struct drm_plane *plane, > + struct drm_atomic_commit *state) > +{ [ ... ] > + ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE); > + if (ret) { > + drm_err(&device->drm, "plane_atomic_update: begin_cpu_access > failed: %d\n", ret); > + goto out_restore; > + } > + > + ret = ssd16xx_fb_dirty(fb, &rect, device, &shadow_state->data[0]); > + if (ret) > + drm_err(&device->drm, "plane_atomic_update: display update > failed: %d\n", ret); > + > + drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE); > + > +out_restore: > + device->refresh_mode = saved_mode; > + device->border_waveform_update_idx = saved_border_waveform_idx; > + > + /* > + * If this was the init frame (which used border_waveform_init_idx > + * inside fb_dirty), arm border_waveform_pending so the normal > + * (non-init) border value is sent at the start of the next update. > + */ > + if (device->init_refresh_pending) { > + device->init_refresh_pending = false; > + device->border_waveform_pending = true; > + } [Severity: Medium] If drm_gem_fb_begin_cpu_access() or ssd16xx_fb_dirty() fails during the initial frame, should device->init_refresh_pending remain set instead of being cleared at out_restore? Clearing device->init_refresh_pending on the error path causes subsequent frames to skip the initial full refresh and perform partial refreshes without a valid RED RAM baseline. [ ... ] > +static int ssd16xx_connector_get_modes(struct drm_connector *connector) > +{ > + struct ssd16xx_device *device = to_ssd16xx_device(connector->dev); > + bool mode_is_portrait = (device->mode->hdisplay < > device->mode->vdisplay); > + bool orient_is_portrait = (device->orientation == 90 || > device->orientation == 270); > + > + drm_dbg(&device->drm, > + "connector_get_modes: orientation=%u° mode=%ux%u > mode_portrait=%d orient_portrait=%d\n", > + device->orientation, device->mode->hdisplay, > device->mode->vdisplay, > + mode_is_portrait, orient_is_portrait); > + > + /* For portrait, swap dimensions so clients see logical size. */ > + if (mode_is_portrait != orient_is_portrait) { > + struct drm_display_mode *mode; > + > + mode = drm_mode_duplicate(&device->drm, device->mode); > + if (!mode) > + return 0; > + swap(mode->hdisplay, mode->vdisplay); > + swap(mode->hsync_start, mode->vsync_start); > + swap(mode->hsync_end, mode->vsync_end); > + swap(mode->htotal, mode->vtotal); > + swap(mode->width_mm, mode->height_mm); > + mode->type |= DRM_MODE_TYPE_PREFERRED; > + drm_mode_set_name(mode); > + drm_mode_probed_add(connector, mode); > + return 1; > + } > + > + return drm_connector_helper_get_modes_fixed(connector, device->mode); > +} [Severity: Low] Since ssd16xx_probe() already swaps device->mode in-place when device->orientation is 90 or 270, is the swap check in ssd16xx_connector_get_modes() redundant? For natively landscape panels like GDEY042T81, device->mode->hdisplay < device->mode->vdisplay is already true when orient_is_portrait is true, so mode_is_portrait != orient_is_portrait is never taken. Conversely, for any natively portrait panel, mode_is_portrait != orient_is_portrait would be true in all orientations and undo the swap performed in ssd16xx_probe() (while also skipping the connector->display_info.width_mm and height_mm assignments done by drm_connector_helper_get_modes_fixed()). [ ... ] > +static int ssd16xx_probe(struct spi_device *spi) > +{ [ ... ] > + mode = devm_kmemdup(dev, device->device_cfg->mode, > + sizeof(*device->device_cfg->mode), GFP_KERNEL); > + if (!mode) > + return -ENOMEM; [Severity: Medium] Can allocating device->mode with devm_kmemdup() lead to a use-after-free after the SPI device is unbound? While struct ssd16xx_device is allocated via devm_drm_dev_alloc() and stays alive until the last open DRM file descriptor is closed, devm_kmemdup() ties device->mode to the SPI struct device devres list, which is freed as soon as ssd16xx_remove() returns. If an in-flight KMS ioctl (such as DRM_IOCTL_MODE_GETCONNECTOR or DRM_IOCTL_MODE_SETCRTC) passes the initial unplug check in drm_ioctl_kernel() and waits on modeset locks while ssd16xx_remove() runs, it can subsequently invoke ssd16xx_connector_get_modes() or ssd16xx_crtc_mode_valid() (neither of which is guarded by drm_dev_enter()) and dereference device->mode after devres_release_all() has freed it. Would embedding struct drm_display_mode directly in struct ssd16xx_device (or using drmm_kmemdup()) avoid this lifetime mismatch? > + > + device->refresh_mode = device->device_cfg->default_refresh_mode; [ ... ] > + /* Swap mode dimensions for portrait so clients see logical size. */ > + if (device->orientation == 90 || device->orientation == 270) { > + swap(mode->hdisplay, mode->vdisplay); > + swap(mode->hsync_start, mode->vsync_start); > + swap(mode->hsync_end, mode->vsync_end); > + swap(mode->htotal, mode->vtotal); > + swap(mode->width_mm, mode->height_mm); > + drm_dbg(drm, "Mode dimensions swapped for portrait: %ux%u\n", > + mode->hdisplay, mode->vdisplay); > + } else { > + drm_dbg(drm, "Mode dimensions unchanged: %ux%u\n", > + mode->hdisplay, mode->vdisplay); > + } > + device->mode = mode; [ ... ] > +static void ssd16xx_remove(struct spi_device *spi) > +{ > + struct ssd16xx_device *device = spi_get_drvdata(spi); > + > + drm_dev_unplug(&device->drm); > + drm_atomic_helper_shutdown(&device->drm); > +} [ ... ] > + -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
