Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sun, 16 May 2021 14:07:23 +0200
Message-ID: <s5h5yzi7uh0.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Sergey Senozhatsky <senozhatsky@chromium.org>
Cc:     Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>,
        "Gustavo A. R. Silva" <gustavoars@kernel.org>,
        Leon Romanovsky <leon@kernel.org>, alsa-devel@alsa-project.org,
        linux-kernel@vger.kernel.org
Subject: Re: ALSA: intel8x0: div by zero in snd_intel8x0_update()
In-Reply-To: <YKEAqdIAZ5K5FS+1@google.com>
References: <YJ4yBmIV6RJCo42U@google.com>
        <s5hk0o18tio.wl-tiwai@suse.de>
        <YJ5cHdv6MVmAKD3b@google.com>
        <YKDYQfDf7GiMfGCN@google.com>
        <YKDYbaprE3K2QpCe@google.com>
        <s5hbl9b6mah.wl-tiwai@suse.de>
        <YKEAqdIAZ5K5FS+1@google.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka)
 FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3
 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Sun, 16 May 2021 13:23:21 +0200,
Sergey Senozhatsky wrote:
> 
> On (21/05/16 11:49), Takashi Iwai wrote:
> > 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.
> 
> VM, yes. I didn't see NULL derefs, my VMs crash because of div by
> zero in `% size`.

Ah, right, I'll fix the description.

> > 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.
> 
> I reproduced the "spurious IRQ" case, and the patch handled it correctly
> (VM did not crash).
> 
> > Cc: <stable@vger.kernel.org>
> > Reported-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> 
> I'll keep running test, but seems that it works as intended
> 
> Tested-by: Sergey Senozhatsky <senozhatsky@chromium.org>

OK, below is the revised patch I'm going to apply.


Thanks!

Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH v2] 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 zero-
division 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-and-tested-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
v1->v2: fixed description, updated tested-by tag

 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