Bug 196133

Summary: Double fetch problem in Linux-4.10.1/sound/isa/msnd/msnd_midi.c
Product: Drivers Reporter: Pengfei Wang (wpengfeinudt)
Component: Sound(ALSA)Assignee: Jaroslav Kysela (perex)
Status: NEW ---    
Severity: normal CC: tiwai, wpengfeinudt
Priority: P1    
Hardware: All   
OS: Linux   
Kernel Version: 4.10.1 Subsystem:
Regression: No Bisected commit-id:
Attachments: source file

Description Pengfei Wang 2017-06-20 13:03:55 UTC
Created attachment 257087 [details]
source file

Hi,

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

In this file, function snd_msndmidi_input_read() 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 126 to fetch the header pointer (mpu->dev->MIDQ + JQS_wHead) of a message queue and check it with the tail pointer to proceed the loop. Then, at line 128, it fetches the header pointer again to use it to get the message value from the queue. 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 126,128,135). 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 139.

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 10:07:06 UTC
Basically same as bug 196131.

IMO, it's an optimization issue rather than the security problem.
Comment 2 Pengfei Wang 2017-08-05 01:30:15 UTC
We proposed a patch for this issue, hoping it can be helpful, thanks!
=====================================================================
--- msnd_midi.c	2017-06-20 21:02:14.000000000 +0800
+++ msnd_midi-patched.c	2017-06-27 09:00:18.000000000 +0800
@@ -122,21 +122,27 @@
 	void *pwMIDQData = mpu->dev->mappedbase + MIDQ_DATA_BUFF;
 
 	spin_lock_irqsave(&mpu->input_lock, flags);
-	while (readw(mpu->dev->MIDQ + JQS_wTail) !=
-	       readw(mpu->dev->MIDQ + JQS_wHead)) {
+
+	u16 tail = readw(mpu->dev->MIDQ + JQS_wTail);
+	u16 head = readw(mpu->dev->MIDQ + JQS_wHead);
+
+	while (tail != head) {
 		u16 wTmp, val;
-		val = readw(pwMIDQData + 2 * readw(mpu->dev->MIDQ + JQS_wHead));
+		val = readw(pwMIDQData + 2 * head);
 
 			if (test_bit(MSNDMIDI_MODE_BIT_INPUT_TRIGGER,
 				     &mpu->mode))
 				snd_rawmidi_receive(mpu->substream_input,
 						    (unsigned char *)&val, 1);
 
-		wTmp = readw(mpu->dev->MIDQ + JQS_wHead) + 1;
+		wTmp = head + 1;
 		if (wTmp > readw(mpu->dev->MIDQ + JQS_wSize))
 			writew(0,  mpu->dev->MIDQ + JQS_wHead);
 		else
 			writew(wTmp,  mpu->dev->MIDQ + JQS_wHead);
+
+		tail = readw(mpu->dev->MIDQ + JQS_wTail);
+		head = readw(mpu->dev->MIDQ + JQS_wHead);
 	}
 	spin_unlock_irqrestore(&mpu->input_lock, flags);
 }
======================================================================