* [patch] ALSA: rawmidi: cleanup the get next midi device ioctl
@ 2010-09-08 8:53 ` Dan Carpenter
0 siblings, 0 replies; 24+ messages in thread
From: Dan Carpenter @ 2010-09-08 8:53 UTC (permalink / raw)
To: Jaroslav Kysela
Cc: alsa-devel, Takashi Iwai, Clemens Ladisch, kernel-janitors,
Kyle McMartin, Ulrich Drepper
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(®ister_mutex);
- device = device < 0 ? 0 : device + 1;
+ device++;
while (device < SNDRV_RAWMIDI_DEVICES) {
if (snd_rawmidi_search(card, device))
break;
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [patch] ALSA: rawmidi: cleanup the get next midi device ioctl
@ 2010-09-08 8:53 ` Dan Carpenter
0 siblings, 0 replies; 24+ messages in thread
From: Dan Carpenter @ 2010-09-08 8:53 UTC (permalink / raw)
To: Jaroslav Kysela
Cc: alsa-devel, Takashi Iwai, Clemens Ladisch, kernel-janitors,
Kyle McMartin, Ulrich Drepper
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(®ister_mutex);
- device = device < 0 ? 0 : device + 1;
+ device++;
while (device < SNDRV_RAWMIDI_DEVICES) {
if (snd_rawmidi_search(card, device))
break;
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [patch] ALSA: rawmidi: cleanup the get next midi device ioctl
2010-09-08 8:53 ` Dan Carpenter
@ 2010-09-08 9:40 ` Clemens Ladisch
-1 siblings, 0 replies; 24+ messages in thread
From: Clemens Ladisch @ 2010-09-08 9:40 UTC (permalink / raw)
To: Dan Carpenter
Cc: alsa-devel, Takashi Iwai, kernel-janitors, Kyle McMartin, Ulrich Drepper
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [patch] ALSA: rawmidi: cleanup the get next midi device ioctl
@ 2010-09-08 9:40 ` Clemens Ladisch
0 siblings, 0 replies; 24+ messages in thread
From: Clemens Ladisch @ 2010-09-08 9:40 UTC (permalink / raw)
To: Dan Carpenter
Cc: alsa-devel, Takashi Iwai, kernel-janitors, Kyle McMartin, Ulrich Drepper
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* [patch v2] ALSA: rawmidi: fix the get next midi device ioctl
2010-09-08 8:53 ` Dan Carpenter
@ 2010-09-08 19:36 ` Dan Carpenter
-1 siblings, 0 replies; 24+ messages in thread
From: Dan Carpenter @ 2010-09-08 19:36 UTC (permalink / raw)
To: Jaroslav Kysela
Cc: alsa-devel, Takashi Iwai, Clemens Ladisch, kernel-janitors,
Kyle McMartin, Ulrich Drepper
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(®ister_mutex);
device = device < 0 ? 0 : device + 1;
while (device < SNDRV_RAWMIDI_DEVICES) {
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [patch v2] ALSA: rawmidi: fix the get next midi device ioctl
@ 2010-09-08 19:36 ` Dan Carpenter
0 siblings, 0 replies; 24+ messages in thread
From: Dan Carpenter @ 2010-09-08 19:36 UTC (permalink / raw)
To: Jaroslav Kysela
Cc: alsa-devel, Takashi Iwai, Clemens Ladisch, kernel-janitors,
Kyle McMartin, Ulrich Drepper
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(®ister_mutex);
device = device < 0 ? 0 : device + 1;
while (device < SNDRV_RAWMIDI_DEVICES) {
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [patch v2] ALSA: rawmidi: fix the get next midi device ioctl
2010-09-08 19:36 ` Dan Carpenter
@ 2010-09-08 19:56 ` Takashi Iwai
-1 siblings, 0 replies; 24+ messages in thread
From: Takashi Iwai @ 2010-09-08 19:56 UTC (permalink / raw)
To: Dan Carpenter
Cc: alsa-devel, Clemens Ladisch, kernel-janitors, Kyle McMartin,
Ulrich Drepper
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(®ister_mutex);
> device = device < 0 ? 0 : device + 1;
> while (device < SNDRV_RAWMIDI_DEVICES) {
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [patch v2] ALSA: rawmidi: fix the get next midi device ioctl
@ 2010-09-08 19:56 ` Takashi Iwai
0 siblings, 0 replies; 24+ messages in thread
From: Takashi Iwai @ 2010-09-08 19:56 UTC (permalink / raw)
To: Dan Carpenter
Cc: alsa-devel, Clemens Ladisch, kernel-janitors, Kyle McMartin,
Ulrich Drepper
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(®ister_mutex);
> device = device < 0 ? 0 : device + 1;
> while (device < SNDRV_RAWMIDI_DEVICES) {
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [patch v2] ALSA: rawmidi: fix the get next midi device ioctl
2010-09-08 19:56 ` Takashi Iwai
@ 2010-09-08 21:29 ` Jaroslav Kysela
-1 siblings, 0 replies; 24+ messages in thread
From: Jaroslav Kysela @ 2010-09-08 21:29 UTC (permalink / raw)
To: Takashi Iwai
Cc: ALSA development, Dan Carpenter, Clemens Ladisch,
kernel-janitors, Kyle McMartin, Ulrich Drepper
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.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [patch v2] ALSA: rawmidi: fix the get next midi device ioctl
@ 2010-09-08 21:29 ` Jaroslav Kysela
0 siblings, 0 replies; 24+ messages in thread
From: Jaroslav Kysela @ 2010-09-08 21:29 UTC (permalink / raw)
To: Takashi Iwai
Cc: ALSA development, Dan Carpenter, Clemens Ladisch,
kernel-janitors, Kyle McMartin, Ulrich Drepper
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.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [patch v3] ALSA: rawmidi: fix the get next midi device ioctl
2010-09-08 21:29 ` Jaroslav Kysela
@ 2010-09-08 22:11 ` Dan Carpenter
-1 siblings, 0 replies; 24+ messages in thread
From: Dan Carpenter @ 2010-09-08 22:11 UTC (permalink / raw)
To: Jaroslav Kysela
Cc: ALSA development, Takashi Iwai, Clemens Ladisch, kernel-janitors,
Kyle McMartin, Ulrich Drepper
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(®ister_mutex);
device = device < 0 ? 0 : device + 1;
while (device < SNDRV_RAWMIDI_DEVICES) {
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [patch v3] ALSA: rawmidi: fix the get next midi device ioctl
@ 2010-09-08 22:11 ` Dan Carpenter
0 siblings, 0 replies; 24+ messages in thread
From: Dan Carpenter @ 2010-09-08 22:11 UTC (permalink / raw)
To: Jaroslav Kysela
Cc: ALSA development, Takashi Iwai, Clemens Ladisch, kernel-janitors,
Kyle McMartin, Ulrich Drepper
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(®ister_mutex);
device = device < 0 ? 0 : device + 1;
while (device < SNDRV_RAWMIDI_DEVICES) {
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [patch v2] ALSA: rawmidi: fix the get next midi device ioctl
2010-09-08 21:29 ` Jaroslav Kysela
@ 2010-09-09 6:57 ` Takashi Iwai
-1 siblings, 0 replies; 24+ messages in thread
From: Takashi Iwai @ 2010-09-09 6:57 UTC (permalink / raw)
To: Jaroslav Kysela
Cc: ALSA development, Dan Carpenter, Clemens Ladisch,
kernel-janitors, Kyle McMartin, Ulrich Drepper
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [patch v2] ALSA: rawmidi: fix the get next midi device ioctl
@ 2010-09-09 6:57 ` Takashi Iwai
0 siblings, 0 replies; 24+ messages in thread
From: Takashi Iwai @ 2010-09-09 6:57 UTC (permalink / raw)
To: Jaroslav Kysela
Cc: ALSA development, Dan Carpenter, Clemens Ladisch,
kernel-janitors, Kyle McMartin, Ulrich Drepper
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [patch v3] ALSA: rawmidi: fix the get next midi device ioctl
2010-09-08 22:11 ` Dan Carpenter
@ 2010-09-09 7:07 ` Takashi Iwai
-1 siblings, 0 replies; 24+ messages in thread
From: Takashi Iwai @ 2010-09-09 7:07 UTC (permalink / raw)
To: Dan Carpenter
Cc: ALSA development, Clemens Ladisch, kernel-janitors,
Kyle McMartin, Ulrich Drepper
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(®ister_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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [patch v3] ALSA: rawmidi: fix the get next midi device ioctl
@ 2010-09-09 7:07 ` Takashi Iwai
0 siblings, 0 replies; 24+ messages in thread
From: Takashi Iwai @ 2010-09-09 7:07 UTC (permalink / raw)
To: Dan Carpenter
Cc: ALSA development, Clemens Ladisch, kernel-janitors,
Kyle McMartin, Ulrich Drepper
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(®ister_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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [patch v3] ALSA: rawmidi: fix the get next midi device ioctl
2010-09-08 22:11 ` Dan Carpenter
@ 2010-09-09 7:23 ` walter harms
-1 siblings, 0 replies; 24+ messages in thread
From: walter harms @ 2010-09-09 7:23 UTC (permalink / raw)
To: Dan Carpenter
Cc: ALSA development, Takashi Iwai, kernel-janitors, Clemens Ladisch,
Kyle McMartin, Ulrich Drepper
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(®ister_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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [patch v3] ALSA: rawmidi: fix the get next midi device ioctl
@ 2010-09-09 7:23 ` walter harms
0 siblings, 0 replies; 24+ messages in thread
From: walter harms @ 2010-09-09 7:23 UTC (permalink / raw)
To: Dan Carpenter
Cc: ALSA development, Takashi Iwai, kernel-janitors, Clemens Ladisch,
Kyle McMartin, Ulrich Drepper
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(®ister_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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [patch v2] ALSA: rawmidi: fix the get next midi device ioctl
2010-09-08 19:36 ` Dan Carpenter
@ 2010-09-09 7:44 ` Clemens Ladisch
-1 siblings, 0 replies; 24+ messages in thread
From: Clemens Ladisch @ 2010-09-09 7:44 UTC (permalink / raw)
To: Dan Carpenter
Cc: alsa-devel, Takashi Iwai, kernel-janitors, Kyle McMartin, Ulrich Drepper
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [patch v2] ALSA: rawmidi: fix the get next midi device ioctl
@ 2010-09-09 7:44 ` Clemens Ladisch
0 siblings, 0 replies; 24+ messages in thread
From: Clemens Ladisch @ 2010-09-09 7:44 UTC (permalink / raw)
To: Dan Carpenter
Cc: alsa-devel, Takashi Iwai, kernel-janitors, Kyle McMartin, Ulrich Drepper
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [patch v3] ALSA: rawmidi: fix the get next midi device ioctl
2010-09-09 7:07 ` Takashi Iwai
@ 2010-09-09 8:36 ` Jaroslav Kysela
-1 siblings, 0 replies; 24+ messages in thread
From: Jaroslav Kysela @ 2010-09-09 8:36 UTC (permalink / raw)
To: Takashi Iwai
Cc: ALSA development, Dan Carpenter, Clemens Ladisch,
kernel-janitors, Kyle McMartin, Ulrich Drepper
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(®ister_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.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [patch v3] ALSA: rawmidi: fix the get next midi device ioctl
@ 2010-09-09 8:36 ` Jaroslav Kysela
0 siblings, 0 replies; 24+ messages in thread
From: Jaroslav Kysela @ 2010-09-09 8:36 UTC (permalink / raw)
To: Takashi Iwai
Cc: ALSA development, Dan Carpenter, Clemens Ladisch,
kernel-janitors, Kyle McMartin, Ulrich Drepper
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(®ister_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.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [patch v2] ALSA: rawmidi: fix the get next midi device ioctl
2010-09-09 7:44 ` Clemens Ladisch
@ 2010-09-09 8:46 ` Dan Carpenter
-1 siblings, 0 replies; 24+ messages in thread
From: Dan Carpenter @ 2010-09-09 8:46 UTC (permalink / raw)
To: Clemens Ladisch
Cc: alsa-devel, Takashi Iwai, kernel-janitors, Kyle McMartin, Ulrich Drepper
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [patch v2] ALSA: rawmidi: fix the get next midi device ioctl
@ 2010-09-09 8:46 ` Dan Carpenter
0 siblings, 0 replies; 24+ messages in thread
From: Dan Carpenter @ 2010-09-09 8:46 UTC (permalink / raw)
To: Clemens Ladisch
Cc: alsa-devel, Takashi Iwai, kernel-janitors, Kyle McMartin, Ulrich Drepper
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
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2010-09-09 8:46 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-09-08 8:53 [patch] ALSA: rawmidi: cleanup the get next midi device ioctl Dan Carpenter
2010-09-08 8:53 ` Dan Carpenter
2010-09-08 9:40 ` Clemens Ladisch
2010-09-08 9:40 ` Clemens Ladisch
2010-09-08 19:36 ` [patch v2] ALSA: rawmidi: fix " Dan Carpenter
2010-09-08 19:36 ` Dan Carpenter
2010-09-08 19:56 ` Takashi Iwai
2010-09-08 19:56 ` Takashi Iwai
2010-09-08 21:29 ` Jaroslav Kysela
2010-09-08 21:29 ` Jaroslav Kysela
2010-09-08 22:11 ` [patch v3] " Dan Carpenter
2010-09-08 22:11 ` Dan Carpenter
2010-09-09 7:07 ` Takashi Iwai
2010-09-09 7:07 ` Takashi Iwai
2010-09-09 8:36 ` Jaroslav Kysela
2010-09-09 8:36 ` Jaroslav Kysela
2010-09-09 7:23 ` walter harms
2010-09-09 7:23 ` walter harms
2010-09-09 6:57 ` [patch v2] " Takashi Iwai
2010-09-09 6:57 ` Takashi Iwai
2010-09-09 7:44 ` Clemens Ladisch
2010-09-09 7:44 ` Clemens Ladisch
2010-09-09 8:46 ` Dan Carpenter
2010-09-09 8:46 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.