Re: [PATCH RFC 3/4] drm/fl2000: Add frame streaming

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] drm/fl2000: out-of-bounds shadow write after fl2000_stream_mode_set() error
- [High] drm/fl2000: NULL pointer dereference of sb->pages in fl2000_free_sb()
- [High] drm/fl2000: infinite retry loop on -EAGAIN in fl2000_submit_urb()
- [High] drm/fl2000: invalid URB transfer_buffer usage in fl2000_stream_work()
- [Medium] drm/fl2000: missing flush_kernel_vmap_range() before SG DMA in fl2000_stream_work()
--

commit 3826e48192d62fc9fe0e2b4c5ca7c2e42d218844
Author: Angel Gabriel Crispin Valdivia <angelcrispinvaldivia@gmail.com>

drm/fl2000: Add frame streaming

This commit adds frame streaming support for the FL2000 USB display
adapter, managing a pool of scatter-gather URB buffers, a shadow
framebuffer for damage updates, pixel format conversion, and cursor
blending.

> diff --git a/drivers/gpu/drm/fl2000/fl2000.h b/drivers/gpu/drm/fl2000/fl2000.h
> index 95603247907cf..d800d943e6e47 100644
> --- a/drivers/gpu/drm/fl2000/fl2000.h
> +++ b/drivers/gpu/drm/fl2000/fl2000.h
> @@ -42,6 +42,48 @@ enum fl2000_interface {
>  	FL2000_USBIF_INTERRUPT = 2,
>  };
>  
> +/* Process context only: may sleep between retries */
> +static inline int fl2000_submit_urb(struct urb *urb)
> +{
> +	int ret;
> +	int attempts = 10;
> +
> +	do {
> +		ret = usb_submit_urb(urb, GFP_KERNEL);
> +		switch (ret) {
> +		case -ENXIO:
> +		case -ENOMEM:
> +			if (attempts--) {
> +				cond_resched();
> +				ret = -EAGAIN;
> +			}
> +			break;
> +		default:
> +			break;
> +		}
> +	} while (ret == -EAGAIN);
> +
> +	return ret;
> +}

[Severity: High]
What happens in fl2000_submit_urb() in drivers/gpu/drm/fl2000/fl2000.h if
usb_submit_urb() returns -EAGAIN directly?

For example, usb_hcd_map_urb_for_dma() in drivers/usb/core/hcd.c and
xhci_map_temp_buffer() in drivers/usb/host/xhci.c return -EAGAIN when DMA
mapping fails (such as dma_map_sg() or dma_map_single()).

When usb_submit_urb() returns -EAGAIN, the switch statement hits the
default branch without decrementing attempts or calling cond_resched(),
while the loop condition (ret == -EAGAIN) remains true. Could this spin
indefinitely in fl2000_stream_work() -> fl2000_stream_submit() ->
fl2000_submit_urb()?

[ ... ]

> diff --git a/drivers/gpu/drm/fl2000/fl2000_streaming.c b/drivers/gpu/drm/fl2000/fl2000_streaming.c
> new file mode 100644
> index 0000000000000..2d2c30cb74424
> --- /dev/null
> +++ b/drivers/gpu/drm/fl2000/fl2000_streaming.c

[ ... ]

> +static void fl2000_free_sb(struct fl2000_stream_buf *sb)
> +{
> +	vunmap(sb->vaddr);
> +
> +	sg_free_table(&sb->sgt);
> +
> +	for (int i = 0; i < sb->nr_pages && sb->pages[i]; i++)
> +		__free_page(sb->pages[i]);
> +
> +	kfree(sb->pages);
> +
> +	kfree(sb);
> +}
> +
> +static struct fl2000_stream_buf *fl2000_alloc_sb(unsigned int size)
> +{
> +	int ret;
> +	struct fl2000_stream_buf *sb;
> +	unsigned int nr_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;
> +
> +	sb = kzalloc_obj(*sb);
> +	if (!sb)
> +		return NULL;
> +
> +	sb->nr_pages = nr_pages;
> +
> +	sb->pages = kzalloc_objs(*sb->pages, nr_pages);
> +	if (!sb->pages)
> +		goto error;
> +
> +	for (int i = 0; i < nr_pages; i++) {
> +		sb->pages[i] = alloc_page(GFP_KERNEL);
> +		if (!sb->pages[i])
> +			goto error;
> +	}
> +
> +	ret = sg_alloc_table_from_pages(&sb->sgt, sb->pages, nr_pages, 0, size, GFP_KERNEL);
> +	if (ret != 0)
> +		goto error;
> +
> +	sb->vaddr = vmap(sb->pages, nr_pages, VM_MAP, PAGE_KERNEL);
> +	if (!sb->vaddr)
> +		goto error;
> +
> +	INIT_LIST_HEAD(&sb->list);
> +	memset(sb->vaddr, 0, nr_pages << PAGE_SHIFT);
> +
> +	return sb;
> +
> +error:
> +	fl2000_free_sb(sb);
> +
> +	return NULL;
> +}

[Severity: High]
In drivers/gpu/drm/fl2000/fl2000_streaming.c, fl2000_alloc_sb() sets
sb->nr_pages = nr_pages before allocating sb->pages with kzalloc_objs().
If that allocation fails, it jumps to error and calls fl2000_free_sb(sb).

In fl2000_free_sb(), the loop checks:

	for (int i = 0; i < sb->nr_pages && sb->pages[i]; i++)

When sb->nr_pages is non-zero and sb->pages is NULL, won't evaluating
sb->pages[0] dereference a NULL pointer?

[ ... ]

> +static void fl2000_stream_data_completion(struct urb *urb)
> +{
> +	struct fl2000_stream_buf *cur_sb = urb->transfer_buffer;
> +	struct usb_device *usb_dev = urb->dev;
> +	struct fl2000_stream *stream = urb->context;
> +	unsigned long flags;
> +
> +	if (stream) {
> +		spin_lock_irqsave(&stream->list_lock, flags);
> +		list_move_tail(&cur_sb->list, &stream->render_list);
> +		spin_unlock_irqrestore(&stream->list_lock, flags);

[ ... ]

> +		/*
> +		 * Building the frame copies up to several MB and must run with
> +		 * interrupts enabled, so cur_sb is owned (off all lists) for
> +		 * the duration. Holding list_lock (which disables IRQs) across
> +		 * it starves the whole system.
> +		 */
> +		fl2000_stream_build(stream, cur_sb);
> +
> +		spin_lock_irq(&stream->list_lock);
> +		list_add_tail(&cur_sb->list, &stream->wait_list);
> +		spin_unlock_irq(&stream->list_lock);

[Severity: Medium]
In fl2000_alloc_sb() in drivers/gpu/drm/fl2000/fl2000_streaming.c, the
scatter-gather pages in sb->sgt are mapped into a virtually contiguous
kernel range via vmap() at sb->vaddr, and fl2000_stream_build() writes the
frame and cursor data through cur_sb->vaddr.

When fl2000_stream_work() then submits data_urb with data_urb->sg and
data_urb->num_sgs, usb_hcd_map_urb_for_dma() calls dma_map_sg(), which only
flushes the linear direct-map alias for each scatterlist page rather than
the vmap() alias cur_sb->vaddr.

On architectures with virtually indexed (VIVT or aliasing VIPT) caches
where ARCH_IMPLEMENTS_FLUSH_KERNEL_VMAP_RANGE is set, does
fl2000_stream_work() need to call flush_kernel_vmap_range(cur_sb->vaddr,
stream->buf_size) after fl2000_stream_build() and before submitting the URB
so dirty cache lines in the vmap alias are written back before DMA?

> +
> +		data_urb = usb_alloc_urb(0, GFP_KERNEL);
> +		eof_urb = usb_alloc_urb(0, GFP_KERNEL);
> +		if (!data_urb || !eof_urb) {
> +			usb_free_urb(data_urb);
> +			usb_free_urb(eof_urb);
> +			dev_err(&usb_dev->dev, "Data URB allocation error\n");
> +			WRITE_ONCE(stream->enabled, false);
> +			return;
> +		}
> +
> +		/*
> +		 * Endpoint 1 bulk out. We store pointer to current stream buffer structure in
> +		 * transfer_buffer field of URB which is unused due to SGT.
> +		 */
> +		usb_fill_bulk_urb(data_urb, usb_dev, usb_sndbulkpipe(usb_dev, 1), cur_sb,
> +				  (int)stream->buf_size, fl2000_stream_data_completion, stream);
> +		data_urb->interval = 0;
> +		data_urb->sg = cur_sb->sgt.sgl;
> +		data_urb->num_sgs = cur_sb->sgt.nents;

[Severity: High]
Is it safe to store cur_sb in data_urb->transfer_buffer while setting
data_urb->transfer_buffer_length to stream->buf_size in
fl2000_stream_work()?

Even when urb->sg and urb->num_sgs are set, several paths in the USB core
and host controller drivers inspect or overwrite urb->transfer_buffer:

1. usb_submit_urb() calls kmsan_handle_urb(), which checks
urb->transfer_buffer_length bytes starting at urb->transfer_buffer when
urb->transfer_buffer is non-NULL, reading stream->buf_size bytes (several
megabytes) out-of-bounds past the cur_sb slab object.

2. In usb_hcd_map_urb_for_dma(), HCDs with hcd->localmem_pool check
localmem_pool before urb->num_sgs and call hcd_alloc_coherent(), which
copies urb->transfer_buffer_length bytes out-of-bounds from cur_sb and
overwrites urb->transfer_buffer.

3. In xhci_map_urb_for_dma(), when XHCI_SG_TRB_CACHE_SIZE_QUIRK triggers
xhci_map_temp_buffer(), xHCI replaces urb->transfer_buffer with a temporary
bounce buffer and xhci_unmap_temp_buf() sets urb->transfer_buffer = NULL
before completion, causing fl2000_stream_data_completion() to read cur_sb
as NULL and dereference it at list_move_tail(&cur_sb->list,
&stream->render_list).

Could data_urb->transfer_buffer be left NULL and urb->context (with a back
pointer in struct fl2000_stream_buf to struct fl2000_stream) be used
instead?

[ ... ]

> +void fl2000_stream_compress(struct fl2000_stream *stream, const struct iosys_map *src_map,
> +			    bool imported, u32 format, unsigned int width, unsigned int height,
> +			    unsigned int pitch, const struct drm_rect *clip)
> +{
> +	bool wc = imported || src_map->is_iomem;
> +	u32 dst_line_len;
> +	u32 x_start, x_end, y_start, y_end;
> +	size_t span;
> +
> +	x_start = min_t(u32, max_t(int, clip->x1, 0), width);
> +	x_end = min_t(u32, max_t(int, clip->x2, 0), width);
> +	y_start = min_t(u32, max_t(int, clip->y1, 0), height);
> +	y_end = min_t(u32, max_t(int, clip->y2, 0), height);
> +	if (x_start >= x_end || y_start >= y_end)
> +		return;
> +
> +	guard(mutex)(&stream->shadow_lock);
> +
> +	if (!stream->shadow || width != stream->width || height != stream->height)
> +		return;

[ ... ]

> +int fl2000_stream_mode_set(struct fl2000_stream *stream, unsigned int width,
> +			   unsigned int height, u32 bytes_pix)
> +{
> +	int ret;
> +	unsigned int size;
> +	void *shadow;
> +
> +	/* Round buffer size up to multiple of 8 to meet HW expectations */
> +	size = ALIGN(width * height * bytes_pix, 8);
> +
> +	guard(mutex)(&stream->shadow_lock);
> +
> +	stream->bytes_pix = bytes_pix;
> +	stream->width = width;
> +	stream->height = height;
> +
> +	/* If there are buffers with same size - keep them */
> +	if (stream->buf_size == size)
> +		return 0;
> +
> +	/* Destroy wrong size buffers if they exist */
> +	if (!list_empty(&stream->render_list))
> +		fl2000_stream_put_buffers(stream);
> +
> +	/* Allocate new buffers */
> +	ret = fl2000_stream_get_buffers(stream, size);
> +	if (ret) {
> +		fl2000_stream_put_buffers(stream);
> +		stream->buf_size = 0;
> +		return ret;
> +	}
> +
> +	/* One full frame in the device format; only the CPU ever reads it */
> +	shadow = vzalloc(size);
> +	if (!shadow) {
> +		fl2000_stream_put_buffers(stream);
> +		stream->buf_size = 0;
> +		return -ENOMEM;
> +	}
> +	vfree(stream->shadow);
> +	stream->shadow = shadow;

[Severity: Critical]
In fl2000_stream_mode_set() in drivers/gpu/drm/fl2000/fl2000_streaming.c,
stream->bytes_pix, stream->width, and stream->height are updated to the new
mode dimensions before fl2000_stream_get_buffers(stream, size) and
vzalloc(size) succeed.

If a smaller mode was previously active and switching to a larger mode
fails in fl2000_stream_get_buffers() or vzalloc(), the error paths set
stream->buf_size = 0 but leave stream->shadow pointing to the old, smaller
buffer while stream->width, stream->height, and stream->bytes_pix hold the
new, larger dimensions.

When fl2000_stream_compress() is subsequently called for the new mode, its
check:

	if (!stream->shadow || width != stream->width || height != stream->height)
		return;

succeeds because stream->shadow is non-NULL and width and height match the
updated fields. Doesn't the conversion loop then write up to
width * height * stream->bytes_pix bytes past the end of the old smaller
stream->shadow allocation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-fl2000-upstream-v1-0-bb163ecb2309@gmail.com?part=3