* [patch] ALSA: seq: potential out of bounds in do_control()
@ 2015-02-11 15:10 ` Dan Carpenter
0 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2015-02-11 15:10 UTC (permalink / raw)
To: Jaroslav Kysela; +Cc: Takashi Iwai, alsa-devel, kernel-janitors
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 */
^ permalink raw reply related [flat|nested] 8+ messages in thread* [patch] ALSA: seq: potential out of bounds in do_control()
@ 2015-02-11 15:10 ` Dan Carpenter
0 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2015-02-11 15:10 UTC (permalink / raw)
To: Jaroslav Kysela; +Cc: Takashi Iwai, alsa-devel, kernel-janitors
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 */
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [patch] ALSA: seq: potential out of bounds in do_control()
2015-02-11 15:10 ` Dan Carpenter
@ 2015-02-11 15:35 ` Clemens Ladisch
-1 siblings, 0 replies; 8+ messages in thread
From: Clemens Ladisch @ 2015-02-11 15:35 UTC (permalink / raw)
To: Dan Carpenter, Jaroslav Kysela; +Cc: Takashi Iwai, alsa-devel, kernel-janitors
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [alsa-devel] [patch] ALSA: seq: potential out of bounds in do_control()
@ 2015-02-11 15:35 ` Clemens Ladisch
0 siblings, 0 replies; 8+ messages in thread
From: Clemens Ladisch @ 2015-02-11 15:35 UTC (permalink / raw)
To: Dan Carpenter, Jaroslav Kysela; +Cc: Takashi Iwai, alsa-devel, kernel-janitors
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [alsa-devel] [patch] ALSA: seq: potential out of bounds in do_control()
2015-02-11 15:35 ` [alsa-devel] " Clemens Ladisch
@ 2015-02-11 15:46 ` Takashi Iwai
-1 siblings, 0 replies; 8+ messages in thread
From: Takashi Iwai @ 2015-02-11 15:46 UTC (permalink / raw)
To: Clemens Ladisch
Cc: Dan Carpenter, Jaroslav Kysela, alsa-devel, kernel-janitors
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [alsa-devel] [patch] ALSA: seq: potential out of bounds in do_control()
@ 2015-02-11 15:46 ` Takashi Iwai
0 siblings, 0 replies; 8+ messages in thread
From: Takashi Iwai @ 2015-02-11 15:46 UTC (permalink / raw)
To: Clemens Ladisch
Cc: Dan Carpenter, Jaroslav Kysela, alsa-devel, kernel-janitors
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch] ALSA: seq: potential out of bounds in do_control()
2015-02-11 15:10 ` Dan Carpenter
@ 2015-02-11 15:50 ` Takashi Iwai
-1 siblings, 0 replies; 8+ messages in thread
From: Takashi Iwai @ 2015-02-11 15:50 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Jaroslav Kysela, alsa-devel, kernel-janitors
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 */
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [patch] ALSA: seq: potential out of bounds in do_control()
@ 2015-02-11 15:50 ` Takashi Iwai
0 siblings, 0 replies; 8+ messages in thread
From: Takashi Iwai @ 2015-02-11 15:50 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Jaroslav Kysela, alsa-devel, kernel-janitors
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 */
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-02-11 15:50 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-02-11 15:10 [patch] ALSA: seq: potential out of bounds in do_control() Dan Carpenter
2015-02-11 15:10 ` Dan Carpenter
2015-02-11 15:35 ` Clemens Ladisch
2015-02-11 15:35 ` [alsa-devel] " Clemens Ladisch
2015-02-11 15:46 ` Takashi Iwai
2015-02-11 15:46 ` Takashi Iwai
2015-02-11 15:50 ` Takashi Iwai
2015-02-11 15:50 ` Takashi Iwai
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.