Skip to contentSkip to conversation

hwdep: silence integer overflow warning

PATCH Superseded Dan Carpenter 3 participants 7 messages Oct 20, 2011 – Oct 31, 2011

Subsystems (2)
Sound Kernel janitors
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 hwdep: silence integer overflow warning

Smatch complains that if device is INT_MAX then device + 1 can overflow. It just means we would have an annoying loop while we check all the devices from -2147483648 to SNDRV_MINOR_HWDEPS.

Signed-off-by Dan Carpenter <[email protected]>
sound/core/hwdep.c +8 -1
index c7ceb28..edfdf6d 100644
@@ -273,7 +273,14 @@ static int snd_hwdep_control_ioctl(struct snd_card *card,
 			if (get_user(device, (int __user *)arg))
 				return -EFAULT;
 			mutex_lock(&register_mutex);
-			device = device < 0 ? 0 : device + 1;
+
+			if (device < 0)
+				device = 0;
+			else if (device > SNDRV_MINOR_HWDEPS)
+				device = SNDRV_MINOR_HWDEPS;
+			else
+				device++;
+
 			while (device < SNDRV_MINOR_HWDEPS) {
 				if (snd_hwdep_search(card, device))
 					break;
#1 lore raw

Am 20.10.2011 08:09, schrieb Dan Carpenter:

Quoted 16 lines

Smatch complains that if device is INT_MAX then device + 1 can overflow. It just means we would have an annoying loop while we check all the devices from -2147483648 to SNDRV_MINOR_HWDEPS.

Signed-off-by Dan Carpenter <[email protected]>
sound/core/hwdep.c +1 -1
index c7ceb28..edfdf6d 100644
@@ -273,7 +273,14 @@ static int snd_hwdep_control_ioctl(struct snd_card *card,
 			if (get_user(device, (int __user *)arg))
 				return -EFAULT;
 			mutex_lock(&register_mutex);
-			device = device < 0 ? 0 : device + 1;
+

sound/core/hwdep.c static int snd_hwdep_control_ioctl(struct snd_card *card, · line 286

Quoted 10 lines
sound/core/hwdep.c +7 -0
+			if (device < 0)
+				device = 0;
+			else if (device > SNDRV_MINOR_HWDEPS)
+				device = SNDRV_MINOR_HWDEPS;
+			else
+				device++;
+
 			while (device < SNDRV_MINOR_HWDEPS) {
 				if (snd_hwdep_search(card, device))
 					break;

I am not sure how SNDRV_MINOR_HWDEPS is used further but:

		else if (device < SNDRV_MINOR_HWDEPS)
				device++;
	        else
	              device = SNDRV_MINOR_HWDEPS;

that cuts the corner case device = SNDRV_MINOR_HWDEPS.

re,
wh

Quoted 5 lines

To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#2 lore raw
Quoted 1 line · walter harms

I am not sure how SNDRV_MINOR_HWDEPS is used further

Either device = SNDRV_MINOR_HWDEPS or device = SNDRV_MINOR_HWDEPS + 1 are fine (equivalent) but it's a fair point. Your version is nicer.

I'll redo the patch.

regards,
dan carpenter

#3 lore raw

Am 28.10.2011 08:45, schrieb Dan Carpenter:

Quoted 8 lines · walter harms
Quoted 1 line

I am not sure how SNDRV_MINOR_HWDEPS is used further

Either device = SNDRV_MINOR_HWDEPS or device = SNDRV_MINOR_HWDEPS + 1 are fine (equivalent) but it's a fair point. Your version is nicer.

I'll redo the patch.

An other question is:
Should the user be informed that the device is modified ? The Problem i see is that a user parameter is silently modified and buggy code will come through.

re,
wh

author
#4 lore raw
Quoted 9 lines · walter harms

Am 28.10.2011 08:45, schrieb Dan Carpenter:

Quoted 6 lines · walter harms
Quoted 1 line

I am not sure how SNDRV_MINOR_HWDEPS is used further

Either device = SNDRV_MINOR_HWDEPS or device = SNDRV_MINOR_HWDEPS + 1 are fine (equivalent) but it's a fair point. Your version is nicer.

Quoted 7 lines
Quoted 2 lines

I'll redo the patch.

An other question is:
Should the user be informed that the device is modified ? The Problem i see is that a user parameter is silently modified and buggy code will come through.

That would be an API change.

This is cut and pasted code, and we've had this exact discussion before for sound/core/rawmidi.c a year ago. You were involved in the discussion too. :P
http://marc.info/?l=kernel-janitors&m8401699203676&w=2

regards,
dan carpenter

author
#5 lore raw

PATCH v2 1/1 hwdep: silence integer overflow warning

In reply to walter harms

Smatch complains that if device is INT_MAX then device + 1 can overflow. It just means we would have an annoying loop while we check all the devices from -2147483648 to SNDRV_MINOR_HWDEPS.

Signed-off-by Dan Carpenter <[email protected]>

---
v2: reshuffled it a bit per Walter Harms's suggestion.

sound/core/hwdep.c +8 -1
index c7ceb28..75ea16f 100644
@@ -273,7 +273,14 @@ static int snd_hwdep_control_ioctl(struct snd_card *card,
 			if (get_user(device, (int __user *)arg))
 				return -EFAULT;
 			mutex_lock(&register_mutex);
-			device = device < 0 ? 0 : device + 1;
+
+			if (device < 0)
+				device = 0;
+			else if (device < SNDRV_MINOR_HWDEPS)
+				device++;
+			else
+				device = SNDRV_MINOR_HWDEPS;
+
 			while (device < SNDRV_MINOR_HWDEPS) {
 				if (snd_hwdep_search(card, device))
 					break;
#6 lore raw

At Fri, 28 Oct 2011 09:46:01 +0300,

Quoted 6 lines · Dan Carpenter

Smatch complains that if device is INT_MAX then device + 1 can overflow. It just means we would have an annoying loop while we check all the devices from -2147483648 to SNDRV_MINOR_HWDEPS.

Signed-off-by Dan Carpenter <[email protected]>

Applied now. Thanks!

Takashi

Quoted 29 lines

---
v2: reshuffled it a bit per Walter Harms's suggestion.

sound/core/hwdep.c +8 -1
index c7ceb28..75ea16f 100644
@@ -273,7 +273,14 @@ static int snd_hwdep_control_ioctl(struct snd_card *card,
 			if (get_user(device, (int __user *)arg))
 				return -EFAULT;
 			mutex_lock(&register_mutex);
-			device = device < 0 ? 0 : device + 1;
+
+			if (device < 0)
+				device = 0;
+			else if (device < SNDRV_MINOR_HWDEPS)
+				device++;
+			else
+				device = SNDRV_MINOR_HWDEPS;
+
 			while (device < SNDRV_MINOR_HWDEPS) {
 				if (snd_hwdep_search(card, device))
 					break;

_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel