Bug 196131 - Double fetch problem in Linux-4.10.1/sound/isa/msnd/msnd_pinnacle.c
Summary: Double fetch problem in Linux-4.10.1/sound/isa/msnd/msnd_pinnacle.c
Status: NEW
Alias: None
Product: Drivers
Classification: Unclassified
Component: Sound(ALSA) (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: Jaroslav Kysela
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-06-20 12:54 UTC by Pengfei Wang
Modified: 2017-08-05 01:28 UTC (History)
2 users (show)

See Also:
Kernel Version: 4.10.1
Tree: Mainline
Regression: No


Attachments
source file (30.35 KB, text/x-csrc)
2017-06-20 12:54 UTC, Pengfei Wang
Details

Description Pengfei Wang 2017-06-20 12:54:58 UTC
Created attachment 257085 [details]
source file

Hi,

I found this double fetch problem in Linux-4.10.1/sound/isa/msnd/msnd_pinnacle.c
 when I was examining the source code.

In this file, function snd_msnd_interrupt() uses readw() to fetch peripheral device data from I/O memory to the kernel (driver). However, a double fetch problem exists in this procedure. It first uses line 178 to fetch the header pointer (chip->DSPQ + JQS_wHead) of a message queue and check it with the tail pointer to proceed the loop. Then, at line 182, it fetches the header pointer again to use it to get the message from the queue and send it by function snd_msnd_eval_dsp_msg(). This double-fetch situation of the header pointer is problematic because the value of the pointer is likely to be changed (e.g. by unexpected data flip or intentionally compromised hardware) between the check and the use of the pointer. Once the pointer value is changed, a result like an array (queue) over-access could be caused.

The pointer is fetched from the I/O memory multiple times in this function (line 178,182,184). A secure and effective way to handle this pointer is to read it to a local variable the first time and use the variable instead until it is written back at line 188.

Iā€™m looking forward to a reply to confirm this, and if I am right, please fix it. Thanks.

Kind regards
Pengfei Wang
Comment 1 Takashi Iwai 2017-06-29 09:59:13 UTC
(In reply to Pengfei Wang from comment #0)
> This double-fetch situation of the header
> pointer is problematic because the value of the pointer is likely to be
> changed (e.g. by unexpected data flip or intentionally compromised hardware)
> between the check and the use of the pointer. Once the pointer value is
> changed, a result like an array (queue) over-access could be caused.

If you assume such a hardware behavior, any code can't be safe :)

> The pointer is fetched from the I/O memory multiple times in this function
> (line 178,182,184). A secure and effective way to handle this pointer is to
> read it to a local variable the first time and use the variable instead
> until it is written back at line 188.

I agree that using a local variable would improve the code performance, but it doesn't help much about the security if it's under the assumption of a hardware failure.

That said, I appreciate the report and will be happy to apply the fix patches you can think of.  But it's a overreaction to treat this as a security flaw, especially without actual examples of DoS.  Thanks.
Comment 2 Pengfei Wang 2017-08-05 01:28:50 UTC
We proposed a patch for this issue, hoping it can be helpful, thanks!
=====================================================================
--- msnd_pinnacle.c	2017-06-20 20:39:59.000000000 +0800
+++ msnd_pinnacle-patched.c	2017-06-27 09:31:31.000000000 +0800
@@ -175,17 +175,21 @@ 			clear_bit(F_READING, &chip->flags);
 	/* inb(chip->io + HP_RXL); */
 
 	/* Evaluate queued DSP messages */
-	while (readw(chip->DSPQ + JQS_wTail) != readw(chip->DSPQ + JQS_wHead)) {
+	u16 head = readw(chip->DSPQ + JQS_wHead);
+	u16 tail = readw(chip->DSPQ + JQS_wTail);
+	while (tail != head) {
 		u16 wTmp;
 
-		snd_msnd_eval_dsp_msg(chip,
-			readw(pwDSPQData + 2 * readw(chip->DSPQ + JQS_wHead)));
+		snd_msnd_eval_dsp_msg(chip, readw(pwDSPQData + 2 * head));
 
-		wTmp = readw(chip->DSPQ + JQS_wHead) + 1;
+		wTmp = head + 1;
 		if (wTmp > readw(chip->DSPQ + JQS_wSize))
 			writew(0, chip->DSPQ + JQS_wHead);
 		else
 			writew(wTmp, chip->DSPQ + JQS_wHead);
+
+		head = readw(chip->DSPQ + JQS_wHead);
+		tail = readw(chip->DSPQ + JQS_wTail);
 	}
 	/* Send ack to DSP */
 	inb(chip->io + HP_RXL);
=======================================================================

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