[patch] ALSA: rawmidi: cleanup the get next midi device ioctl

[patch] ALSA: rawmidi: cleanup the get next midi device ioctl

[Dan Carpenter]

I'm doing an audit to find integer overflows and my static checker
complained that in the original code "device + 1" could overflow.  The
overflow is harmless, but it's still worth cleaning up.  The other thing
that I noticed is that if you pass in a device which is higher than
SNDRV_RAWMIDI_DEVICES then it doesn't return an error code but just
tells you that the next device is "device + 1".

I have rewritten it to just return -EINVAL if you pass in a bogus value
that's either too high or too low.  

Signed-off-by: Dan Carpenter <error27@gmail.com>

diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
index eb68326..f944180 100644
--- a/sound/core/rawmidi.c
+++ b/sound/core/rawmidi.c
@@ -829,8 +829,12 @@ static int snd_rawmidi_control_ioctl(struct snd_card *card,
 		
 		if (get_user(device, (int __user *)argp))
 			return -EFAULT;
+		if (device < 0)
+			return -EINVAL;
+		if (device > SNDRV_RAWMIDI_DEVICES)
+			return -EINVAL;
 		mutex_lock(&register_mutex);
-		device = device < 0 ? 0 : device + 1;
+		device++;
 		while (device < SNDRV_RAWMIDI_DEVICES) {
 			if (snd_rawmidi_search(card, device))
 				break;

[patch] ALSA: rawmidi: cleanup the get next midi device ioctl

[Dan Carpenter]

I'm doing an audit to find integer overflows and my static checker
complained that in the original code "device + 1" could overflow.  The
overflow is harmless, but it's still worth cleaning up.  The other thing
that I noticed is that if you pass in a device which is higher than
SNDRV_RAWMIDI_DEVICES then it doesn't return an error code but just
tells you that the next device is "device + 1".

I have rewritten it to just return -EINVAL if you pass in a bogus value
that's either too high or too low.  

Signed-off-by: Dan Carpenter <error27@gmail.com>

diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
index eb68326..f944180 100644
--- a/sound/core/rawmidi.c
+++ b/sound/core/rawmidi.c
@@ -829,8 +829,12 @@ static int snd_rawmidi_control_ioctl(struct snd_card *card,
 		
 		if (get_user(device, (int __user *)argp))
 			return -EFAULT;
+		if (device < 0)
+			return -EINVAL;
+		if (device > SNDRV_RAWMIDI_DEVICES)
+			return -EINVAL;
 		mutex_lock(&register_mutex);
-		device = device < 0 ? 0 : device + 1;
+		device++;
 		while (device < SNDRV_RAWMIDI_DEVICES) {
 			if (snd_rawmidi_search(card, device))
 				break;

Re: [patch] ALSA: rawmidi: cleanup the get next midi device ioctl

[Clemens Ladisch]

Dan Carpenter wrote:
> I'm doing an audit to find integer overflows and my static checker
> complained that in the original code "device + 1" could overflow.  The
> overflow is harmless, but it's still worth cleaning up.  The other thing
> that I noticed is that if you pass in a device which is higher than
> SNDRV_RAWMIDI_DEVICES then it doesn't return an error code but just
> tells you that the next device is "device + 1".
> 
> I have rewritten it to just return -EINVAL if you pass in a bogus value
> that's either too high or too low.

A negative value is a valid input.

> +		if (device > SNDRV_RAWMIDI_DEVICES)
> +			return -EINVAL;

		if (device >= SNDRV_RAWMIDI_DEVICES)


Regards,
Clemens

Re: [patch] ALSA: rawmidi: cleanup the get next midi device ioctl

[Clemens Ladisch]

Dan Carpenter wrote:
> I'm doing an audit to find integer overflows and my static checker
> complained that in the original code "device + 1" could overflow.  The
> overflow is harmless, but it's still worth cleaning up.  The other thing
> that I noticed is that if you pass in a device which is higher than
> SNDRV_RAWMIDI_DEVICES then it doesn't return an error code but just
> tells you that the next device is "device + 1".
> 
> I have rewritten it to just return -EINVAL if you pass in a bogus value
> that's either too high or too low.

A negative value is a valid input.

> +		if (device > SNDRV_RAWMIDI_DEVICES)
> +			return -EINVAL;

		if (device >= SNDRV_RAWMIDI_DEVICES)


Regards,
Clemens

[patch v2] ALSA: rawmidi: fix the get next midi device ioctl

[Dan Carpenter]

If we pass in a device which is higher than SNDRV_RAWMIDI_DEVICES then
this function just returns device + 1 which isn't helpful.  I've
modified it to return -EINVAL instead.

Also Smatch complains because the "device + 1" could be an integer
overflow.  It's harmless, but we may as well silence the warning.

Signed-off-by: Dan Carpenter <error27@gmail.com>
---
V2:  In the first version I made negative values return -EINVAL

diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
index eb68326..1633bac 100644
--- a/sound/core/rawmidi.c
+++ b/sound/core/rawmidi.c
@@ -829,6 +829,8 @@ static int snd_rawmidi_control_ioctl(struct snd_card *card,
 		
 		if (get_user(device, (int __user *)argp))
 			return -EFAULT;
+		if (device > SNDRV_RAWMIDI_DEVICES)
+			return -EINVAL;
 		mutex_lock(&register_mutex);
 		device = device < 0 ? 0 : device + 1;
 		while (device < SNDRV_RAWMIDI_DEVICES) {

[patch v2] ALSA: rawmidi: fix the get next midi device ioctl

[Dan Carpenter]

If we pass in a device which is higher than SNDRV_RAWMIDI_DEVICES then
this function just returns device + 1 which isn't helpful.  I've
modified it to return -EINVAL instead.

Also Smatch complains because the "device + 1" could be an integer
overflow.  It's harmless, but we may as well silence the warning.

Signed-off-by: Dan Carpenter <error27@gmail.com>
---
V2:  In the first version I made negative values return -EINVAL

diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
index eb68326..1633bac 100644
--- a/sound/core/rawmidi.c
+++ b/sound/core/rawmidi.c
@@ -829,6 +829,8 @@ static int snd_rawmidi_control_ioctl(struct snd_card *card,
 		
 		if (get_user(device, (int __user *)argp))
 			return -EFAULT;
+		if (device > SNDRV_RAWMIDI_DEVICES)
+			return -EINVAL;
 		mutex_lock(&register_mutex);
 		device = device < 0 ? 0 : device + 1;
 		while (device < SNDRV_RAWMIDI_DEVICES) {

Re: [patch v2] ALSA: rawmidi: fix the get next midi device ioctl

[Takashi Iwai]

At Wed, 8 Sep 2010 21:36:41 +0200,
Dan Carpenter wrote:
> 
> If we pass in a device which is higher than SNDRV_RAWMIDI_DEVICES then
> this function just returns device + 1 which isn't helpful.  I've
> modified it to return -EINVAL instead.
> 
> Also Smatch complains because the "device + 1" could be an integer
> overflow.  It's harmless, but we may as well silence the warning.
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> V2:  In the first version I made negative values return -EINVAL
> 
> diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
> index eb68326..1633bac 100644
> --- a/sound/core/rawmidi.c
> +++ b/sound/core/rawmidi.c
> @@ -829,6 +829,8 @@ static int snd_rawmidi_control_ioctl(struct snd_card *card,
>  		
>  		if (get_user(device, (int __user *)argp))
>  			return -EFAULT;
> +		if (device > SNDRV_RAWMIDI_DEVICES)
> +			return -EINVAL;

This should be "device >= SNDRV_RAWMIDI_DEVICES".


Takashi

>  		mutex_lock(&register_mutex);
>  		device = device < 0 ? 0 : device + 1;
>  		while (device < SNDRV_RAWMIDI_DEVICES) {
> 

Re: [patch v2] ALSA: rawmidi: fix the get next midi device ioctl

[Takashi Iwai]

At Wed, 8 Sep 2010 21:36:41 +0200,
Dan Carpenter wrote:
> 
> If we pass in a device which is higher than SNDRV_RAWMIDI_DEVICES then
> this function just returns device + 1 which isn't helpful.  I've
> modified it to return -EINVAL instead.
> 
> Also Smatch complains because the "device + 1" could be an integer
> overflow.  It's harmless, but we may as well silence the warning.
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> V2:  In the first version I made negative values return -EINVAL
> 
> diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
> index eb68326..1633bac 100644
> --- a/sound/core/rawmidi.c
> +++ b/sound/core/rawmidi.c
> @@ -829,6 +829,8 @@ static int snd_rawmidi_control_ioctl(struct snd_card *card,
>  		
>  		if (get_user(device, (int __user *)argp))
>  			return -EFAULT;
> +		if (device > SNDRV_RAWMIDI_DEVICES)
> +			return -EINVAL;

This should be "device >= SNDRV_RAWMIDI_DEVICES".


Takashi

>  		mutex_lock(&register_mutex);
>  		device = device < 0 ? 0 : device + 1;
>  		while (device < SNDRV_RAWMIDI_DEVICES) {
> 

Re: [patch v2] ALSA: rawmidi: fix the get next midi device ioctl

[Jaroslav Kysela]

On Wed, 8 Sep 2010, Takashi Iwai wrote:

> At Wed, 8 Sep 2010 21:36:41 +0200,
> Dan Carpenter wrote:
>>
>> If we pass in a device which is higher than SNDRV_RAWMIDI_DEVICES then
>> this function just returns device + 1 which isn't helpful.  I've
>> modified it to return -EINVAL instead.
>>
>> Also Smatch complains because the "device + 1" could be an integer
>> overflow.  It's harmless, but we may as well silence the warning.
>>
>> Signed-off-by: Dan Carpenter <error27@gmail.com>
>> ---
>> V2:  In the first version I made negative values return -EINVAL
>>
>> diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
>> index eb68326..1633bac 100644
>> --- a/sound/core/rawmidi.c
>> +++ b/sound/core/rawmidi.c
>> @@ -829,6 +829,8 @@ static int snd_rawmidi_control_ioctl(struct snd_card *card,
>>
>>  		if (get_user(device, (int __user *)argp))
>>  			return -EFAULT;
>> +		if (device > SNDRV_RAWMIDI_DEVICES)
>> +			return -EINVAL;
>
> This should be "device >= SNDRV_RAWMIDI_DEVICES".

Also note that this check changes a bit semantics. All other NEXT_DEVICE 
ioctls returns -1 if the value is beyond the last device (meaning no more 
devices were found). So the

                if (device = SNDRV_RAWMIDI_DEVICES)
                         device = -1;

check should be

                if (device >= SNDRV_RAWMIDI_DEVICES)
                         device = -1;

... resulting in one line patch.

 					Jaroslav


-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

Re: [patch v2] ALSA: rawmidi: fix the get next midi device ioctl

[Jaroslav Kysela]

On Wed, 8 Sep 2010, Takashi Iwai wrote:

> At Wed, 8 Sep 2010 21:36:41 +0200,
> Dan Carpenter wrote:
>>
>> If we pass in a device which is higher than SNDRV_RAWMIDI_DEVICES then
>> this function just returns device + 1 which isn't helpful.  I've
>> modified it to return -EINVAL instead.
>>
>> Also Smatch complains because the "device + 1" could be an integer
>> overflow.  It's harmless, but we may as well silence the warning.
>>
>> Signed-off-by: Dan Carpenter <error27@gmail.com>
>> ---
>> V2:  In the first version I made negative values return -EINVAL
>>
>> diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
>> index eb68326..1633bac 100644
>> --- a/sound/core/rawmidi.c
>> +++ b/sound/core/rawmidi.c
>> @@ -829,6 +829,8 @@ static int snd_rawmidi_control_ioctl(struct snd_card *card,
>>
>>  		if (get_user(device, (int __user *)argp))
>>  			return -EFAULT;
>> +		if (device > SNDRV_RAWMIDI_DEVICES)
>> +			return -EINVAL;
>
> This should be "device >= SNDRV_RAWMIDI_DEVICES".

Also note that this check changes a bit semantics. All other NEXT_DEVICE 
ioctls returns -1 if the value is beyond the last device (meaning no more 
devices were found). So the

                if (device == SNDRV_RAWMIDI_DEVICES)
                         device = -1;

check should be

                if (device >= SNDRV_RAWMIDI_DEVICES)
                         device = -1;

... resulting in one line patch.

 					Jaroslav


-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

[patch v3] ALSA: rawmidi: fix the get next midi device ioctl

[Dan Carpenter]

If we pass in a device which is higher than SNDRV_RAWMIDI_DEVICES then
the "next device" should be -1.  This function just returns device + 1.

But the main thing is that "device + 1" can lead to a (harmless) integer
overflow and that annoys static analysis tools.

Signed-off-by: Dan Carpenter <error27@gmail.com>
---
V2:  In the first version I made negative values return -EINVAL
V3:  We shouldn't return -EINVAL for numbers which are too large but
     just set the next device to -1.

diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
index eb68326..df67605 100644
--- a/sound/core/rawmidi.c
+++ b/sound/core/rawmidi.c
@@ -829,6 +829,8 @@ static int snd_rawmidi_control_ioctl(struct snd_card *card,
 		
 		if (get_user(device, (int __user *)argp))
 			return -EFAULT;
+		if (device > SNDRV_RAWMIDI_DEVICES) /* next device is -1 */
+			device = SNDRV_RAWMIDI_DEVICES;
 		mutex_lock(&register_mutex);
 		device = device < 0 ? 0 : device + 1;
 		while (device < SNDRV_RAWMIDI_DEVICES) {

[patch v3] ALSA: rawmidi: fix the get next midi device ioctl

[Dan Carpenter]

If we pass in a device which is higher than SNDRV_RAWMIDI_DEVICES then
the "next device" should be -1.  This function just returns device + 1.

But the main thing is that "device + 1" can lead to a (harmless) integer
overflow and that annoys static analysis tools.

Signed-off-by: Dan Carpenter <error27@gmail.com>
---
V2:  In the first version I made negative values return -EINVAL
V3:  We shouldn't return -EINVAL for numbers which are too large but
     just set the next device to -1.

diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
index eb68326..df67605 100644
--- a/sound/core/rawmidi.c
+++ b/sound/core/rawmidi.c
@@ -829,6 +829,8 @@ static int snd_rawmidi_control_ioctl(struct snd_card *card,
 		
 		if (get_user(device, (int __user *)argp))
 			return -EFAULT;
+		if (device > SNDRV_RAWMIDI_DEVICES) /* next device is -1 */
+			device = SNDRV_RAWMIDI_DEVICES;
 		mutex_lock(&register_mutex);
 		device = device < 0 ? 0 : device + 1;
 		while (device < SNDRV_RAWMIDI_DEVICES) {

Re: [patch v2] ALSA: rawmidi: fix the get next midi device ioctl

[Takashi Iwai]

At Wed, 8 Sep 2010 23:29:14 +0200 (CEST),
Jaroslav Kysela wrote:
> 
> On Wed, 8 Sep 2010, Takashi Iwai wrote:
> 
> > At Wed, 8 Sep 2010 21:36:41 +0200,
> > Dan Carpenter wrote:
> >>
> >> If we pass in a device which is higher than SNDRV_RAWMIDI_DEVICES then
> >> this function just returns device + 1 which isn't helpful.  I've
> >> modified it to return -EINVAL instead.
> >>
> >> Also Smatch complains because the "device + 1" could be an integer
> >> overflow.  It's harmless, but we may as well silence the warning.
> >>
> >> Signed-off-by: Dan Carpenter <error27@gmail.com>
> >> ---
> >> V2:  In the first version I made negative values return -EINVAL
> >>
> >> diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
> >> index eb68326..1633bac 100644
> >> --- a/sound/core/rawmidi.c
> >> +++ b/sound/core/rawmidi.c
> >> @@ -829,6 +829,8 @@ static int snd_rawmidi_control_ioctl(struct snd_card *card,
> >>
> >>  		if (get_user(device, (int __user *)argp))
> >>  			return -EFAULT;
> >> +		if (device > SNDRV_RAWMIDI_DEVICES)
> >> +			return -EINVAL;
> >
> > This should be "device >= SNDRV_RAWMIDI_DEVICES".
> 
> Also note that this check changes a bit semantics. All other NEXT_DEVICE 
> ioctls returns -1 if the value is beyond the last device (meaning no more 
> devices were found). So the
> 
>                 if (device = SNDRV_RAWMIDI_DEVICES)
>                          device = -1;
> 
> check should be
> 
>                 if (device >= SNDRV_RAWMIDI_DEVICES)
>                          device = -1;
> 
> ... resulting in one line patch.

But this doesn't work when you pass device = INT_MAX :)


Takashi

Re: [patch v2] ALSA: rawmidi: fix the get next midi device ioctl

[Takashi Iwai]

At Wed, 8 Sep 2010 23:29:14 +0200 (CEST),
Jaroslav Kysela wrote:
> 
> On Wed, 8 Sep 2010, Takashi Iwai wrote:
> 
> > At Wed, 8 Sep 2010 21:36:41 +0200,
> > Dan Carpenter wrote:
> >>
> >> If we pass in a device which is higher than SNDRV_RAWMIDI_DEVICES then
> >> this function just returns device + 1 which isn't helpful.  I've
> >> modified it to return -EINVAL instead.
> >>
> >> Also Smatch complains because the "device + 1" could be an integer
> >> overflow.  It's harmless, but we may as well silence the warning.
> >>
> >> Signed-off-by: Dan Carpenter <error27@gmail.com>
> >> ---
> >> V2:  In the first version I made negative values return -EINVAL
> >>
> >> diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
> >> index eb68326..1633bac 100644
> >> --- a/sound/core/rawmidi.c
> >> +++ b/sound/core/rawmidi.c
> >> @@ -829,6 +829,8 @@ static int snd_rawmidi_control_ioctl(struct snd_card *card,
> >>
> >>  		if (get_user(device, (int __user *)argp))
> >>  			return -EFAULT;
> >> +		if (device > SNDRV_RAWMIDI_DEVICES)
> >> +			return -EINVAL;
> >
> > This should be "device >= SNDRV_RAWMIDI_DEVICES".
> 
> Also note that this check changes a bit semantics. All other NEXT_DEVICE 
> ioctls returns -1 if the value is beyond the last device (meaning no more 
> devices were found). So the
> 
>                 if (device == SNDRV_RAWMIDI_DEVICES)
>                          device = -1;
> 
> check should be
> 
>                 if (device >= SNDRV_RAWMIDI_DEVICES)
>                          device = -1;
> 
> ... resulting in one line patch.

But this doesn't work when you pass device = INT_MAX :)


Takashi

Re: [patch v3] ALSA: rawmidi: fix the get next midi device ioctl

[Takashi Iwai]

At Thu, 9 Sep 2010 00:11:41 +0200,
Dan Carpenter wrote:
> 
> If we pass in a device which is higher than SNDRV_RAWMIDI_DEVICES then
> the "next device" should be -1.  This function just returns device + 1.
> 
> But the main thing is that "device + 1" can lead to a (harmless) integer
> overflow and that annoys static analysis tools.
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> V2:  In the first version I made negative values return -EINVAL
> V3:  We shouldn't return -EINVAL for numbers which are too large but
>      just set the next device to -1.
> 
> diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
> index eb68326..df67605 100644
> --- a/sound/core/rawmidi.c
> +++ b/sound/core/rawmidi.c
> @@ -829,6 +829,8 @@ static int snd_rawmidi_control_ioctl(struct snd_card *card,
>  		
>  		if (get_user(device, (int __user *)argp))
>  			return -EFAULT;
> +		if (device > SNDRV_RAWMIDI_DEVICES) /* next device is -1 */
> +			device = SNDRV_RAWMIDI_DEVICES;
>  		mutex_lock(&register_mutex);
>  		device = device < 0 ? 0 : device + 1;
>  		while (device < SNDRV_RAWMIDI_DEVICES) {
> 

We still need to cover the case device = SNDRV_RAWMIDI_DEVICES.
Also, device is incremented, so it has to be SNDRV_RAWMIDI_DEVICE - 1.
i.e.

> +		if (device >= SNDRV_RAWMIDI_DEVICES) /* next device is -1 */
> +			device = SNDRV_RAWMIDI_DEVICES - 1;


I applied the fixed patch now.


thanks,

Takashi

Re: [patch v3] ALSA: rawmidi: fix the get next midi device ioctl

[Takashi Iwai]

At Thu, 9 Sep 2010 00:11:41 +0200,
Dan Carpenter wrote:
> 
> If we pass in a device which is higher than SNDRV_RAWMIDI_DEVICES then
> the "next device" should be -1.  This function just returns device + 1.
> 
> But the main thing is that "device + 1" can lead to a (harmless) integer
> overflow and that annoys static analysis tools.
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> V2:  In the first version I made negative values return -EINVAL
> V3:  We shouldn't return -EINVAL for numbers which are too large but
>      just set the next device to -1.
> 
> diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
> index eb68326..df67605 100644
> --- a/sound/core/rawmidi.c
> +++ b/sound/core/rawmidi.c
> @@ -829,6 +829,8 @@ static int snd_rawmidi_control_ioctl(struct snd_card *card,
>  		
>  		if (get_user(device, (int __user *)argp))
>  			return -EFAULT;
> +		if (device > SNDRV_RAWMIDI_DEVICES) /* next device is -1 */
> +			device = SNDRV_RAWMIDI_DEVICES;
>  		mutex_lock(&register_mutex);
>  		device = device < 0 ? 0 : device + 1;
>  		while (device < SNDRV_RAWMIDI_DEVICES) {
> 

We still need to cover the case device == SNDRV_RAWMIDI_DEVICES.
Also, device is incremented, so it has to be SNDRV_RAWMIDI_DEVICE - 1.
i.e.

> +		if (device >= SNDRV_RAWMIDI_DEVICES) /* next device is -1 */
> +			device = SNDRV_RAWMIDI_DEVICES - 1;


I applied the fixed patch now.


thanks,

Takashi

Re: [patch v3] ALSA: rawmidi: fix the get next midi device ioctl

[walter harms]

Dan Carpenter schrieb:
> If we pass in a device which is higher than SNDRV_RAWMIDI_DEVICES then
> the "next device" should be -1.  This function just returns device + 1.
> 
> But the main thing is that "device + 1" can lead to a (harmless) integer
> overflow and that annoys static analysis tools.
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> V2:  In the first version I made negative values return -EINVAL
> V3:  We shouldn't return -EINVAL for numbers which are too large but
>      just set the next device to -1.
> 
> diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
> index eb68326..df67605 100644
> --- a/sound/core/rawmidi.c
> +++ b/sound/core/rawmidi.c
> @@ -829,6 +829,8 @@ static int snd_rawmidi_control_ioctl(struct snd_card *card,
>  		
>  		if (get_user(device, (int __user *)argp))
>  			return -EFAULT;
> +		if (device > SNDRV_RAWMIDI_DEVICES) /* next device is -1 */
> +			device = SNDRV_RAWMIDI_DEVICES;
>  		mutex_lock(&register_mutex);
>  		device = device < 0 ? 0 : device + 1;
>  		while (device < SNDRV_RAWMIDI_DEVICES) {


i am not the expert here but i sound a good idea to put all device changes into one place. like:

if (device > SNDRV_RAWMIDI_DEVICES )
	device = SNDRV_RAWMIDI_DEVICES;
else if (device < 0 )
	device = 0;
else
        device++;

just my 2 cents,
re,
 wh

Re: [patch v3] ALSA: rawmidi: fix the get next midi device ioctl

[walter harms]

Dan Carpenter schrieb:
> If we pass in a device which is higher than SNDRV_RAWMIDI_DEVICES then
> the "next device" should be -1.  This function just returns device + 1.
> 
> But the main thing is that "device + 1" can lead to a (harmless) integer
> overflow and that annoys static analysis tools.
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> V2:  In the first version I made negative values return -EINVAL
> V3:  We shouldn't return -EINVAL for numbers which are too large but
>      just set the next device to -1.
> 
> diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
> index eb68326..df67605 100644
> --- a/sound/core/rawmidi.c
> +++ b/sound/core/rawmidi.c
> @@ -829,6 +829,8 @@ static int snd_rawmidi_control_ioctl(struct snd_card *card,
>  		
>  		if (get_user(device, (int __user *)argp))
>  			return -EFAULT;
> +		if (device > SNDRV_RAWMIDI_DEVICES) /* next device is -1 */
> +			device = SNDRV_RAWMIDI_DEVICES;
>  		mutex_lock(&register_mutex);
>  		device = device < 0 ? 0 : device + 1;
>  		while (device < SNDRV_RAWMIDI_DEVICES) {


i am not the expert here but i sound a good idea to put all device changes into one place. like:

if (device > SNDRV_RAWMIDI_DEVICES )
	device = SNDRV_RAWMIDI_DEVICES;
else if (device < 0 )
	device = 0;
else
        device++;

just my 2 cents,
re,
 wh

Re: [patch v2] ALSA: rawmidi: fix the get next midi device ioctl

[Clemens Ladisch]

Dan Carpenter wrote:
> If we pass in a device which is higher than SNDRV_RAWMIDI_DEVICES then
> this function just returns device + 1 which isn't helpful.  I've
> modified it to return -EINVAL instead.
> 
> Also Smatch complains because the "device + 1" could be an integer
> overflow.  It's harmless,

It would result in device=INT_MIN, which would make the while loop go
through 2^31 values before finding the first MIDI device.


Regards,
Clemens

Re: [patch v2] ALSA: rawmidi: fix the get next midi device ioctl

[Clemens Ladisch]

Dan Carpenter wrote:
> If we pass in a device which is higher than SNDRV_RAWMIDI_DEVICES then
> this function just returns device + 1 which isn't helpful.  I've
> modified it to return -EINVAL instead.
> 
> Also Smatch complains because the "device + 1" could be an integer
> overflow.  It's harmless,

It would result in device==INT_MIN, which would make the while loop go
through 2^31 values before finding the first MIDI device.


Regards,
Clemens

Re: [patch v3] ALSA: rawmidi: fix the get next midi device ioctl

[Jaroslav Kysela]

On Thu, 9 Sep 2010, Takashi Iwai wrote:

> At Thu, 9 Sep 2010 00:11:41 +0200,
> Dan Carpenter wrote:
>>
>> If we pass in a device which is higher than SNDRV_RAWMIDI_DEVICES then
>> the "next device" should be -1.  This function just returns device + 1.
>>
>> But the main thing is that "device + 1" can lead to a (harmless) integer
>> overflow and that annoys static analysis tools.
>>
>> Signed-off-by: Dan Carpenter <error27@gmail.com>
>> ---
>> V2:  In the first version I made negative values return -EINVAL
>> V3:  We shouldn't return -EINVAL for numbers which are too large but
>>      just set the next device to -1.
>>
>> diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
>> index eb68326..df67605 100644
>> --- a/sound/core/rawmidi.c
>> +++ b/sound/core/rawmidi.c
>> @@ -829,6 +829,8 @@ static int snd_rawmidi_control_ioctl(struct snd_card *card,
>>
>>  		if (get_user(device, (int __user *)argp))
>>  			return -EFAULT;
>> +		if (device > SNDRV_RAWMIDI_DEVICES) /* next device is -1 */
>> +			device = SNDRV_RAWMIDI_DEVICES;
>>  		mutex_lock(&register_mutex);
>>  		device = device < 0 ? 0 : device + 1;
>>  		while (device < SNDRV_RAWMIDI_DEVICES) {
>>
>
> We still need to cover the case device = SNDRV_RAWMIDI_DEVICES.
> Also, device is incremented, so it has to be SNDRV_RAWMIDI_DEVICE - 1.
> i.e.
>
>> +		if (device >= SNDRV_RAWMIDI_DEVICES) /* next device is -1 */
>> +			device = SNDRV_RAWMIDI_DEVICES - 1;
>
>
> I applied the fixed patch now.

Maybe a goto to 'device = -1' line from the condition above might be more 
light (resulted instruction code size) and understandable for this case.

 						Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

Re: [patch v3] ALSA: rawmidi: fix the get next midi device ioctl

[Jaroslav Kysela]

On Thu, 9 Sep 2010, Takashi Iwai wrote:

> At Thu, 9 Sep 2010 00:11:41 +0200,
> Dan Carpenter wrote:
>>
>> If we pass in a device which is higher than SNDRV_RAWMIDI_DEVICES then
>> the "next device" should be -1.  This function just returns device + 1.
>>
>> But the main thing is that "device + 1" can lead to a (harmless) integer
>> overflow and that annoys static analysis tools.
>>
>> Signed-off-by: Dan Carpenter <error27@gmail.com>
>> ---
>> V2:  In the first version I made negative values return -EINVAL
>> V3:  We shouldn't return -EINVAL for numbers which are too large but
>>      just set the next device to -1.
>>
>> diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
>> index eb68326..df67605 100644
>> --- a/sound/core/rawmidi.c
>> +++ b/sound/core/rawmidi.c
>> @@ -829,6 +829,8 @@ static int snd_rawmidi_control_ioctl(struct snd_card *card,
>>
>>  		if (get_user(device, (int __user *)argp))
>>  			return -EFAULT;
>> +		if (device > SNDRV_RAWMIDI_DEVICES) /* next device is -1 */
>> +			device = SNDRV_RAWMIDI_DEVICES;
>>  		mutex_lock(&register_mutex);
>>  		device = device < 0 ? 0 : device + 1;
>>  		while (device < SNDRV_RAWMIDI_DEVICES) {
>>
>
> We still need to cover the case device == SNDRV_RAWMIDI_DEVICES.
> Also, device is incremented, so it has to be SNDRV_RAWMIDI_DEVICE - 1.
> i.e.
>
>> +		if (device >= SNDRV_RAWMIDI_DEVICES) /* next device is -1 */
>> +			device = SNDRV_RAWMIDI_DEVICES - 1;
>
>
> I applied the fixed patch now.

Maybe a goto to 'device = -1' line from the condition above might be more 
light (resulted instruction code size) and understandable for this case.

 						Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

Re: [patch v2] ALSA: rawmidi: fix the get next midi device ioctl

[Dan Carpenter]

On Thu, Sep 09, 2010 at 09:44:52AM +0200, Clemens Ladisch wrote:
> Dan Carpenter wrote:
> > If we pass in a device which is higher than SNDRV_RAWMIDI_DEVICES then
> > this function just returns device + 1 which isn't helpful.  I've
> > modified it to return -EINVAL instead.
> > 
> > Also Smatch complains because the "device + 1" could be an integer
> > overflow.  It's harmless,
> 
> It would result in device=INT_MIN, which would make the while loop go
> through 2^31 values before finding the first MIDI device.
> 

Oh crap.  You're right.  For some reason I got mixed up.

regards,
dan carpenter

> 
> Regards,
> Clemens

Re: [patch v2] ALSA: rawmidi: fix the get next midi device ioctl

[Dan Carpenter]

On Thu, Sep 09, 2010 at 09:44:52AM +0200, Clemens Ladisch wrote:
> Dan Carpenter wrote:
> > If we pass in a device which is higher than SNDRV_RAWMIDI_DEVICES then
> > this function just returns device + 1 which isn't helpful.  I've
> > modified it to return -EINVAL instead.
> > 
> > Also Smatch complains because the "device + 1" could be an integer
> > overflow.  It's harmless,
> 
> It would result in device==INT_MIN, which would make the while loop go
> through 2^31 values before finding the first MIDI device.
> 

Oh crap.  You're right.  For some reason I got mixed up.

regards,
dan carpenter

> 
> Regards,
> Clemens