[PATCH] ALSA: msnd: Optimize / harden DSP and MIDI loops

[PATCH] ALSA: msnd: Optimize / harden DSP and MIDI loops

[grygorii tertychnyi]

Hi Greg,

Could you please apply it for 4.4-stable.
This fixes https://nvd.nist.gov/vuln/detail/CVE-2017-9985

Takashi Iwai (1):
  ALSA: msnd: Optimize / harden DSP and MIDI loops

 sound/isa/msnd/msnd_midi.c     | 30 +++++++++++++++---------------
 sound/isa/msnd/msnd_pinnacle.c | 23 ++++++++++++-----------
 2 files changed, 27 insertions(+), 26 deletions(-)

-- 
2.10.3.dirty

Re: [PATCH] ALSA: msnd: Optimize / harden DSP and MIDI loops

[Greg KH]

On Fri, Sep 08, 2017 at 09:06:25AM -0700, grygorii tertychnyi wrote:
> Hi Greg,
> 
> Could you please apply it for 4.4-stable.
> This fixes https://nvd.nist.gov/vuln/detail/CVE-2017-9985

Why just 4.4?  What about 4.12, 4.9, and any others?

thanks,

greg k-h

Re: [PATCH] ALSA: msnd: Optimize / harden DSP and MIDI loops

[Takashi Iwai]

On Fri, 08 Sep 2017 18:06:25 +0200,
grygorii tertychnyi wrote:
> 
> Hi Greg,
> 
> Could you please apply it for 4.4-stable.
> This fixes https://nvd.nist.gov/vuln/detail/CVE-2017-9985

This vulnerability is just non-issue.  You can't get it working
practically; it requires a modified hardware of the decade old ISA
sound card, and yet the system has to load / set up the module
beforehand.  We should withdraw it from CVE, IMO.


thanks,

Takashi

> 
> Takashi Iwai (1):
>   ALSA: msnd: Optimize / harden DSP and MIDI loops
> 
>  sound/isa/msnd/msnd_midi.c     | 30 +++++++++++++++---------------
>  sound/isa/msnd/msnd_pinnacle.c | 23 ++++++++++++-----------
>  2 files changed, 27 insertions(+), 26 deletions(-)
> 
> -- 
> 2.10.3.dirty
> 

Re: [PATCH] ALSA: msnd: Optimize / harden DSP and MIDI loops

[Grygorii Tertychnyi (gtertych)]

>> Hi Greg,
>>
>> Could you please apply it for 4.4-stable.
>> This fixes https://nvd.nist.gov/vuln/detail/CVE-2017-9985
>
> This vulnerability is just non-issue.  You can't get it working
> practically; it requires a modified hardware of the decade old ISA
> sound card, and yet the system has to load / set up the module
> beforehand.  We should withdraw it from CVE, IMO.

I think it is worth having it in 4.4, 4.9 and 4.12 also.

>>
>> Takashi Iwai (1):
>>   ALSA: msnd: Optimize / harden DSP and MIDI loops
>>
>>  sound/isa/msnd/msnd_midi.c     | 30 +++++++++++++++---------------
>>  sound/isa/msnd/msnd_pinnacle.c | 23 ++++++++++++-----------
>>  2 files changed, 27 insertions(+), 26 deletions(-)
>>

Re: [alsa-devel] [PATCH] ALSA: msnd: Optimize / harden DSP and MIDI loops

[Takashi Iwai]

On Fri, 08 Sep 2017 19:47:32 +0200,
Grygorii Tertychnyi (gtertych) wrote:
> 
> 
> >> Hi Greg,
> >>
> >> Could you please apply it for 4.4-stable.
> >> This fixes https://nvd.nist.gov/vuln/detail/CVE-2017-9985
> >
> > This vulnerability is just non-issue.  You can't get it working
> > practically; it requires a modified hardware of the decade old ISA
> > sound card, and yet the system has to load / set up the module
> > beforehand.  We should withdraw it from CVE, IMO.
> 
> I think it is worth having it in 4.4, 4.9 and 4.12 also.

... even though the code has never been tested on the real hardware?
That doesn't sound good for stable kernels at all.  That's why I
didn't put Cc to stable in the patch.


Takashi

Re: [alsa-devel] [PATCH] ALSA: msnd: Optimize / harden DSP and MIDI loops

[gregkh]

On Tue, Sep 12, 2017 at 09:17:38AM +0200, Takashi Iwai wrote:
> On Fri, 08 Sep 2017 19:47:32 +0200,
> Grygorii Tertychnyi (gtertych) wrote:
> > 
> > 
> > >> Hi Greg,
> > >>
> > >> Could you please apply it for 4.4-stable.
> > >> This fixes https://nvd.nist.gov/vuln/detail/CVE-2017-9985
> > >
> > > This vulnerability is just non-issue.  You can't get it working
> > > practically; it requires a modified hardware of the decade old ISA
> > > sound card, and yet the system has to load / set up the module
> > > beforehand.  We should withdraw it from CVE, IMO.
> > 
> > I think it is worth having it in 4.4, 4.9 and 4.12 also.
> 
> ... even though the code has never been tested on the real hardware?
> That doesn't sound good for stable kernels at all.  That's why I
> didn't put Cc to stable in the patch.

Oh, I didn't know that, want me to drop the patch from the stable queues
now?

thanks,

greg k-h

Re: [PATCH] ALSA: msnd: Optimize / harden DSP and MIDI loops

[Takashi Iwai]

On Tue, 12 Sep 2017 14:34:18 +0200,
gregkh@linuxfoundation.org wrote:
> 
> On Tue, Sep 12, 2017 at 09:17:38AM +0200, Takashi Iwai wrote:
> > On Fri, 08 Sep 2017 19:47:32 +0200,
> > Grygorii Tertychnyi (gtertych) wrote:
> > > 
> > > 
> > > >> Hi Greg,
> > > >>
> > > >> Could you please apply it for 4.4-stable.
> > > >> This fixes https://nvd.nist.gov/vuln/detail/CVE-2017-9985
> > > >
> > > > This vulnerability is just non-issue.  You can't get it working
> > > > practically; it requires a modified hardware of the decade old ISA
> > > > sound card, and yet the system has to load / set up the module
> > > > beforehand.  We should withdraw it from CVE, IMO.
> > > 
> > > I think it is worth having it in 4.4, 4.9 and 4.12 also.
> > 
> > ... even though the code has never been tested on the real hardware?
> > That doesn't sound good for stable kernels at all.  That's why I
> > didn't put Cc to stable in the patch.
> 
> Oh, I didn't know that, want me to drop the patch from the stable queues
> now?

Honestly, I don't mind.  The patch should work, and even if it
doesn't, it would be harmless as no one can see the breakage in
practice :)

It's just ridiculous that people urge such commit for stable kernels
even though they never tested / care the real cases but only look at
the CVE entry.


thanks,

Takashi

Re: [PATCH] ALSA: msnd: Optimize / harden DSP and MIDI loops

[Greg KH]

On Fri, Sep 08, 2017 at 06:57:57PM +0200, Takashi Iwai wrote:
> On Fri, 08 Sep 2017 18:06:25 +0200,
> grygorii tertychnyi wrote:
> > 
> > Hi Greg,
> > 
> > Could you please apply it for 4.4-stable.
> > This fixes https://nvd.nist.gov/vuln/detail/CVE-2017-9985
> 
> This vulnerability is just non-issue.  You can't get it working
> practically; it requires a modified hardware of the decade old ISA
> sound card, and yet the system has to load / set up the module
> beforehand.  We should withdraw it from CVE, IMO.

Hah, good luck trying to get a CVE withdrawn, people seem to love the
foolish things...

[PATCH] ALSA: msnd: Optimize / harden DSP and MIDI loops

[grygorii tertychnyi]

From: Takashi Iwai <tiwai@suse.de>

commit 20e2b791796bd68816fa115f12be5320de2b8021 upstream.

The ISA msnd drivers have loops fetching the ring-buffer head, tail
and size values inside the loops.  Such codes are inefficient and
fragile.

This patch optimizes it, and also adds the sanity check to avoid the
endless loops.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=196131
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=196133
Signed-off-by: Takashi Iwai <tiwai@suse.de>
Cc: xe-linux-external@cisco.com
Signed-off-by: grygorii tertychnyi <gtertych@cisco.com>
---
 sound/isa/msnd/msnd_midi.c     | 30 +++++++++++++++---------------
 sound/isa/msnd/msnd_pinnacle.c | 23 ++++++++++++-----------
 2 files changed, 27 insertions(+), 26 deletions(-)

diff --git a/sound/isa/msnd/msnd_midi.c b/sound/isa/msnd/msnd_midi.c
index ffc67fd80c23..58e59cd3c95c 100644
--- a/sound/isa/msnd/msnd_midi.c
+++ b/sound/isa/msnd/msnd_midi.c
@@ -120,24 +120,24 @@ void snd_msndmidi_input_read(void *mpuv)
 	unsigned long flags;
 	struct snd_msndmidi *mpu = mpuv;
 	void *pwMIDQData = mpu->dev->mappedbase + MIDQ_DATA_BUFF;
+	u16 head, tail, size;
 
 	spin_lock_irqsave(&mpu->input_lock, flags);
-	while (readw(mpu->dev->MIDQ + JQS_wTail) !=
-	       readw(mpu->dev->MIDQ + JQS_wHead)) {
-		u16 wTmp, val;
-		val = readw(pwMIDQData + 2 * readw(mpu->dev->MIDQ + JQS_wHead));
-
-			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;
-		if (wTmp > readw(mpu->dev->MIDQ + JQS_wSize))
-			writew(0,  mpu->dev->MIDQ + JQS_wHead);
-		else
-			writew(wTmp,  mpu->dev->MIDQ + JQS_wHead);
+	head = readw(mpu->dev->MIDQ + JQS_wHead);
+	tail = readw(mpu->dev->MIDQ + JQS_wTail);
+	size = readw(mpu->dev->MIDQ + JQS_wSize);
+	if (head > size || tail > size)
+		goto out;
+	while (head != tail) {
+		unsigned char val = readw(pwMIDQData + 2 * head);
+
+		if (test_bit(MSNDMIDI_MODE_BIT_INPUT_TRIGGER, &mpu->mode))
+			snd_rawmidi_receive(mpu->substream_input, &val, 1);
+		if (++head > size)
+			head = 0;
+		writew(head, mpu->dev->MIDQ + JQS_wHead);
 	}
+ out:
 	spin_unlock_irqrestore(&mpu->input_lock, flags);
 }
 EXPORT_SYMBOL(snd_msndmidi_input_read);
diff --git a/sound/isa/msnd/msnd_pinnacle.c b/sound/isa/msnd/msnd_pinnacle.c
index 4c072666115d..a31ea6c22d19 100644
--- a/sound/isa/msnd/msnd_pinnacle.c
+++ b/sound/isa/msnd/msnd_pinnacle.c
@@ -170,23 +170,24 @@ static irqreturn_t snd_msnd_interrupt(int irq, void *dev_id)
 {
 	struct snd_msnd *chip = dev_id;
 	void *pwDSPQData = chip->mappedbase + DSPQ_DATA_BUFF;
+	u16 head, tail, size;
 
 	/* Send ack to DSP */
 	/* inb(chip->io + HP_RXL); */
 
 	/* Evaluate queued DSP messages */
-	while (readw(chip->DSPQ + JQS_wTail) != readw(chip->DSPQ + JQS_wHead)) {
-		u16 wTmp;
-
-		snd_msnd_eval_dsp_msg(chip,
-			readw(pwDSPQData + 2 * readw(chip->DSPQ + JQS_wHead)));
-
-		wTmp = readw(chip->DSPQ + JQS_wHead) + 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);
+	size = readw(chip->DSPQ + JQS_wSize);
+	if (head > size || tail > size)
+		goto out;
+	while (head != tail) {
+		snd_msnd_eval_dsp_msg(chip, readw(pwDSPQData + 2 * head));
+		if (++head > size)
+			head = 0;
+		writew(head, chip->DSPQ + JQS_wHead);
 	}
+ out:
 	/* Send ack to DSP */
 	inb(chip->io + HP_RXL);
 	return IRQ_HANDLED;
-- 
2.10.3.dirty

[PATCH] ALSA: msnd: Optimize / harden DSP and MIDI loops

[Takashi Iwai]

The ISA msnd drivers have loops fetching the ring-buffer head, tail
and size values inside the loops.  Such codes are inefficient and
fragile.

This patch optimizes it, and also adds the sanity check to avoid the
endless loops.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=196131
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=196133
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/isa/msnd/msnd_midi.c     | 30 +++++++++++++++---------------
 sound/isa/msnd/msnd_pinnacle.c | 23 ++++++++++++-----------
 2 files changed, 27 insertions(+), 26 deletions(-)

diff --git a/sound/isa/msnd/msnd_midi.c b/sound/isa/msnd/msnd_midi.c
index 912b5a9ccbab..013d8d1170fe 100644
--- a/sound/isa/msnd/msnd_midi.c
+++ b/sound/isa/msnd/msnd_midi.c
@@ -120,24 +120,24 @@ void snd_msndmidi_input_read(void *mpuv)
 	unsigned long flags;
 	struct snd_msndmidi *mpu = mpuv;
 	void *pwMIDQData = mpu->dev->mappedbase + MIDQ_DATA_BUFF;
+	u16 head, tail, size;
 
 	spin_lock_irqsave(&mpu->input_lock, flags);
-	while (readw(mpu->dev->MIDQ + JQS_wTail) !=
-	       readw(mpu->dev->MIDQ + JQS_wHead)) {
-		u16 wTmp, val;
-		val = readw(pwMIDQData + 2 * readw(mpu->dev->MIDQ + JQS_wHead));
-
-			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;
-		if (wTmp > readw(mpu->dev->MIDQ + JQS_wSize))
-			writew(0,  mpu->dev->MIDQ + JQS_wHead);
-		else
-			writew(wTmp,  mpu->dev->MIDQ + JQS_wHead);
+	head = readw(mpu->dev->MIDQ + JQS_wHead);
+	tail = readw(mpu->dev->MIDQ + JQS_wTail);
+	size = readw(mpu->dev->MIDQ + JQS_wSize);
+	if (head > size || tail > size)
+		goto out;
+	while (head != tail) {
+		unsigned char val = readw(pwMIDQData + 2 * head);
+
+		if (test_bit(MSNDMIDI_MODE_BIT_INPUT_TRIGGER, &mpu->mode))
+			snd_rawmidi_receive(mpu->substream_input, &val, 1);
+		if (++head > size)
+			head = 0;
+		writew(head, mpu->dev->MIDQ + JQS_wHead);
 	}
+ out:
 	spin_unlock_irqrestore(&mpu->input_lock, flags);
 }
 EXPORT_SYMBOL(snd_msndmidi_input_read);
diff --git a/sound/isa/msnd/msnd_pinnacle.c b/sound/isa/msnd/msnd_pinnacle.c
index ad4897337df5..fc4fb1904aef 100644
--- a/sound/isa/msnd/msnd_pinnacle.c
+++ b/sound/isa/msnd/msnd_pinnacle.c
@@ -170,23 +170,24 @@ static irqreturn_t snd_msnd_interrupt(int irq, void *dev_id)
 {
 	struct snd_msnd *chip = dev_id;
 	void *pwDSPQData = chip->mappedbase + DSPQ_DATA_BUFF;
+	u16 head, tail, size;
 
 	/* Send ack to DSP */
 	/* inb(chip->io + HP_RXL); */
 
 	/* Evaluate queued DSP messages */
-	while (readw(chip->DSPQ + JQS_wTail) != readw(chip->DSPQ + JQS_wHead)) {
-		u16 wTmp;
-
-		snd_msnd_eval_dsp_msg(chip,
-			readw(pwDSPQData + 2 * readw(chip->DSPQ + JQS_wHead)));
-
-		wTmp = readw(chip->DSPQ + JQS_wHead) + 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);
+	size = readw(chip->DSPQ + JQS_wSize);
+	if (head > size || tail > size)
+		goto out;
+	while (head != tail) {
+		snd_msnd_eval_dsp_msg(chip, readw(pwDSPQData + 2 * head));
+		if (++head > size)
+			head = 0;
+		writew(head, chip->DSPQ + JQS_wHead);
 	}
+ out:
 	/* Send ack to DSP */
 	inb(chip->io + HP_RXL);
 	return IRQ_HANDLED;
-- 
2.13.2