[patch] ALSA: seq_midi_emul: small array underflow

[patch] ALSA: seq_midi_emul: small array underflow

[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.

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 */

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

[Clemens Ladisch]

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

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

[Dan Carpenter]

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

[patch v2] ALSA: opl3: small array underflow

[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;
 

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

[Clemens Ladisch]

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

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

[Dan Carpenter]

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