2009-03-16 10:02:11

by Lubomir Rintel

[permalink] [raw]
Subject: [PATCH] Fix NULL dereference when suspending snd_opl3sa2

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

Signed-off-by: Lubomir Rintel <[email protected]>
---
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



2009-03-16 14:16:24

by Lubomir Rintel

[permalink] [raw]
Subject: Re: [PATCH] Fix NULL dereference when suspending snd_opl3sa2

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

2009-03-16 18:15:49

by Krzysztof Helt

[permalink] [raw]
Subject: Re: [PATCH] Fix NULL dereference when suspending snd_opl3sa2

On Mon, 16 Mar 2009 15:16:08 +0100
Lubomir Rintel <[email protected]> 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

2009-03-16 20:26:21

by Krzysztof Helt

[permalink] [raw]
Subject: Re: [PATCH] Fix NULL dereference when suspending snd_opl3sa2

On Mon, 16 Mar 2009 15:16:08 +0100
Lubomir Rintel <[email protected]> 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 <[email protected]>

--- 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);

2009-03-17 08:30:48

by Takashi Iwai

[permalink] [raw]
Subject: Re: [PATCH] Fix NULL dereference when suspending snd_opl3sa2

At Mon, 16 Mar 2009 21:32:25 +0100,
Krzysztof Helt wrote:
>
> On Mon, 16 Mar 2009 15:16:08 +0100
> Lubomir Rintel <[email protected]> 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 <[email protected]>

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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>