Re: [PATCH] drm/gud: fix bulk_len divide-by-zero, connector bitmap OOB, and short USB reads

[Ruben Wauters]

On Sat, 2026-09-19 at 22:35 +0000, Hui Peng wrote:
> In drivers/gpu/drm/gud/ (gud_drv.c, gud_pipe.c, gud_connector.c), ensure
> gdrm->bulk_len >= max_pitch so lines = bulk_len / pitch cannot be 0 in
> gud_flush_damage(), cap num_connectors to GUD_CONNECTORS_MAX, and reject
> short USB control transfers in gud_usb_get().
> 
> Fixes: 40e1a70b4aed ("drm: Add GUD USB Display driver")
> Assisted-by: LLM
> Signed-off-by: Hui Peng <benquike@xxxxxxxxx>
> ---
> diff --git a/drivers/gpu/drm/gud/gud_connector.c b/drivers/gpu/drm/gud/gud_connector.c
> index 8141c3a1e30a..1bf0495bf43f 100644
> --- a/drivers/gpu/drm/gud/gud_connector.c
> +++ b/drivers/gpu/drm/gud/gud_connector.c
> @@ -569,7 +569,7 @@ static int gud_connector_add_properties(struct gud_device *gdrm, struct gud_conn
>  			continue; /* not a DRM property */
>  
>  		property = gud_connector_property_lookup(connector, prop);
> -		if (drm_WARN_ON(drm, IS_ERR(property)))
> +		if (drm_WARN_ON(drm, IS_ERR_OR_NULL(property)))
>  			continue;
>  
>  		state_val = gud_connector_tv_state_val(prop, &gconn->initial_tv_state);
> diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c
> index 3a1b9e2a2eaa..f69b0e6e2ee0 100644
> --- a/drivers/gpu/drm/gud/gud_drv.c
> +++ b/drivers/gpu/drm/gud/gud_drv.c
> @@ -328,7 +328,7 @@ static int gud_stats_debugfs(struct seq_file *m, void *data)
>  		seq_puts(m, " none");
>  	seq_puts(m, "\n");
>  
> -	if (gdrm->compression) {
> +	if (gdrm->compression && gdrm->stats_actual_length) {
>  		u64 remainder;
>  		u64 ratio = div64_u64_rem(gdrm->stats_length, gdrm->stats_actual_length,
>  					  &remainder);
> @@ -427,6 +427,8 @@ static void gud_free_buffers_and_mutex(void *data)
>  {
>  	struct gud_device *gdrm = data;
>  
> +	vfree(gdrm->shadow_buf);
> +	gdrm->shadow_buf = NULL;
>  	vfree(gdrm->compress_buf);
>  	gdrm->compress_buf = NULL;
>  	sg_free_table(&gdrm->bulk_sgt);
> @@ -443,7 +445,7 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id)
>  	struct usb_endpoint_descriptor *bulk_out;
>  	struct gud_display_descriptor_req desc;
>  	struct device *dev = &intf->dev;
> -	size_t max_buffer_size = 0;
> +	size_t max_buffer_size = 0, max_pitch = 0;
>  	struct gud_device *gdrm;
>  	struct drm_device *drm;
>  	struct device *dma_dev;
> @@ -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;
>  	}
>  
>  	/* Mode config init */
> @@ -523,7 +529,7 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id)
>  	num_formats_dev = ret;
>  	for (i = 0; i < num_formats_dev; i++) {
>  		const struct drm_format_info *info;
> -		size_t fmt_buf_size;
> +		size_t fmt_buf_size, fmt_pitch;
>  		u32 format;
>  
>  		format = gud_to_fourcc(formats_dev[i]);
> @@ -562,8 +568,9 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id)
>  			break;
>  		}
>  
> -		fmt_buf_size = drm_format_info_min_pitch(info, 0, drm->mode_config.max_width) *
> -			       drm->mode_config.max_height;
> +		fmt_pitch = drm_format_info_min_pitch(info, 0, drm->mode_config.max_width);
> +		fmt_buf_size = fmt_pitch * drm->mode_config.max_height;
> +		max_pitch = max(max_pitch, fmt_pitch);
>  		max_buffer_size = max(max_buffer_size, fmt_buf_size);
>  
>  		if (format == GUD_DRM_FORMAT_R1 || format == GUD_DRM_FORMAT_XRGB1111)
> @@ -588,9 +595,13 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id)
>  
>  	if (desc.max_buffer_size)
>  		max_buffer_size = le32_to_cpu(desc.max_buffer_size);
> +	if (max_buffer_size < max_pitch)
> +		max_buffer_size = max_pitch;
>  	/* Prevent a misbehaving device from allocating loads of RAM. 4096x4096@XRGB8888 = 64 MB */
>  	if (max_buffer_size > SZ_64M)
>  		max_buffer_size = SZ_64M;
> +	if (max_buffer_size < max_pitch)
> +		return -EINVAL;
This feels like it can just be changed to a check for max_pitch >
SZ_64M || max_buffer_size > SZ_64M? Comparing max_buffer_size against
max_pitch twice seems... odd
>  
>  	gdrm->bulk_pipe = usb_sndbulkpipe(interface_to_usbdev(intf), usb_endpoint_num(bulk_out));
>  	gdrm->bulk_len = max_buffer_size;
> diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
> index 5ef887d8485a..1f08226b23b2 100644
> --- 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);
> +
>  	pitch = drm_format_info_min_pitch(format, 0, drm_rect_width(rect));
>  	len = pitch * drm_rect_height(rect);
>  	if (len > gdrm->bulk_len)
> @@ -327,7 +331,7 @@ static void gud_flush_damage(struct gud_device *gdrm, struct drm_framebuffer *fb
>  {
>  	struct drm_format_conv_state fmtcnv_state = DRM_FORMAT_CONV_STATE_INIT;
>  	const struct drm_format_info *format;
> -	unsigned int i, lines;
> +	unsigned int i, lines, block_width;
>  	size_t pitch;
>  	int ret;
>  
> @@ -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);
> +
>  	/* Split update if it's too big */
>  	pitch = drm_format_info_min_pitch(format, 0, drm_rect_width(damage));
>  	lines = drm_rect_height(damage);
>  
> +	if (!pitch || !lines)
> +		return;
> +
>  	if (gdrm->bulk_len < lines * pitch)
>  		lines = gdrm->bulk_len / pitch;
> +	if (!lines)
> +		return;
This also feels a bit odd, checking !lines twice, it shouldn't be
possible at this point for pitch < gdrm->bulk_len due to your changes
to max_pitch above, given that max_pitch should always be at least
min_pitch for all the formats.
>  
>  	for (i = 0; i < DIV_ROUND_UP(drm_rect_height(damage), lines); i++) {
>  		struct drm_rect rect = *damage;
> @@ -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;
> +		gud_clear_damage(gdrm);
> +	}
> +
>  	if (!gdrm->shadow_buf) {
>  		gdrm->shadow_buf = vcalloc(fb->pitches[0], fb->height);
>  		if (!gdrm->shadow_buf) {
> @@ -562,8 +582,8 @@ int gud_plane_atomic_check(struct drm_plane *plane,
>  			goto out;
>  		}
>  
> -		req->properties[num_properties + i].prop = cpu_to_le16(prop);
> -		req->properties[num_properties + i].val = cpu_to_le64(val);
> +		req->properties[num_properties].prop = cpu_to_le16(prop);
> +		req->properties[num_properties].val = cpu_to_le64(val);
Please rebase your patch, this has already been fixed upstream

Ruben
>  		num_properties++;
>  	}
>

Attachment: signature.asc
Description: This is a digitally signed message part