* [patch] sound/oss: potential integer overflow
@ 2010-09-08 7:26 Dan Carpenter
2010-09-08 7:54 ` Takashi Iwai
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2010-09-08 7:26 UTC (permalink / raw)
To: Jaroslav Kysela; +Cc: Takashi Iwai, alsa-devel, kernel-janitors, Dan Carpenter
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;
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [patch] sound/oss: potential integer overflow
2010-09-08 7:26 [patch] sound/oss: potential integer overflow Dan Carpenter
@ 2010-09-08 7:54 ` Takashi Iwai
2010-09-08 9:50 ` walter harms
0 siblings, 1 reply; 3+ messages in thread
From: Takashi Iwai @ 2010-09-08 7:54 UTC (permalink / raw)
To: Dan Carpenter; +Cc: alsa-devel, kernel-janitors
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;
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [patch] sound/oss: potential integer overflow
2010-09-08 7:54 ` Takashi Iwai
@ 2010-09-08 9:50 ` walter harms
0 siblings, 0 replies; 3+ messages in thread
From: walter harms @ 2010-09-08 9:50 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel, kernel-janitors, Dan Carpenter
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;
> + }
> 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;
>
> --
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2010-09-08 9:50 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-09-08 7:26 [patch] sound/oss: potential integer overflow Dan Carpenter
2010-09-08 7:54 ` Takashi Iwai
2010-09-08 9:50 ` walter harms
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox