[PATCH] Fix NULL dereference when suspending snd_opl3sa2

[PATCH] Fix NULL dereference when suspending snd_opl3sa2

[Lubomir Rintel]

This should fix the following OOPS:
http://www.kerneloops.org/raw.php?rawid=80591&msgid=

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 sound/isa/opl3sa2.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/sound/isa/opl3sa2.c b/sound/isa/opl3sa2.c
index 58c972b..483c284 100644
--- a/sound/isa/opl3sa2.c
+++ b/sound/isa/opl3sa2.c
@@ -553,7 +553,8 @@ static int snd_opl3sa2_suspend(struct snd_card
*card, pm_message_t state)
 	struct snd_opl3sa2 *chip = card->private_data;
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	chip->wss->suspend(chip->wss);
+	if (chip->wss->suspend)
+		chip->wss->suspend(chip->wss);
 	/* power down */
 	snd_opl3sa2_write(chip, OPL3SA2_PM_CTRL, OPL3SA2_PM_D3);
 
-- 
1.5.5.6

Re: [PATCH] Fix NULL dereference when suspending snd_opl3sa2

[Lubomir Rintel]

On Mon, 2009-03-16 at 11:01 +0100, Lubomir Rintel wrote:
> This should fix the following OOPS:
> http://www.kerneloops.org/raw.php?rawid=80591&msgid=

Wrong, sorry, the disassembly from kerneloops was a bit confusing to me.
Looking at the disassembly of the actual module:

> @@ -553,7 +553,8 @@ static int snd_opl3sa2_suspend(struct snd_card
> *card, pm_message_t state)
>  	struct snd_opl3sa2 *chip = card->private_data;

card must somehow be NULL here:

00000270 <snd_opl3sa2_suspend>:
 270:   53                      push   %ebx
 271:   8b 98 38 01 00 00       mov    0x138(%eax),%ebx
 277:   b9 01 00 00 00          mov    $0x1,%ecx
 27c:   ba 03 00 00 00          mov    $0x3,%edx
 281:   c7 80 a8 01 00 00 00    movl   $0x300,0x1a8(%eax)
 288:   03 00 00 
 28b:   05 bc 01 00 00          add    $0x1bc,%eax
 290:   6a 00                   push   $0x0
 292:   e8 fc ff ff ff          call   293 <snd_opl3sa2_suspend+0x23>
 297:   8b 53 24                mov    0x24(%ebx),%edx
 29a:   89 d0                   mov    %edx,%eax
 29c:   ff 92 d4 00 00 00       call   *0xd4(%edx)
 2a2:   89 d8                   mov    %ebx,%eax
 2a4:   b9 27 00 00 00          mov    $0x27,%ecx
 2a9:   ba 01 00 00 00          mov    $0x1,%edx
 2ae:   e8 72 fe ff ff          call   125 <snd_opl3sa2_write>
 2b3:   58                      pop    %eax
 2b4:   31 c0                   xor    %eax,%eax
 2b6:   5b                      pop    %ebx
 2b7:   c3                      ret    

This is the same issue, with 2.6.28:
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=519939

Re: [PATCH] Fix NULL dereference when suspending snd_opl3sa2

[Krzysztof Helt]

On Mon, 16 Mar 2009 15:16:08 +0100
Lubomir Rintel <lkundrak@v3.sk> wrote:

> On Mon, 2009-03-16 at 11:01 +0100, Lubomir Rintel wrote:
> > This should fix the following OOPS:
> > http://www.kerneloops.org/raw.php?rawid=80591&msgid=
> 

> 
> This is the same issue, with 2.6.28:
> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=519939
> 
> 

I'll look into this.

Regards,
Krzysztof

Re: [PATCH] Fix NULL dereference when suspending snd_opl3sa2

[Krzysztof Helt]

On Mon, 16 Mar 2009 15:16:08 +0100
Lubomir Rintel <lkundrak@v3.sk> wrote:

> On Mon, 2009-03-16 at 11:01 +0100, Lubomir Rintel wrote:
> > This should fix the following OOPS:
> > http://www.kerneloops.org/raw.php?rawid=80591&msgid=
> 

> This is the same issue, with 2.6.28:
> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=519939
> 
> 

One should no force loading of the snd-opl3sa2 driver
in the above cases. It was forced as the opl3sa2 is a PnP chip
and the drivers were loaded as plain ISA devices.

However, this is not a cause of the OOPS. The cause is that
despite the driver returns -ENODEV, the higher layer
hides it and the driver is bound to the bus. I suppose,
the suspend and resume functions are called on it.

Look at the ISA probe function:

static int __devinit snd_opl3sa2_isa_probe(struct device *pdev,
                       unsigned int dev)
{
    struct snd_card *card;
    int err;

    card = snd_opl3sa2_card_new(dev);
    if (! card)
        return -ENOMEM;
    snd_card_set_dev(card, pdev);
    if ((err = snd_opl3sa2_probe(card, dev)) < 0) {
        snd_card_free(card);
        return err;
    }
    dev_set_drvdata(pdev, card);
    return 0;
}

If the probe function fails, the drvdata is
never set to the snd_card structure pointer.

The patch below should fix suspend and resume
functions.

Regards,
Krzysztof
PS. It is the same situation for PnP and
BIOS PnP probing in this driver.
---

Fix the OOPS during a opl3sa2 card suspend
and resume if the driver is loaded but the card
is not found.

Signed-off-by: Krzysztof Helt <krzysztof.h1@wp.pl>

--- a/sound/isa/opl3sa2.c~	2008-10-13 18:03:52.000000000 +0200
+++ b/sound/isa/opl3sa2.c	2009-03-16 21:20:30.841348261 +0100
@@ -550,21 +550,27 @@ static int __devinit snd_opl3sa2_mixer(s
 #ifdef CONFIG_PM
 static int snd_opl3sa2_suspend(struct snd_card *card, pm_message_t state)
 {
-	struct snd_opl3sa2 *chip = card->private_data;
+	if (card) {
+		struct snd_opl3sa2 *chip = card->private_data;
 
-	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	chip->wss->suspend(chip->wss);
-	/* power down */
-	snd_opl3sa2_write(chip, OPL3SA2_PM_CTRL, OPL3SA2_PM_D3);
+		snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
+		chip->wss->suspend(chip->wss);
+		/* power down */
+		snd_opl3sa2_write(chip, OPL3SA2_PM_CTRL, OPL3SA2_PM_D3);
+	}
 
 	return 0;
 }
 
 static int snd_opl3sa2_resume(struct snd_card *card)
 {
-	struct snd_opl3sa2 *chip = card->private_data;
+	struct snd_opl3sa2 *chip;
 	int i;
 
+	if (!card)
+		return 0;
+
+	chip = card->private_data;
 	/* power up */
 	snd_opl3sa2_write(chip, OPL3SA2_PM_CTRL, OPL3SA2_PM_D0);
 

Re: [PATCH] Fix NULL dereference when suspending snd_opl3sa2

[Takashi Iwai]

At Mon, 16 Mar 2009 21:32:25 +0100,
Krzysztof Helt wrote:
> 
> On Mon, 16 Mar 2009 15:16:08 +0100
> Lubomir Rintel <lkundrak@v3.sk> wrote:
> 
> > On Mon, 2009-03-16 at 11:01 +0100, Lubomir Rintel wrote:
> > > This should fix the following OOPS:
> > > http://www.kerneloops.org/raw.php?rawid=80591&msgid=
> > 
> 
> > This is the same issue, with 2.6.28:
> > http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=519939
> > 
> > 
> 
> One should no force loading of the snd-opl3sa2 driver
> in the above cases. It was forced as the opl3sa2 is a PnP chip
> and the drivers were loaded as plain ISA devices.
> 
> However, this is not a cause of the OOPS. The cause is that
> despite the driver returns -ENODEV, the higher layer
> hides it and the driver is bound to the bus. I suppose,
> the suspend and resume functions are called on it.
> 
> Look at the ISA probe function:
> 
> static int __devinit snd_opl3sa2_isa_probe(struct device *pdev,
>                        unsigned int dev)
> {
>     struct snd_card *card;
>     int err;
> 
>     card = snd_opl3sa2_card_new(dev);
>     if (! card)
>         return -ENOMEM;
>     snd_card_set_dev(card, pdev);
>     if ((err = snd_opl3sa2_probe(card, dev)) < 0) {
>         snd_card_free(card);
>         return err;
>     }
>     dev_set_drvdata(pdev, card);
>     return 0;
> }
> 
> If the probe function fails, the drvdata is
> never set to the snd_card structure pointer.
> 
> The patch below should fix suspend and resume
> functions.
> 
> Regards,
> Krzysztof
> PS. It is the same situation for PnP and
> BIOS PnP probing in this driver.
> ---
> 
> Fix the OOPS during a opl3sa2 card suspend
> and resume if the driver is loaded but the card
> is not found.
> 
> Signed-off-by: Krzysztof Helt <krzysztof.h1@wp.pl>

This patch looks good.  I applied it to sound git tree now.

Thanks,

Takashi


> --- a/sound/isa/opl3sa2.c~	2008-10-13 18:03:52.000000000 +0200
> +++ b/sound/isa/opl3sa2.c	2009-03-16 21:20:30.841348261 +0100
> @@ -550,21 +550,27 @@ static int __devinit snd_opl3sa2_mixer(s
>  #ifdef CONFIG_PM
>  static int snd_opl3sa2_suspend(struct snd_card *card, pm_message_t state)
>  {
> -	struct snd_opl3sa2 *chip = card->private_data;
> +	if (card) {
> +		struct snd_opl3sa2 *chip = card->private_data;
>  
> -	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
> -	chip->wss->suspend(chip->wss);
> -	/* power down */
> -	snd_opl3sa2_write(chip, OPL3SA2_PM_CTRL, OPL3SA2_PM_D3);
> +		snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
> +		chip->wss->suspend(chip->wss);
> +		/* power down */
> +		snd_opl3sa2_write(chip, OPL3SA2_PM_CTRL, OPL3SA2_PM_D3);
> +	}
>  
>  	return 0;
>  }
>  
>  static int snd_opl3sa2_resume(struct snd_card *card)
>  {
> -	struct snd_opl3sa2 *chip = card->private_data;
> +	struct snd_opl3sa2 *chip;
>  	int i;
>  
> +	if (!card)
> +		return 0;
> +
> +	chip = card->private_data;
>  	/* power up */
>  	snd_opl3sa2_write(chip, OPL3SA2_PM_CTRL, OPL3SA2_PM_D0);
>  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>