ALSA: seq: potential out of bounds in do_control()
Subsystems (2)
PATCH 1/1 ALSA: seq: potential out of bounds in do_control()
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()---
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.
sound/core/seq/seq_midi_emul.c +3 -0
@@ -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 */
Quoted 12 lines · 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()
Quoted 5 lines
Signed-off-by Dan Carpenter <[email protected]>---
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.
sound/core/seq/seq_midi_emul.c do_control(struct snd_midi_op *ops, void *drv, struct snd_midi_channel_set *chse · line 273
Quoted 10 lines
sound/core/seq/seq_midi_emul.c +2
-0@@ -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
At Wed, 11 Feb 2015 16:35:50 +0100,
Quoted 14 lines · Clemens Ladisch
Quoted 12 lines · 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()
Quoted 8 lines
Quoted 5 lines
Signed-off-by Dan Carpenter <[email protected]>---
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.
Quoted 8 lines
Quoted 8 lines
sound/core/seq/seq_midi_emul.c +0
-0@@ -269,6 +269,9 @@ do_control(struct snd_midi_op *ops, void *drv, struct snd_midi_channel_set *chse {int i;
Quoted 4 lines
sound/core/seq/seq_midi_emul.c do_control(struct snd_midi_op *ops, void *drv, struct snd_midi_channel_set *chse · line 273
Quoted 2 lines
sound/core/seq/seq_midi_emul.c +2
-0+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
At Wed, 11 Feb 2015 18:10:54 +0300,
Quoted 13 lines · 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()
Quoted 5 lines
Signed-off-by Dan Carpenter <[email protected]>---
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
Quoted 16 lines
sound/core/seq/seq_midi_emul.c +3
-0@@ -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 */