Skip to contentSkip to conversation

ALSA: seq_midi_emul: small array underflow

PATCH Superseded Dan Carpenter 2 participants 6 messages Mar 3, 2015

Subsystems (2)
Sound Kernel janitors

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 ALSA: seq_midi_emul: small array underflow

In snd_opl3_calc_pitch() then the limit is:

	if (pitchbend > 0x1FFF)
		pitchbend = 0x1FFF;

But it can underflow meaning that segment can be as low as SHORT_MIN / 0x1000 and we can read 6 elements before the start of the opl3_note_table[] array.

Signed-off-by Dan Carpenter <[email protected]>
include/sound/seq_midi_emul.h +1 -1
index 8139d8c..c02b840 100644
@@ -44,7 +44,7 @@ struct snd_midi_channel {
 	unsigned char midi_aftertouch;	/* Aftertouch (key pressure) */
 	unsigned char midi_pressure;	/* Channel pressure */
 	unsigned char midi_program;	/* Instrument number */
-	short midi_pitchbend;		/* Pitch bend amount */
+	unsigned short midi_pitchbend;	/* Pitch bend amount */
 
 	unsigned char control[128];	/* Current value of all controls */
 	unsigned char note[128];	/* Current status for all notes */
#1 lore raw

include/sound/seq_midi_emul.h struct snd_midi_channel { · line 47

Quoted 11 lines · Dan Carpenter

In snd_opl3_calc_pitch() then the limit is:

	if (pitchbend > 0x1FFF)
		pitchbend = 0x1FFF;

But it can underflow meaning that segment can be as low as SHORT_MIN / 0x1000 and we can read 6 elements before the start of the opl3_note_table[] array.

include/sound/seq_midi_emul.h +1 -1
-	short midi_pitchbend;		/* Pitch bend amount */
+	unsigned short midi_pitchbend;	/* Pitch bend amount */

Pitch bend is a signed 14-bit value. What is wrong is the missing check for the lower bound.

Regards,
Clemens

author
#2 lore raw
Quoted 10 lines · Clemens Ladisch
Quoted 8 lines · Dan Carpenter

In snd_opl3_calc_pitch() then the limit is:

	if (pitchbend > 0x1FFF)
		pitchbend = 0x1FFF;

But it can underflow meaning that segment can be as low as SHORT_MIN / 0x1000 and we can read 6 elements before the start of the opl3_note_table[] array.

Quoted 6 lines

include/sound/seq_midi_emul.h struct snd_midi_channel { · line 47

Quoted 2 lines
include/sound/seq_midi_emul.h +1 -1
-	short midi_pitchbend;		/* Pitch bend amount */
+	unsigned short midi_pitchbend;	/* Pitch bend amount */

Pitch bend is a signed 14-bit value. What is wrong is the missing check for the lower bound.

Thanks for the review. I will resend.

regards,
dan carpenter

author
#3 lore raw

PATCH v2 1/1 ALSA: opl3: small array underflow

In reply to Clemens Ladisch

We don't check for negatives so "pitchbend" can be SHRT_MIN here. It means that we can read up to 6 elements before the start of the opl3_note_table[] array.

There are several ways we could fix this. I have gone with what is maybe the lazier approach of just changing negative values to zero. Hopefully, people aren't passing negatives here anyway.

Signed-off-by Dan Carpenter <[email protected]>

---
v2: The first patch just chan->midi_pitchbend unsigned but Clemens Ladisch pointed out that that breaks the API.

sound/drivers/opl3/opl3_midi.c +2 -0
index f62780e..0cb91dc 100644
@@ -105,6 +105,8 @@ static void snd_opl3_calc_pitch(unsigned char *fnum, unsigned char *blocknum,
 		int pitchbend = chan->midi_pitchbend;
 		int segment;
 
+		if (pitchbend < 0)
+			pitchbend = 0;
 		if (pitchbend > 0x1FFF)
 			pitchbend = 0x1FFF;
#4 lore raw

Re: [alsa-devel] [patch v2] ALSA: opl3: small array underflow

Quoted 6 lines · Dan Carpenter

We don't check for negatives so "pitchbend" can be SHRT_MIN here. It means that we can read up to 6 elements before the start of the opl3_note_table[] array.

There are several ways we could fix this. I have gone with what is maybe the lazier approach of just changing negative values to zero.

The lower bound is not zero but -8192.

Regards,
Clemens

author
#5 lore raw

Re: [alsa-devel] [patch v2] ALSA: opl3: small array underflow

Quoted 9 lines · Clemens Ladisch
Quoted 6 lines · Dan Carpenter

We don't check for negatives so "pitchbend" can be SHRT_MIN here. It means that we can read up to 6 elements before the start of the opl3_note_table[] array.

There are several ways we could fix this. I have gone with what is maybe the lazier approach of just changing negative values to zero.

The lower bound is not zero but -8192.

Oh. I should have understood that from your previous email. Sorry for that. Third time is lucky, I suppose.

regards,
dan carpenter