[CBR 7.9] media: technisat-usb2: break out of loop at end of buffer

[pvts-mat]

[CBR 7.9]
CVE-2019-15505
VULN-7734

Problem

https://www.cve.org/CVERecord?id=CVE-2019-15505

drivers/media/usb/dvb-usb/technisat-usb2.c in the Linux kernel through 5.2.9 has an out-of-bounds read via crafted USB device traffic (which may be remote via usbip or usbredir).

More concrete info about the issue in the fixing commit 0c4df39:

media: technisat-usb2: break out of loop at end of buffer

Ensure we do not access the buffer beyond the end if no 0xff byte
is encountered.

Applicability: yes

The driver is enabled in configs/kernel-3.10.0-x86_64.config

kernel-src-tree/configs/kernel-3.10.0-x86_64.config

Line 3808 in 3f2bd20

CONFIG_DVB_USB_TECHNISAT_USB2=m

Moreover, the affected function technisat_usb2_get_ir from https://github.com/ctrliq/kernel-src-tree/blob/ciqcbr7_9/drivers/media/usb/dvb-usb/technisat-usb2.c clearly doesn't contain any boundary check for b, relying instead solely on the *b == 0xff condition to break the loop:

kernel-src-tree/drivers/media/usb/dvb-usb/technisat-usb2.c

Lines 640 to 652 in 3f2bd20

	while (1) {
	ev.pulse = !ev.pulse;
	ev.duration = (*b * FIRMWARE_CLOCK_DIVISOR * FIRMWARE_CLOCK_TICK) / 1000;
	ir_raw_event_store(d->rc_dev, &ev);
	b++;
	if (*b == 0xff) {
	ev.pulse = 0;
	ev.duration = 888888*2;
	ir_raw_event_store(d->rc_dev, &ev);
	break;
	}
	}

Solution

The essence of mainline solution 0c4df39 can be captured in the following points:

1. Changing the way buffer elements are accessed: from dereferencing a crawling pointer b to indexing a constant pointer buf pointing at the beginning of the buffer, and introducing the necessary for that index i.
2. Replacing the infinite loop while(1) with a for loop controlling the increments of i and its upper bound - the size of the buffer.
3. Reordering the loop's break condition relative to the rest of the loop's body, such that the checking for 0xff value starts from the buf + 1 element instead of buf + 2.

Breaking down the change in such way is necessary to apply the patch correctly as the ciqcbr7_9 codebase misses the 5d0f2df commit on which the mainline fix 0c4df39 is based: there is no d->priv->buf to operate on. The proposed fix preserves all 3 modifications with the buffer located on stack, in the local buf array, instead of d->priv->buf.

Notes:

• It was decided to modify the patch instead of applying 5d0f2df as the prerequisite because the commit is quite extensive and thus risky.
• The ARRAY_SIZE macro works for buf in ciqcbr7_9 just as it works for state->buf in the mainline - both arrays are defined as static
  • ciqcbr7_9

    kernel-src-tree/drivers/media/usb/dvb-usb/technisat-usb2.c

    Line 594 in 3f2bd20

    u8 buf[62], *b;

  • kernel-mainline

    kernel-src-tree/drivers/media/usb/dvb-usb/technisat-usb2.c

    Line 59 in 6832a93

    u8 buf[64];

• The buffer sizes differ in ciqcbr7_9 and in the mainline - 62 vs 64 - so the proposed fix is not 100% functionally equivalent to 0c4df39. However, if the size 62 was appropriate for the driver in ciqcbr7_9 so far then it should be preserved - the CVE-2019-15505 bug and its patch aren't concerned with the buffer sizes but with their checking.
• For additional reassurance see a very similar official backport to stable 3.16: 2389a65

kABI check: passed

[root@ciqcbr-7-9 pvts]# python /mnt/code/kernel-dist-git-el-7.9/SOURCES/check-kabi -k /mnt/code/kernel-dist-git-el-7.9/SOURCES/Module.kabi_x86_64 -s /mnt/build_files/kernel-src-tree-ciqcbr7_9-CVE-2019-15505/Module.symvers
[root@ciqcbr-7-9 pvts]# echo $? 
0

Boot test: passed

boot-test.log

Kselftests: passed relative

Reference

kselftests–ciqcbr7_9–run1.log
kselftests–ciqcbr7_9–run2.log
kselftests–ciqcbr7_9–run3.log

Patch

kselftests–ciqcbr7_9-CVE-2019-15505–run1.log
kselftests–ciqcbr7_9-CVE-2019-15505–run2.log
kselftests–ciqcbr7_9-CVE-2019-15505–run3.log

Comparison

The results were compared manually with Meld. No changes indicative of some newly introduced malfunctions were found.

Specific tests: skipped

[PlaidCat]

The buffer sizes differ in ciqcbr7_9 and in the mainline - 62 vs 64 - so the proposed fix is not 100% functionally equivalent to 0c4df39. However, if the size 62 was appropriate for the driver in ciqcbr7_9 so far then it should be preserved - the GHSA-8q2x-x7hh-j593 bug and its patch aren't concerned with the buffer sizes but with their checking.

Thank You for this clarification.

[thefossguy-ciq]

🚤