[alsa-devel] [patch] sound/oss: potential integer overflow
We don't want "pre_event_timeout" to be negative because that would result in a stack traces in dmesg when we schedule a negative timeout. In the original code "HZ * val" could overflow so I just moved the check for negative below the multiply.
Also the code in snd_seq_oss_ioctl() deliberately set the timeout to -1 which is wrong.
Signed-off-by: Dan Carpenter error27@gmail.com
diff --git a/sound/oss/sequencer.c b/sound/oss/sequencer.c index e85789e..de61295 100644 --- a/sound/oss/sequencer.c +++ b/sound/oss/sequencer.c @@ -1507,9 +1507,9 @@ int sequencer_ioctl(int dev, struct file *file, unsigned int cmd, void __user *a case SNDCTL_MIDI_PRETIME: if (get_user(val, p)) return -EFAULT; + val = (HZ * val) / 10; if (val < 0) val = 0; - val = (HZ * val) / 10; pre_event_timeout = val; break;
diff --git a/sound/oss/midibuf.c b/sound/oss/midibuf.c index 782b3b8..73acacd 100644 --- a/sound/oss/midibuf.c +++ b/sound/oss/midibuf.c @@ -380,9 +380,9 @@ int MIDIbuf_ioctl(int dev, struct file *file, case SNDCTL_MIDI_PRETIME: if (get_user(val, (int __user *)arg)) return -EFAULT; + val = (HZ * val) / 10; if (val < 0) val = 0; - val = (HZ * val) / 10; parms[dev].prech_timeout = val; return put_user(val, (int __user *)arg); diff --git a/sound/core/seq/oss/seq_oss_ioctl.c b/sound/core/seq/oss/seq_oss_ioctl.c index 5ac701c..ae2a39f 100644 --- a/sound/core/seq/oss/seq_oss_ioctl.c +++ b/sound/core/seq/oss/seq_oss_ioctl.c @@ -191,10 +191,9 @@ snd_seq_oss_ioctl(struct seq_oss_devinfo *dp, unsigned int cmd, unsigned long ca return 0; if (get_user(val, p)) return -EFAULT; - if (val <= 0) - val = -1; - else - val = (HZ * val) / 10; + val = (HZ * val) / 10; + if (val < 0) + val = 0; dp->readq->pre_event_timeout = val; return put_user(val, p) ? -EFAULT : 0;
At Wed, 8 Sep 2010 09:26:32 +0200, Dan Carpenter wrote:
We don't want "pre_event_timeout" to be negative because that would result in a stack traces in dmesg when we schedule a negative timeout. In the original code "HZ * val" could overflow so I just moved the check for negative below the multiply.
This would bring another side-effect. When a value like 0x80001234 is passed, this would result in a positive value in turn. We need additional check like below.
thanks,
Takashi
--- diff --git a/sound/core/seq/oss/seq_oss_ioctl.c b/sound/core/seq/oss/seq_oss_ioctl.c index 5ac701c..b2e0789 100644 --- a/sound/core/seq/oss/seq_oss_ioctl.c +++ b/sound/core/seq/oss/seq_oss_ioctl.c @@ -191,10 +191,13 @@ snd_seq_oss_ioctl(struct seq_oss_devinfo *dp, unsigned int cmd, unsigned long ca return 0; if (get_user(val, p)) return -EFAULT; - if (val <= 0) - val = -1; - else + if (val < 0) + val = 0; + else { val = (HZ * val) / 10; + if (val < 0) /* check overflow */ + val = 0; + } dp->readq->pre_event_timeout = val; return put_user(val, p) ? -EFAULT : 0;
diff --git a/sound/oss/midibuf.c b/sound/oss/midibuf.c index 782b3b8..b8da210 100644 --- a/sound/oss/midibuf.c +++ b/sound/oss/midibuf.c @@ -382,7 +382,11 @@ int MIDIbuf_ioctl(int dev, struct file *file, return -EFAULT; if (val < 0) val = 0; - val = (HZ * val) / 10; + else { + val = (HZ * val) / 10; + if (val < 0) /* check overflow */ + val = 0; + } parms[dev].prech_timeout = val; return put_user(val, (int __user *)arg); diff --git a/sound/oss/sequencer.c b/sound/oss/sequencer.c index e85789e..f579210 100644 --- a/sound/oss/sequencer.c +++ b/sound/oss/sequencer.c @@ -1509,7 +1509,11 @@ int sequencer_ioctl(int dev, struct file *file, unsigned int cmd, void __user *a return -EFAULT; if (val < 0) val = 0; - val = (HZ * val) / 10; + else { + val = (HZ * val) / 10; + if (val < 0) /* check overflow */ + val = 0; + } pre_event_timeout = val; break;
Takashi Iwai schrieb:
At Wed, 8 Sep 2010 09:26:32 +0200, Dan Carpenter wrote:
We don't want "pre_event_timeout" to be negative because that would result in a stack traces in dmesg when we schedule a negative timeout. In the original code "HZ * val" could overflow so I just moved the check for negative below the multiply.
This would bring another side-effect. When a value like 0x80001234 is passed, this would result in a positive value in turn. We need additional check like below.
thanks,
Takashi
diff --git a/sound/core/seq/oss/seq_oss_ioctl.c b/sound/core/seq/oss/seq_oss_ioctl.c index 5ac701c..b2e0789 100644 --- a/sound/core/seq/oss/seq_oss_ioctl.c +++ b/sound/core/seq/oss/seq_oss_ioctl.c @@ -191,10 +191,13 @@ snd_seq_oss_ioctl(struct seq_oss_devinfo *dp, unsigned int cmd, unsigned long ca return 0; if (get_user(val, p)) return -EFAULT;
if (val <= 0)val = -1;else
if (val < 0)val = 0;else { val = (HZ * val) / 10;if (val < 0) /* check overflow */val = 0;}diff --git a/sound/oss/midibuf.c b/sound/oss/midibuf.c index 782b3b8..b8da210 100644 --- a/sound/oss/midibuf.c +++ b/sound/oss/midibuf.c @@ -382,7 +382,11 @@ int MIDIbuf_ioctl(int dev, struct file *file, return -EFAULT; if (val < 0) val = 0;
val = (HZ * val) / 10;
else {val = (HZ * val) / 10;if (val < 0) /* check overflow */val = 0;} parms[dev].prech_timeout = val; return put_user(val, (int __user *)arg);diff --git a/sound/oss/sequencer.c b/sound/oss/sequencer.c index e85789e..f579210 100644 --- a/sound/oss/sequencer.c +++ b/sound/oss/sequencer.c @@ -1509,7 +1509,11 @@ int sequencer_ioctl(int dev, struct file *file, unsigned int cmd, void __user *a return -EFAULT; if (val < 0) val = 0;
val = (HZ * val) / 10;
else {val = (HZ * val) / 10;if (val < 0) /* check overflow */val = 0;} pre_event_timeout = val; break;--
Perhaps a recalc_val() is here better ? That would avoid duplication and make sure that they behave equal. (see: > - if (val <= 0) > - val = -1; )
int recalc_val(int hz, int val) { if (val < 0) return 0 val = (hz * val) / 10; if (val < 0) return 0
return val; }
just my 2 cents, not a tested patch. re, wh
participants (3)
-
Dan Carpenter -
Takashi Iwai -
walter harms