Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] drm/gud: probe unconditionally fails on non-DMA devices due to uninitialized access - [High] drm/gud: Use-After-Free of shadow_buf in background flush worker - [High] drm/gud: Heap Buffer Overflow in shadow_buf during framebuffer resize - [High] drm/gud: NULL pointer dereference in flush worker after vcalloc failure - [Medium] drm/gud: Incomplete damage coordinate alignment yields unaligned width --
commit 25423c5e9ff4d00f13c41b50149983a21e74d1aa Author: Hui Peng <[email protected]> drm/gud: fix bulk_len divide-by-zero, connector bitmap OOB, and short USB reads This commit adds bounds checks during the probe process, updates shadow buffer management in queue damage, and rounds down coordinates to block width for damage flushes. > diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c > --- a/drivers/gpu/drm/gud/gud_drv.c > +++ b/drivers/gpu/drm/gud/gud_drv.c [ ... ] > @@ -495,6 +497,10 @@ static int gud_probe(struct usb_interface *intf, const > struct usb_device_id *id) > put_device(dma_dev); > } else { > dev_warn(dev, "buffer sharing not supported"); /* not an error > */ > + if (!drm->mode_config.min_width || !drm->mode_config.min_height || > + drm->mode_config.max_width < drm->mode_config.min_width || > + drm->mode_config.max_height < drm->mode_config.min_height) > + return -EINVAL; [Severity: High] Does this unconditionally fail the probe for devices without DMA? Inside the else branch, this newly added bounds check evaluates fields in drm->mode_config, but drmm_mode_config_init() and the descriptor assignments have not yet been executed. Since this memory is zero-initialized, !drm->mode_config.min_width evaluates to true and the function unconditionally returns -EINVAL. > } > [ ... ] > diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c > --- a/drivers/gpu/drm/gud/gud_pipe.c > +++ b/drivers/gpu/drm/gud/gud_pipe.c [ ... ] > @@ -156,10 +156,14 @@ static int gud_prep_flush(struct gud_device *gdrm, > struct drm_framebuffer *fb, > struct drm_format_conv_state *fmtcnv_state) > { > u8 compression = gdrm->compression; > + unsigned int block_width = drm_format_info_block_width(format, 0); > struct iosys_map dst; > void *vaddr, *buf; > size_t pitch, len; > > + if (block_width > 1) > + rect->x1 = ALIGN_DOWN(rect->x1, block_width); [Severity: Medium] Will this result in an unaligned rectangle width? ALIGN_DOWN is applied to rect->x1, but rect->x2 is not adjusted. Because the rectangle width is dynamically calculated as x2 - x1, this isolated modification of x1 without a corresponding adjustment to x2 can result in a width that is no longer a multiple of block_width. > + > pitch = drm_format_info_min_pitch(format, 0, drm_rect_width(rect)); [ ... ] > @@ -335,12 +339,21 @@ static void gud_flush_damage(struct gud_device *gdrm, > struct drm_framebuffer *fb > if (format->format == DRM_FORMAT_XRGB8888 && > gdrm->xrgb8888_emulation_format) > format = gdrm->xrgb8888_emulation_format; > > + block_width = drm_format_info_block_width(format, 0); > + if (block_width > 1) > + damage->x1 = ALIGN_DOWN(damage->x1, block_width); [Severity: Medium] Does this have the same alignment issue as in gud_prep_flush()? Adjusting damage->x1 without adjusting damage->x2 might result in an unaligned width when calculating x2 - x1. > + > /* Split update if it's too big */ [ ... ] > @@ -399,6 +412,13 @@ static int gud_fb_queue_damage(struct gud_device *gdrm, > struct drm_framebuffer * > > mutex_lock(&gdrm->damage_lock); > > + if (gdrm->shadow_buf && gdrm->fb && > + (fb->pitches[0] != gdrm->fb->pitches[0] || fb->height != > gdrm->fb->height)) { > + vfree(gdrm->shadow_buf); > + gdrm->shadow_buf = NULL; [Severity: High] Can this lead to a use-after-free of gdrm->shadow_buf if the asynchronous worker gud_flush_work() is currently running? The worker clears gdrm->fb and drops the lock while performing a lengthy USB bulk transfer. Concurrently, if a new frame with different dimensions is queued, gdrm->fb may be set to the new fb, and a subsequent frame could trigger this vfree() while the worker is still actively accessing the buffer. [Severity: High] Additionally, does the && gdrm->fb condition bypass the reallocation block when the background worker is active? If gud_flush_work() has set gdrm->fb to NULL, the reallocation check fails. This bypasses the buffer reallocation, and subsequently drm_fb_memcpy() will copy data from a larger framebuffer into the undersized shadow_buf, causing a heap buffer overflow. > + gud_clear_damage(gdrm); > + } > + > if (!gdrm->shadow_buf) { > gdrm->shadow_buf = vcalloc(fb->pitches[0], fb->height); > if (!gdrm->shadow_buf) { > mutex_unlock(&gdrm->damage_lock); > return -ENOMEM; [Severity: High] What happens if vcalloc() fails here? If the function returns -ENOMEM without clearing gdrm->fb or canceling the pending gud_flush_work(), the worker will eventually run, see a valid gdrm->fb, and pass a NULL shadow map along with a cleared damage rect to gud_flush_damage(). Because gud_clear_damage() sets y1 = INT_MAX and y2 = 0, calculating drm_rect_height(damage) will yield a negative number, which underflows to a massive positive iteration count and leads to repeated calls to gud_prep_flush() dereferencing a NULL source virtual address. > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
