Re: [patch] sound/oss: potential integer overflow

[walter harms <wharms@bfs.de>]

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