[alsa-devel] [patch] 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 dan.carpenter@oracle.com
diff --git a/include/sound/seq_midi_emul.h b/include/sound/seq_midi_emul.h index 8139d8c..c02b840 100644 --- a/include/sound/seq_midi_emul.h +++ b/include/sound/seq_midi_emul.h @@ -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 */
Dan Carpenter wrote:
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.
- 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
On Tue, Mar 03, 2015 at 12:21:34PM +0100, Clemens Ladisch wrote:
Dan Carpenter wrote:
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.
- 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
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 dan.carpenter@oracle.com --- v2: The first patch just chan->midi_pitchbend unsigned but Clemens Ladisch pointed out that that breaks the API.
diff --git a/sound/drivers/opl3/opl3_midi.c b/sound/drivers/opl3/opl3_midi.c index f62780e..0cb91dc 100644 --- a/sound/drivers/opl3/opl3_midi.c +++ b/sound/drivers/opl3/opl3_midi.c @@ -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;
Dan Carpenter wrote:
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
On Tue, Mar 03, 2015 at 09:59:17PM +0100, Clemens Ladisch wrote:
Dan Carpenter wrote:
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
participants (2)
-
Clemens Ladisch -
Dan Carpenter