[patch] ALSA: seq: potential out of bounds in do_control()

[patch] ALSA: seq: potential out of bounds in do_control()

[Dan Carpenter]

Smatch complains that "control" is user specifigy and needs to be
capped.  The call tree to understand this warning is quite long.

snd_seq_write()  <-- get the event from the user
  snd_seq_client_enqueue_event()
    snd_seq_deliver_event()
      deliver_to_subscribers()
        snd_seq_deliver_single_event()
          snd_opl3_oss_event_input()
            snd_midi_process_event()
              do_control()

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I have spent some time reviewing this code, but I may have missed
something where we verify that control is in bounds.  I'm not very
familiar with this code and the call tree is fairly long.

diff --git a/sound/core/seq/seq_midi_emul.c b/sound/core/seq/seq_midi_emul.c
index 9b6470c..7ba9373 100644
--- a/sound/core/seq/seq_midi_emul.c
+++ b/sound/core/seq/seq_midi_emul.c
@@ -269,6 +269,9 @@ do_control(struct snd_midi_op *ops, void *drv, struct snd_midi_channel_set *chse
 {
 	int  i;
 
+	if (control >= ARRAY_SIZE(chan->control))
+		return;
+
 	/* Switches */
 	if ((control >=64 && control <=69) || (control >= 80 && control <= 83)) {
 		/* These are all switches; either off or on so set to 0 or 127 */

[patch] ALSA: seq: potential out of bounds in do_control()

[Dan Carpenter]

Smatch complains that "control" is user specifigy and needs to be
capped.  The call tree to understand this warning is quite long.

snd_seq_write()  <-- get the event from the user
  snd_seq_client_enqueue_event()
    snd_seq_deliver_event()
      deliver_to_subscribers()
        snd_seq_deliver_single_event()
          snd_opl3_oss_event_input()
            snd_midi_process_event()
              do_control()

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I have spent some time reviewing this code, but I may have missed
something where we verify that control is in bounds.  I'm not very
familiar with this code and the call tree is fairly long.

diff --git a/sound/core/seq/seq_midi_emul.c b/sound/core/seq/seq_midi_emul.c
index 9b6470c..7ba9373 100644
--- a/sound/core/seq/seq_midi_emul.c
+++ b/sound/core/seq/seq_midi_emul.c
@@ -269,6 +269,9 @@ do_control(struct snd_midi_op *ops, void *drv, struct snd_midi_channel_set *chse
 {
 	int  i;
 
+	if (control >= ARRAY_SIZE(chan->control))
+		return;
+
 	/* Switches */
 	if ((control >d && control <i) || (control >= 80 && control <= 83)) {
 		/* These are all switches; either off or on so set to 0 or 127 */

Re: [patch] ALSA: seq: potential out of bounds in do_control()

[Clemens Ladisch]

Dan Carpenter wrote:
> Smatch complains that "control" is user specifigy and needs to be
> capped.  The call tree to understand this warning is quite long.
>
> snd_seq_write()  <-- get the event from the user
>   snd_seq_client_enqueue_event()
>     snd_seq_deliver_event()
>       deliver_to_subscribers()
>         snd_seq_deliver_single_event()
>           snd_opl3_oss_event_input()
>             snd_midi_process_event()
>               do_control()
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> I have spent some time reviewing this code, but I may have missed
> something where we verify that control is in bounds.  I'm not very
> familiar with this code and the call tree is fairly long.

BTW, there are other ways to deliver events.  In any case, events are
pretty much opaque blobs, and most fields are not checked.

> diff --git a/sound/core/seq/seq_midi_emul.c b/sound/core/seq/seq_midi_emul.c
> index 9b6470c..7ba9373 100644
> --- a/sound/core/seq/seq_midi_emul.c
> +++ b/sound/core/seq/seq_midi_emul.c
> @@ -269,6 +269,9 @@ do_control(struct snd_midi_op *ops, void *drv, struct snd_midi_channel_set *chse
>  {
>  	int  i;
>
> +	if (control >= ARRAY_SIZE(chan->control))
> +		return;

Is this correct for an unsigned int converted to an int?


Regards,
Clemens

Re: [alsa-devel] [patch] ALSA: seq: potential out of bounds in do_control()

[Clemens Ladisch]

Dan Carpenter wrote:
> Smatch complains that "control" is user specifigy and needs to be
> capped.  The call tree to understand this warning is quite long.
>
> snd_seq_write()  <-- get the event from the user
>   snd_seq_client_enqueue_event()
>     snd_seq_deliver_event()
>       deliver_to_subscribers()
>         snd_seq_deliver_single_event()
>           snd_opl3_oss_event_input()
>             snd_midi_process_event()
>               do_control()
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> I have spent some time reviewing this code, but I may have missed
> something where we verify that control is in bounds.  I'm not very
> familiar with this code and the call tree is fairly long.

BTW, there are other ways to deliver events.  In any case, events are
pretty much opaque blobs, and most fields are not checked.

> diff --git a/sound/core/seq/seq_midi_emul.c b/sound/core/seq/seq_midi_emul.c
> index 9b6470c..7ba9373 100644
> --- a/sound/core/seq/seq_midi_emul.c
> +++ b/sound/core/seq/seq_midi_emul.c
> @@ -269,6 +269,9 @@ do_control(struct snd_midi_op *ops, void *drv, struct snd_midi_channel_set *chse
>  {
>  	int  i;
>
> +	if (control >= ARRAY_SIZE(chan->control))
> +		return;

Is this correct for an unsigned int converted to an int?


Regards,
Clemens

Re: [alsa-devel] [patch] ALSA: seq: potential out of bounds in do_control()

[Takashi Iwai]

At Wed, 11 Feb 2015 16:35:50 +0100,
Clemens Ladisch wrote:
> 
> Dan Carpenter wrote:
> > Smatch complains that "control" is user specifigy and needs to be
> > capped.  The call tree to understand this warning is quite long.
> >
> > snd_seq_write()  <-- get the event from the user
> >   snd_seq_client_enqueue_event()
> >     snd_seq_deliver_event()
> >       deliver_to_subscribers()
> >         snd_seq_deliver_single_event()
> >           snd_opl3_oss_event_input()
> >             snd_midi_process_event()
> >               do_control()
> >
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > I have spent some time reviewing this code, but I may have missed
> > something where we verify that control is in bounds.  I'm not very
> > familiar with this code and the call tree is fairly long.
> 
> BTW, there are other ways to deliver events.  In any case, events are
> pretty much opaque blobs, and most fields are not checked.

Right.


> > diff --git a/sound/core/seq/seq_midi_emul.c b/sound/core/seq/seq_midi_emul.c
> > index 9b6470c..7ba9373 100644
> > --- a/sound/core/seq/seq_midi_emul.c
> > +++ b/sound/core/seq/seq_midi_emul.c
> > @@ -269,6 +269,9 @@ do_control(struct snd_midi_op *ops, void *drv, struct snd_midi_channel_set *chse
> >  {
> >  	int  i;
> >
> > +	if (control >= ARRAY_SIZE(chan->control))
> > +		return;
> 
> Is this correct for an unsigned int converted to an int?

That should be OK.  ARRAY_SIZE() is size_t (i.e. unsigned long), so
the comparison is done in unsigned.  When control is negative, it's
beyond ARRAY_SIZE() and handled as an error here, too.


Takashi

Re: [alsa-devel] [patch] ALSA: seq: potential out of bounds in do_control()

[Takashi Iwai]

At Wed, 11 Feb 2015 16:35:50 +0100,
Clemens Ladisch wrote:
> 
> Dan Carpenter wrote:
> > Smatch complains that "control" is user specifigy and needs to be
> > capped.  The call tree to understand this warning is quite long.
> >
> > snd_seq_write()  <-- get the event from the user
> >   snd_seq_client_enqueue_event()
> >     snd_seq_deliver_event()
> >       deliver_to_subscribers()
> >         snd_seq_deliver_single_event()
> >           snd_opl3_oss_event_input()
> >             snd_midi_process_event()
> >               do_control()
> >
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > I have spent some time reviewing this code, but I may have missed
> > something where we verify that control is in bounds.  I'm not very
> > familiar with this code and the call tree is fairly long.
> 
> BTW, there are other ways to deliver events.  In any case, events are
> pretty much opaque blobs, and most fields are not checked.

Right.


> > diff --git a/sound/core/seq/seq_midi_emul.c b/sound/core/seq/seq_midi_emul.c
> > index 9b6470c..7ba9373 100644
> > --- a/sound/core/seq/seq_midi_emul.c
> > +++ b/sound/core/seq/seq_midi_emul.c
> > @@ -269,6 +269,9 @@ do_control(struct snd_midi_op *ops, void *drv, struct snd_midi_channel_set *chse
> >  {
> >  	int  i;
> >
> > +	if (control >= ARRAY_SIZE(chan->control))
> > +		return;
> 
> Is this correct for an unsigned int converted to an int?

That should be OK.  ARRAY_SIZE() is size_t (i.e. unsigned long), so
the comparison is done in unsigned.  When control is negative, it's
beyond ARRAY_SIZE() and handled as an error here, too.


Takashi

Re: [patch] ALSA: seq: potential out of bounds in do_control()

[Takashi Iwai]

At Wed, 11 Feb 2015 18:10:54 +0300,
Dan Carpenter wrote:
> 
> Smatch complains that "control" is user specifigy and needs to be
> capped.  The call tree to understand this warning is quite long.
> 
> snd_seq_write()  <-- get the event from the user
>   snd_seq_client_enqueue_event()
>     snd_seq_deliver_event()
>       deliver_to_subscribers()
>         snd_seq_deliver_single_event()
>           snd_opl3_oss_event_input()
>             snd_midi_process_event()
>               do_control()
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> I have spent some time reviewing this code, but I may have missed
> something where we verify that control is in bounds.  I'm not very
> familiar with this code and the call tree is fairly long.

I applied locally now and will merge it later once when the current
pull request is resolved.


thanks,

Takashi

> 
> diff --git a/sound/core/seq/seq_midi_emul.c b/sound/core/seq/seq_midi_emul.c
> index 9b6470c..7ba9373 100644
> --- a/sound/core/seq/seq_midi_emul.c
> +++ b/sound/core/seq/seq_midi_emul.c
> @@ -269,6 +269,9 @@ do_control(struct snd_midi_op *ops, void *drv, struct snd_midi_channel_set *chse
>  {
>  	int  i;
>  
> +	if (control >= ARRAY_SIZE(chan->control))
> +		return;
> +
>  	/* Switches */
>  	if ((control >=64 && control <=69) || (control >= 80 && control <= 83)) {
>  		/* These are all switches; either off or on so set to 0 or 127 */
> 

Re: [patch] ALSA: seq: potential out of bounds in do_control()

[Takashi Iwai]

At Wed, 11 Feb 2015 18:10:54 +0300,
Dan Carpenter wrote:
> 
> Smatch complains that "control" is user specifigy and needs to be
> capped.  The call tree to understand this warning is quite long.
> 
> snd_seq_write()  <-- get the event from the user
>   snd_seq_client_enqueue_event()
>     snd_seq_deliver_event()
>       deliver_to_subscribers()
>         snd_seq_deliver_single_event()
>           snd_opl3_oss_event_input()
>             snd_midi_process_event()
>               do_control()
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> I have spent some time reviewing this code, but I may have missed
> something where we verify that control is in bounds.  I'm not very
> familiar with this code and the call tree is fairly long.

I applied locally now and will merge it later once when the current
pull request is resolved.


thanks,

Takashi

> 
> diff --git a/sound/core/seq/seq_midi_emul.c b/sound/core/seq/seq_midi_emul.c
> index 9b6470c..7ba9373 100644
> --- a/sound/core/seq/seq_midi_emul.c
> +++ b/sound/core/seq/seq_midi_emul.c
> @@ -269,6 +269,9 @@ do_control(struct snd_midi_op *ops, void *drv, struct snd_midi_channel_set *chse
>  {
>  	int  i;
>  
> +	if (control >= ARRAY_SIZE(chan->control))
> +		return;
> +
>  	/* Switches */
>  	if ((control >d && control <i) || (control >= 80 && control <= 83)) {
>  		/* These are all switches; either off or on so set to 0 or 127 */
>