Bug 195559 - Double fetch problem in Linux-4.10.1/drivers/media/pci/saa7164/saa7164-bus.c
Summary: Double fetch problem in Linux-4.10.1/drivers/media/pci/saa7164/saa7164-bus.c
Status: NEW
Alias: None
Product: v4l-dvb
Classification: Unclassified
Component: dvb-other (show other bugs)
Hardware: All Linux
: P1 high
Assignee: dvb-other
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-24 02:30 UTC by Pengfei Wang
Modified: 2019-10-08 06:26 UTC (History)
6 users (show)

See Also:
Kernel Version: kernel 4.10.1
Subsystem:
Regression: No
Bisected commit-id:


Attachments
source files (8.68 KB, application/zip)
2017-04-24 02:30 UTC, Pengfei Wang
Details

Description Pengfei Wang 2017-04-24 02:30:09 UTC
Created attachment 255961 [details]
source files

Hi,

I found this double fetch problem in Linux-4.10.1/drivers/media/pci/saa7164/saa7164-bus.c when I was examining the source code.

In this file, function saa7164_bus_get() uses memcpy_fromio() to fetch device data from I/O memory to the kernel (driver). However, a double fetch problem exists in this procedure. It first uses line 384 and 385 to fetch memory data from bus->m_pdwGetRing to local buffer msg_tmp to do a series of checks with the desired data at line 405, including ->size and ->seqno fields. After the checks are satisfied, a second fetch (line 446 and 447) copies the data again from bus->m_pdwGetRing to msg for later use. This double-fetch situation is vulnerable here because the secondly fetched data in msg cannot be guaranteed to satisfied the prior conditions at line 405. Once the ->size or ->seqno of the secondly fetched message changes,  aka an invalid message from the device, using such a message is likely to cause memory access related problems.

I further traced the invocation of this function and found a buggy case caused by this problem, which is in file Linux-4.10.1/drivers/media/pci/saa7164/saa7164-cmd.c. At line 552, function saa7164_bus_get() is called to get the message from the device, and the message is stored in tRsp. Then at line 173, tRsp is passed to function saa7164_cmd_free_seqno(), within which ->seqno field of the message is used again to access an array (dev->cmd[tRsp.seqno]) without an additional validation of tRsp.seqno. If an invalid message is fetched in the second fetch, and the ->seqno field of that message is changed to a very large value, an array over-access error could be caused.

In fact, there are several invocations of function saa7164_bus_get() in saa7164-cmd.c, and either ->size or ->seqno field from the fetched message is used in these invocations. However, these cases except the above one use additional checks (e.g. from line 529 to 535) to prevent buggy results caused by invalid messages. I hope the above-mentioned problem can be fixed, and looking forward to a reply on this, thank you!

Kind regards
Pengfei Wang
Comment 1 Bjorn Helgaas 2017-04-24 16:31:12 UTC
Looks like a driver issue, not a PCI core issue.  Reassigning accordingly.
Comment 2 Pengfei Wang 2017-04-25 01:07:00 UTC
I come up with the following patch for this problem by adding another check after the second fetch of the message. After this additional check, the whole message is guaranteed to be unchanged, and any later use of any fields of this message is sure to be safe. My solution is simple and straightforward, but it still needs to be confirmed by the maintainer. Maybe the maintainer has a better plan for this or has other concerns. 
I hope this helps. Thanks.

==========================================================================
--- saa7164-bus.c	2017-02-26 18:09:33.000000000 +0800
+++ saa7164-bus-patched.c	2017-04-24 19:11:29.000000000 +0800
@@ -479,6 +479,23 @@
 	msg->command = le32_to_cpu((__force __le32)msg->command);
 	msg->controlselector = le16_to_cpu((__force __le16)msg->controlselector);
 
+	/* Check again after the second fetch that 
+	* the command/response matches what is expected, 
+	* msg_tmp is the previously validated message, 
+	* msg is the new fetched message. 
+	*/
+	if ((msg_tmp.id != msg->id) || (msg_tmp.command != msg->command) ||
+		(msg_tmp.controlselector != msg->controlselector) ||
+		(msg_tmp.seqno != msg->seqno) || (msg_tmp.size != msg->size)) {
+
+		printk(KERN_ERR "%s() Unexpected msg miss-match (double fetch)\n", __func__);
+		saa7164_bus_dumpmsg(dev, msg, buf);
+		saa7164_bus_dumpmsg(dev, &msg_tmp, NULL);
+		ret = SAA_ERR_INVALID_COMMAND;
+		goto out;
+	}
+
+
 	/* Update the read positions, adjusting the ring */
 	saa7164_writel(bus->m_dwGetReadPos, new_grp);
 ==========================================================================
Comment 3 Rajasekar 2017-11-27 22:16:22 UTC
Hi Pengfei Wang,

Please let me know the steps to apply the patch in RHEL 7.
Comment 4 Pengfei Wang 2017-11-29 01:52:01 UTC
(In reply to Rajasekar from comment #3)
> Hi Pengfei Wang,
> 
> Please let me know the steps to apply the patch in RHEL 7.

Hello Rajasekar,

The kernel maintainer has applied another patch for this issue (
https://github.com/stoth68000/media-tree/commit/354dd3924a2e43806774953de536257548b5002c ).
You may checkout the patched file.


Regards
Pengfei
Comment 5 Rajasekar 2017-11-29 19:03:18 UTC
Pengfei,

I am running the RHEL7 VM, i dont find the bus module on my machine to update the values under

/usr/lib/modules/3.10.0-693.5.2.el7.x86_64/kernel/drivers/media/pci/saa7164
# ls -lrt
total 48
-rw-r--r-- 1 root root 46160 Oct 13 08:08 saa7164.ko.xz
======
Kernel version - Linux ****** 3.10.0-693.5.2.el7.x86_64 #1 SMP Fri Oct 13 10:46:25 EDT 2017 x86_64 x86_64 x86_64 GNU/Linux
Comment 6 Pengfei Wang 2017-12-01 01:41:54 UTC
This problem was discovered from Linux kernel-4.10. I think your kernel (3.10.0) does not equip with this module.

Pengfei
Comment 8 Carlo B 2018-07-05 16:53:52 UTC
Hello Pengfei, does this issue fixed already? Or any source? Thanks

Castro B.
https://alternatives.co/
Comment 9 Pengfei Wang 2018-07-09 00:15:46 UTC
(In reply to Carlo B from comment #8)
> Hello Pengfei, does this issue fixed already? Or any source? Thanks
> 
> Castro B.
> https://alternatives.co/

Hello Castro,

Yes, this problem had been fixed already.
Please see CVE-2017-8831, and the fix:
https://github.com/stoth68000/media-tree/commit/354dd3924a2e43806774953de536257548b5002c

Pengfei

Note You need to log in before you can comment on or make changes to this bug.