From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934138AbdKBQlX (ORCPT ); Thu, 2 Nov 2017 12:41:23 -0400 Received: from mx07-00178001.pphosted.com ([62.209.51.94]:15955 "EHLO mx07-00178001.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933283AbdKBQlV (ORCPT ); Thu, 2 Nov 2017 12:41:21 -0400 From: Olivier MOYSAN To: Takashi Iwai CC: "alsa-devel@alsa-project.org" , "mark.rutland@arm.com" , "lgirdwood@gmail.com" , "mcoquelin.stm32@gmail.com" , "broonie@kernel.org" , "robh@kernel.org" , "linux-arm-kernel@lists.infradead.org" , "perex@perex.cz" , Alexandre TORGUE , "kernel@stlinux.com" , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Arnaud POULIQUEN , Benjamin GAIGNARD Subject: Re: [INTERNAL][PATCH 4/7] ASoC: stm32: sai: fix stop management in isr Thread-Topic: [INTERNAL][PATCH 4/7] ASoC: stm32: sai: fix stop management in isr Thread-Index: AQHTTm+ZKbfeVt7gKk2ZMZD+PGCOPqMBRGIA Date: Thu, 2 Nov 2017 16:38:36 +0000 Message-ID: References: <1508418203-16840-1-git-send-email-olivier.moysan@st.com> <1508418203-16840-5-git-send-email-olivier.moysan@st.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: user-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 x-ms-exchange-messagesentrepresentingtype: 1 x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.75.127.45] Content-Type: text/plain; charset="utf-8" Content-ID: <172B85CB8331A3469F89F22D57732EF8@st.com> MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-11-02_05:,, signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by nfs id vA2GfSsb015813 Hello Takashi, Sorry for late answer. I was OoO. Ok, I will add a protection on sai->substream accesses. Best regards Olivier On 10/26/2017 05:32 PM, Takashi Iwai wrote: > On Thu, 19 Oct 2017 15:03:20 +0200, > Olivier Moysan wrote: >> >> Add check on substream validity. >> >> Signed-off-by: Olivier Moysan >> --- >> sound/soc/stm/stm32_sai_sub.c | 12 ++++++++---- >> 1 file changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/sound/soc/stm/stm32_sai_sub.c b/sound/soc/stm/stm32_sai_sub.c >> index 2af397d..815ef10 100644 >> --- a/sound/soc/stm/stm32_sai_sub.c >> +++ b/sound/soc/stm/stm32_sai_sub.c >> @@ -184,7 +184,6 @@ static bool stm32_sai_sub_writeable_reg(struct device *dev, unsigned int reg) >> static irqreturn_t stm32_sai_isr(int irq, void *devid) >> { >> struct stm32_sai_sub_data *sai = (struct stm32_sai_sub_data *)devid; >> - struct snd_pcm_substream *substream = sai->substream; >> struct platform_device *pdev = sai->pdev; >> unsigned int sr, imr, flags; >> snd_pcm_state_t status = SNDRV_PCM_STATE_RUNNING; >> @@ -199,6 +198,11 @@ static irqreturn_t stm32_sai_isr(int irq, void *devid) >> regmap_update_bits(sai->regmap, STM_SAI_CLRFR_REGX, SAI_XCLRFR_MASK, >> SAI_XCLRFR_MASK); >> >> + if (!sai->substream) { >> + dev_err(&pdev->dev, "Device stopped. Spurious IRQ 0x%x\n", sr); >> + return IRQ_NONE; >> + } >> + >> if (flags & SAI_XIMR_OVRUDRIE) { >> dev_err(&pdev->dev, "IRQ %s\n", >> STM_SAI_IS_PLAYBACK(sai) ? "underrun" : "overrun"); >> @@ -227,9 +231,9 @@ static irqreturn_t stm32_sai_isr(int irq, void *devid) >> } >> >> if (status != SNDRV_PCM_STATE_RUNNING) { >> - snd_pcm_stream_lock(substream); >> - snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN); >> - snd_pcm_stream_unlock(substream); >> + snd_pcm_stream_lock(sai->substream); >> + snd_pcm_stop(sai->substream, SNDRV_PCM_STATE_XRUN); >> + snd_pcm_stream_unlock(sai->substream); > > Actually changing to sai->substream opens a race, so this chunk is a > bad move, at least. We have no protection of sai->substream in this > context, thus it can hit a NULL dereference... > > > thanks, > > Takashi > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Olivier MOYSAN Subject: Re: [INTERNAL][PATCH 4/7] ASoC: stm32: sai: fix stop management in isr Date: Thu, 2 Nov 2017 16:38:36 +0000 Message-ID: References: <1508418203-16840-1-git-send-email-olivier.moysan@st.com> <1508418203-16840-5-git-send-email-olivier.moysan@st.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Content-Language: en-US Content-ID: <172B85CB8331A3469F89F22D57732EF8@st.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Takashi Iwai Cc: "mark.rutland@arm.com" , "robh@kernel.org" , "alsa-devel@alsa-project.org" , Alexandre TORGUE , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Arnaud POULIQUEN , "lgirdwood@gmail.com" , "broonie@kernel.org" , "mcoquelin.stm32@gmail.com" , Benjamin GAIGNARD , "linux-arm-kernel@lists.infradead.org" , "kernel@stlinux.com" List-Id: devicetree@vger.kernel.org Hello Takashi, Sorry for late answer. I was OoO. Ok, I will add a protection on sai->substream accesses. Best regards Olivier On 10/26/2017 05:32 PM, Takashi Iwai wrote: > On Thu, 19 Oct 2017 15:03:20 +0200, > Olivier Moysan wrote: >> >> Add check on substream validity. >> >> Signed-off-by: Olivier Moysan >> --- >> sound/soc/stm/stm32_sai_sub.c | 12 ++++++++---- >> 1 file changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/sound/soc/stm/stm32_sai_sub.c b/sound/soc/stm/stm32_sai_sub.c >> index 2af397d..815ef10 100644 >> --- a/sound/soc/stm/stm32_sai_sub.c >> +++ b/sound/soc/stm/stm32_sai_sub.c >> @@ -184,7 +184,6 @@ static bool stm32_sai_sub_writeable_reg(struct device *dev, unsigned int reg) >> static irqreturn_t stm32_sai_isr(int irq, void *devid) >> { >> struct stm32_sai_sub_data *sai = (struct stm32_sai_sub_data *)devid; >> - struct snd_pcm_substream *substream = sai->substream; >> struct platform_device *pdev = sai->pdev; >> unsigned int sr, imr, flags; >> snd_pcm_state_t status = SNDRV_PCM_STATE_RUNNING; >> @@ -199,6 +198,11 @@ static irqreturn_t stm32_sai_isr(int irq, void *devid) >> regmap_update_bits(sai->regmap, STM_SAI_CLRFR_REGX, SAI_XCLRFR_MASK, >> SAI_XCLRFR_MASK); >> >> + if (!sai->substream) { >> + dev_err(&pdev->dev, "Device stopped. Spurious IRQ 0x%x\n", sr); >> + return IRQ_NONE; >> + } >> + >> if (flags & SAI_XIMR_OVRUDRIE) { >> dev_err(&pdev->dev, "IRQ %s\n", >> STM_SAI_IS_PLAYBACK(sai) ? "underrun" : "overrun"); >> @@ -227,9 +231,9 @@ static irqreturn_t stm32_sai_isr(int irq, void *devid) >> } >> >> if (status != SNDRV_PCM_STATE_RUNNING) { >> - snd_pcm_stream_lock(substream); >> - snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN); >> - snd_pcm_stream_unlock(substream); >> + snd_pcm_stream_lock(sai->substream); >> + snd_pcm_stop(sai->substream, SNDRV_PCM_STATE_XRUN); >> + snd_pcm_stream_unlock(sai->substream); > > Actually changing to sai->substream opens a race, so this chunk is a > bad move, at least. We have no protection of sai->substream in this > context, thus it can hit a NULL dereference... > > > thanks, > > Takashi > From mboxrd@z Thu Jan 1 00:00:00 1970 From: olivier.moysan@st.com (Olivier MOYSAN) Date: Thu, 2 Nov 2017 16:38:36 +0000 Subject: [INTERNAL][PATCH 4/7] ASoC: stm32: sai: fix stop management in isr In-Reply-To: References: <1508418203-16840-1-git-send-email-olivier.moysan@st.com> <1508418203-16840-5-git-send-email-olivier.moysan@st.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hello Takashi, Sorry for late answer. I was OoO. Ok, I will add a protection on sai->substream accesses. Best regards Olivier On 10/26/2017 05:32 PM, Takashi Iwai wrote: > On Thu, 19 Oct 2017 15:03:20 +0200, > Olivier Moysan wrote: >> >> Add check on substream validity. >> >> Signed-off-by: Olivier Moysan >> --- >> sound/soc/stm/stm32_sai_sub.c | 12 ++++++++---- >> 1 file changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/sound/soc/stm/stm32_sai_sub.c b/sound/soc/stm/stm32_sai_sub.c >> index 2af397d..815ef10 100644 >> --- a/sound/soc/stm/stm32_sai_sub.c >> +++ b/sound/soc/stm/stm32_sai_sub.c >> @@ -184,7 +184,6 @@ static bool stm32_sai_sub_writeable_reg(struct device *dev, unsigned int reg) >> static irqreturn_t stm32_sai_isr(int irq, void *devid) >> { >> struct stm32_sai_sub_data *sai = (struct stm32_sai_sub_data *)devid; >> - struct snd_pcm_substream *substream = sai->substream; >> struct platform_device *pdev = sai->pdev; >> unsigned int sr, imr, flags; >> snd_pcm_state_t status = SNDRV_PCM_STATE_RUNNING; >> @@ -199,6 +198,11 @@ static irqreturn_t stm32_sai_isr(int irq, void *devid) >> regmap_update_bits(sai->regmap, STM_SAI_CLRFR_REGX, SAI_XCLRFR_MASK, >> SAI_XCLRFR_MASK); >> >> + if (!sai->substream) { >> + dev_err(&pdev->dev, "Device stopped. Spurious IRQ 0x%x\n", sr); >> + return IRQ_NONE; >> + } >> + >> if (flags & SAI_XIMR_OVRUDRIE) { >> dev_err(&pdev->dev, "IRQ %s\n", >> STM_SAI_IS_PLAYBACK(sai) ? "underrun" : "overrun"); >> @@ -227,9 +231,9 @@ static irqreturn_t stm32_sai_isr(int irq, void *devid) >> } >> >> if (status != SNDRV_PCM_STATE_RUNNING) { >> - snd_pcm_stream_lock(substream); >> - snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN); >> - snd_pcm_stream_unlock(substream); >> + snd_pcm_stream_lock(sai->substream); >> + snd_pcm_stop(sai->substream, SNDRV_PCM_STATE_XRUN); >> + snd_pcm_stream_unlock(sai->substream); > > Actually changing to sai->substream opens a race, so this chunk is a > bad move, at least. We have no protection of sai->substream in this > context, thus it can hit a NULL dereference... > > > thanks, > > Takashi >