media: cx231xx: short bulk transfer overflow in cx231xx_bulk_copy()

[Maoyi Xie]

Hi all,

I think cx231xx_bulk_copy() in drivers/media/usb/cx231xx/cx231xx-417.c can
overflow the heap when the device returns a very short bulk transfer. I would
appreciate it if you could take a look.

The buffer is sized from the device transfer length, then a fixed 3 byte
header and the payload are copied into it.

	buffer_size = urb->actual_length;
	buffer = kmalloc(buffer_size, GFP_ATOMIC);
	...
	memcpy(buffer, dma_q->ps_head, 3);
	memcpy(buffer + 3, p_buffer, buffer_size - 3);

Nothing checks that actual_length is at least 3. If the device completes the
bulk IN with fewer than 3 bytes, buffer_size - 3 is unsigned and wraps to
about 4 GiB, so the second memcpy copies far past the small allocation.

The device controls actual_length, so a malicious or malfunctioning USB
device on the cx23417 bulk IN endpoint can trigger this.

The matching copy path in the same file guards its length before use.

	if (buffer_size > 0)
		buffer_copy(dev, p_buffer, buffer_size, urb, dma_q);

cx231xx_bulk_copy() has no such check.

I reproduced this on 7.1-rc7 by running the same copy with buffer_size set to
2. The buffer_size minus 3 subtraction wraps and the copy runs off the heap.

  CX231XX-POC: buffer=kmalloc(2) memcpy len=buffer_size-3=4294967295
  BUG: unable to handle page fault for address: ffff8881079ac377
  RIP: 0010:memcpy_orig+0x54/0x130

I should be honest that the bulk path is not the default transfer mode here,
so I am treating this as short packet handling rather than a default config
issue. A lower bound check at the top of the function would close it.

	buffer_size = urb->actual_length;

	if (buffer_size < 4)
		return -EINVAL;

	buffer = kmalloc(buffer_size, GFP_ATOMIC);

Does this look like a real bug to you, and is the driver the right place to
reject short frames, or is the encoder expected to always return at least a
few bytes? If a fix makes sense I am happy to send a proper patch.

Thanks,
Maoyi
https://maoyixie.com/