Re: ALSA: intel8x0: div by zero in snd_intel8x0_update()

[Takashi Iwai <tiwai@suse.de>]

On Sun, 16 May 2021 10:31:41 +0200,
Sergey Senozhatsky wrote:
> 
> On (21/05/16 17:30), Sergey Senozhatsky wrote:
> > On (21/05/14 20:16), Sergey Senozhatsky wrote:
> > > > --- a/sound/pci/intel8x0.c
> > > > +++ b/sound/pci/intel8x0.c
> > > > @@ -691,6 +691,9 @@ static inline void snd_intel8x0_update(struct intel8x0 *chip, struct ichdev *ich
> > > >  	int status, civ, i, step;
> > > >  	int ack = 0;
> > > >  
> > > > +	if (!ichdev->substream || ichdev->suspended)
> > > > +		return;
> > > > +
> > > >  	spin_lock_irqsave(&chip->reg_lock, flags);
> > > >  	status = igetbyte(chip, port + ichdev->roff_sr);
> > > >  	civ = igetbyte(chip, port + ICH_REG_OFF_CIV);
> > 
> > This does the problem for me.
> 
>        ^^^ does fix

OK, thanks for confirmation.  So this looks like some spurious
interrupt with the unexpected hardware bits.

However, the suggested check doesn't seem covering enough, and it
might still hit if the suspend/resume happens before the device is
opened but not set up (and such a spurious irq is triggered).

Below is more comprehensive fix.  Let me know if this works, too.


thanks,

Takashi

-- 8< --
Subject: [PATCH] ALSA: intel8x0: Don't update period unless prepared

The interrupt handler of intel8x0 calls snd_intel8x0_update() whenever
the hardware sets the corresponding status bit for each stream.  This
works fine for most cases as long as the hardware behaves properly.
But when the hardware gives a wrong bit set, this leads to a NULL
dereference Oops, and reportedly, this seems what happened on a VM.

For fixing the crash, this patch adds a internal flag indicating that
the stream is ready to be updated, and check it (as well as the flag
being in suspended) to ignore such spurious update.

Cc: <stable@vger.kernel.org>
Reported-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/intel8x0.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
index 35903d1a1cbd..5b124c4ad572 100644
--- a/sound/pci/intel8x0.c
+++ b/sound/pci/intel8x0.c
@@ -331,6 +331,7 @@ struct ichdev {
 	unsigned int ali_slot;			/* ALI DMA slot */
 	struct ac97_pcm *pcm;
 	int pcm_open_flag;
+	unsigned int prepared:1;
 	unsigned int suspended: 1;
 };
 
@@ -691,6 +692,9 @@ static inline void snd_intel8x0_update(struct intel8x0 *chip, struct ichdev *ich
 	int status, civ, i, step;
 	int ack = 0;
 
+	if (!ichdev->prepared || ichdev->suspended)
+		return;
+
 	spin_lock_irqsave(&chip->reg_lock, flags);
 	status = igetbyte(chip, port + ichdev->roff_sr);
 	civ = igetbyte(chip, port + ICH_REG_OFF_CIV);
@@ -881,6 +885,7 @@ static int snd_intel8x0_hw_params(struct snd_pcm_substream *substream,
 	if (ichdev->pcm_open_flag) {
 		snd_ac97_pcm_close(ichdev->pcm);
 		ichdev->pcm_open_flag = 0;
+		ichdev->prepared = 0;
 	}
 	err = snd_ac97_pcm_open(ichdev->pcm, params_rate(hw_params),
 				params_channels(hw_params),
@@ -902,6 +907,7 @@ static int snd_intel8x0_hw_free(struct snd_pcm_substream *substream)
 	if (ichdev->pcm_open_flag) {
 		snd_ac97_pcm_close(ichdev->pcm);
 		ichdev->pcm_open_flag = 0;
+		ichdev->prepared = 0;
 	}
 	return 0;
 }
@@ -976,6 +982,7 @@ static int snd_intel8x0_pcm_prepare(struct snd_pcm_substream *substream)
 			ichdev->pos_shift = (runtime->sample_bits > 16) ? 2 : 1;
 	}
 	snd_intel8x0_setup_periods(chip, ichdev);
+	ichdev->prepared = 1;
 	return 0;
 }
 
-- 
2.26.2