[patch] ALSA: riptide: off by one in snd_riptide_joystick_probe()

[patch] ALSA: riptide: off by one in snd_riptide_joystick_probe()

[Dan Carpenter]

There is an off by one bug in snd_riptide_joystick_probe() because we
do the "dev++" toward the start of the function instead of waiting until
the end.

The impact of this bug is:
1) A static checker warning.
2) If you connect 32 joysticks to your computer they will fail to load
   with a different error message than intended.
3) If you pass a module option to specify a non-default joystick port
   then the parameter array has to be shifted one element.

The first two are not serious.  For the third one, it's probably too
late to change it since the bug was introduced in 2009.  Either no one
noticed and we can leave it alone, or if they did notice, they probably
have implemented work arounds so we can't change it now without
confusing them.

So instead of fixing it the "theoretically correct way", I have decided
to just work around it by making the joystick_port[] one element
larger.  This silences the static checker warning and doesn't affect
user space.

Fixes: db1005ec6ff8 ('ALSA: riptide - Fix joystick resource handling')
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/sound/pci/riptide/riptide.c b/sound/pci/riptide/riptide.c
index 29f2827..324e34c 100644
--- a/sound/pci/riptide/riptide.c
+++ b/sound/pci/riptide/riptide.c
@@ -125,7 +125,11 @@ static char *id[SNDRV_CARDS] = SNDRV_DEFAULT_STR;
 static bool enable[SNDRV_CARDS] = SNDRV_DEFAULT_ENABLE;
 
 #ifdef SUPPORT_JOYSTICK
-static int joystick_port[SNDRV_CARDS] = { [0 ... (SNDRV_CARDS - 1)] = 0x200 };
+/*
+ * The joystick_port accounting is off by one but I'm afraid to change it for
+ * fear of breaking userspace.
+ */
+static int joystick_port[SNDRV_CARDS + 1] = { [0 ... (SNDRV_CARDS)] = 0x200 };
 #endif
 static int mpu_port[SNDRV_CARDS] = { [0 ... (SNDRV_CARDS - 1)] = 0x330 };
 static int opl3_port[SNDRV_CARDS] = { [0 ... (SNDRV_CARDS - 1)] = 0x388 };

Re: [patch] ALSA: riptide: off by one in snd_riptide_joystick_probe()

[Takashi Iwai]

At Sun, 1 Feb 2015 23:49:02 +0300,
Dan Carpenter wrote:
> 
> There is an off by one bug in snd_riptide_joystick_probe() because we
> do the "dev++" toward the start of the function instead of waiting until
> the end.
> 
> The impact of this bug is:
> 1) A static checker warning.
> 2) If you connect 32 joysticks to your computer they will fail to load
>    with a different error message than intended.
> 3) If you pass a module option to specify a non-default joystick port
>    then the parameter array has to be shifted one element.
> 
> The first two are not serious.  For the third one, it's probably too
> late to change it since the bug was introduced in 2009.  Either no one
> noticed and we can leave it alone, or if they did notice, they probably
> have implemented work arounds so we can't change it now without
> confusing them.
> 
> So instead of fixing it the "theoretically correct way", I have decided
> to just work around it by making the joystick_port[] one element
> larger.  This silences the static checker warning and doesn't affect
> user space.
> 
> Fixes: db1005ec6ff8 ('ALSA: riptide - Fix joystick resource handling')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Well, I bet that no one specified this option, so far.  The only case
you need another address is the multiple analog joystick ports, and
the board is so rare.

IMO, this bug can be classified as the "behavior bug" to be fixed.
It's a fix for a regression that already broke user-space, after all.

So, could you rewrite the patch as a normal off-by-one fix?


thanks,

Takashi

> 
> diff --git a/sound/pci/riptide/riptide.c b/sound/pci/riptide/riptide.c
> index 29f2827..324e34c 100644
> --- a/sound/pci/riptide/riptide.c
> +++ b/sound/pci/riptide/riptide.c
> @@ -125,7 +125,11 @@ static char *id[SNDRV_CARDS] = SNDRV_DEFAULT_STR;
>  static bool enable[SNDRV_CARDS] = SNDRV_DEFAULT_ENABLE;
>  
>  #ifdef SUPPORT_JOYSTICK
> -static int joystick_port[SNDRV_CARDS] = { [0 ... (SNDRV_CARDS - 1)] = 0x200 };
> +/*
> + * The joystick_port accounting is off by one but I'm afraid to change it for
> + * fear of breaking userspace.
> + */
> +static int joystick_port[SNDRV_CARDS + 1] = { [0 ... (SNDRV_CARDS)] = 0x200 };
>  #endif
>  static int mpu_port[SNDRV_CARDS] = { [0 ... (SNDRV_CARDS - 1)] = 0x330 };
>  static int opl3_port[SNDRV_CARDS] = { [0 ... (SNDRV_CARDS - 1)] = 0x388 };
> 

Re: [patch] ALSA: riptide: off by one in snd_riptide_joystick_probe()

[Dan Carpenter]

On Mon, Feb 02, 2015 at 11:42:37AM +0100, Takashi Iwai wrote:
> 
> So, could you rewrite the patch as a normal off-by-one fix?
> 

Sure, I'll do that later this week.

regards,
dan carpenter

[patch v2] ALSA: off by one bug in snd_riptide_joystick_probe()

[Dan Carpenter]

The problem here is that we check:

	if (dev >= SNDRV_CARDS)

Then we increment "dev".

       if (!joystick_port[dev++])

Then we use it as an offset into a array with SNDRV_CARDS elements.

	if (!request_region(joystick_port[dev], 8, "Riptide gameport")) {

This has 3 effects:
1) If you use the module option to specify the joystick port then it has
   to be shifted one space over.
2) The wrong error message will be printed on failure if you have over
   32 cards.
3) Static checkers will correctly complain that are off by one.

Fixes: db1005ec6ff8 ('ALSA: riptide - Fix joystick resource handling')
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: In the original patch I just made the array larger.

diff --git a/sound/pci/riptide/riptide.c b/sound/pci/riptide/riptide.c
index 29f2827..94639d6 100644
--- a/sound/pci/riptide/riptide.c
+++ b/sound/pci/riptide/riptide.c
@@ -2011,32 +2011,43 @@ snd_riptide_joystick_probe(struct pci_dev *pci, const struct pci_device_id *id)
 {
 	static int dev;
 	struct gameport *gameport;
+	int ret;
 
 	if (dev >= SNDRV_CARDS)
 		return -ENODEV;
+
 	if (!enable[dev]) {
-		dev++;
-		return -ENOENT;
+		ret = -ENOENT;
+		goto inc_dev;
 	}
 
-	if (!joystick_port[dev++])
-		return 0;
+	if (!joystick_port[dev]) {
+		ret = 0;
+		goto inc_dev;
+	}
 
 	gameport = gameport_allocate_port();
-	if (!gameport)
-		return -ENOMEM;
+	if (!gameport) {
+		ret = -ENOMEM;
+		goto inc_dev;
+	}
 	if (!request_region(joystick_port[dev], 8, "Riptide gameport")) {
 		snd_printk(KERN_WARNING
 			   "Riptide: cannot grab gameport 0x%x\n",
 			   joystick_port[dev]);
 		gameport_free_port(gameport);
-		return -EBUSY;
+		ret = -EBUSY;
+		goto inc_dev;
 	}
 
 	gameport->io = joystick_port[dev];
 	gameport_register_port(gameport);
 	pci_set_drvdata(pci, gameport);
-	return 0;
+
+	ret = 0;
+inc_dev:
+	dev++;
+	return ret;
 }
 
 static void snd_riptide_joystick_remove(struct pci_dev *pci)

Re: [patch v2] ALSA: off by one bug in snd_riptide_joystick_probe()

[Takashi Iwai]

At Mon, 9 Feb 2015 16:51:40 +0300,
Dan Carpenter wrote:
> 
> The problem here is that we check:
> 
> 	if (dev >= SNDRV_CARDS)
> 
> Then we increment "dev".
> 
>        if (!joystick_port[dev++])
> 
> Then we use it as an offset into a array with SNDRV_CARDS elements.
> 
> 	if (!request_region(joystick_port[dev], 8, "Riptide gameport")) {
> 
> This has 3 effects:
> 1) If you use the module option to specify the joystick port then it has
>    to be shifted one space over.
> 2) The wrong error message will be printed on failure if you have over
>    32 cards.
> 3) Static checkers will correctly complain that are off by one.
> 
> Fixes: db1005ec6ff8 ('ALSA: riptide - Fix joystick resource handling')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: In the original patch I just made the array larger.

Applied, thanks.


Takashi

> 
> diff --git a/sound/pci/riptide/riptide.c b/sound/pci/riptide/riptide.c
> index 29f2827..94639d6 100644
> --- a/sound/pci/riptide/riptide.c
> +++ b/sound/pci/riptide/riptide.c
> @@ -2011,32 +2011,43 @@ snd_riptide_joystick_probe(struct pci_dev *pci, const struct pci_device_id *id)
>  {
>  	static int dev;
>  	struct gameport *gameport;
> +	int ret;
>  
>  	if (dev >= SNDRV_CARDS)
>  		return -ENODEV;
> +
>  	if (!enable[dev]) {
> -		dev++;
> -		return -ENOENT;
> +		ret = -ENOENT;
> +		goto inc_dev;
>  	}
>  
> -	if (!joystick_port[dev++])
> -		return 0;
> +	if (!joystick_port[dev]) {
> +		ret = 0;
> +		goto inc_dev;
> +	}
>  
>  	gameport = gameport_allocate_port();
> -	if (!gameport)
> -		return -ENOMEM;
> +	if (!gameport) {
> +		ret = -ENOMEM;
> +		goto inc_dev;
> +	}
>  	if (!request_region(joystick_port[dev], 8, "Riptide gameport")) {
>  		snd_printk(KERN_WARNING
>  			   "Riptide: cannot grab gameport 0x%x\n",
>  			   joystick_port[dev]);
>  		gameport_free_port(gameport);
> -		return -EBUSY;
> +		ret = -EBUSY;
> +		goto inc_dev;
>  	}
>  
>  	gameport->io = joystick_port[dev];
>  	gameport_register_port(gameport);
>  	pci_set_drvdata(pci, gameport);
> -	return 0;
> +
> +	ret = 0;
> +inc_dev:
> +	dev++;
> +	return ret;
>  }
>  
>  static void snd_riptide_joystick_remove(struct pci_dev *pci)
>