[PATCH] ASoC: max98390: Fix potential crash during param fw loading

[PATCH] ASoC: max98390: Fix potential crash during param fw loading

[Steve Lee]

 malformed firmware file can cause out-of-bound access and crash
 during dsm_param bin loading.
  - add MIN/MAX param size to avoid out-of-bound access.
  - read start addr and size of param and check bound.

Signed-off-by: Steve Lee <steves.lee@maximintegrated.com>
---
 sound/soc/codecs/max98390.c | 23 ++++++++++++++++++-----
 sound/soc/codecs/max98390.h |  3 ++-
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/sound/soc/codecs/max98390.c b/sound/soc/codecs/max98390.c
index be7cd0aeb6a6..8c7db568fe64 100644
--- a/sound/soc/codecs/max98390.c
+++ b/sound/soc/codecs/max98390.c
@@ -754,6 +754,7 @@ static struct snd_soc_dai_driver max98390_dai[] = {
 static int max98390_dsm_init(struct snd_soc_component *component)
 {
 	int ret;
+	int param_size, param_start_addr;
 	char filename[128];
 	const char *vendor, *product;
 	struct max98390_priv *max98390 =
@@ -780,14 +781,27 @@ static int max98390_dsm_init(struct snd_soc_component *component)
 	dev_dbg(component->dev,
 		"max98390: param fw size %zd\n",
 		fw->size);
+	if (fw->size < MAX98390_DSM_PARAM_MIN_SIZE) {
+		dev_err(component->dev,
+			"param fw is invalid.\n");
+		goto err_alloc;
+	}
 	dsm_param = (char *)fw->data;
+	param_start_addr = (dsm_param[0] & 0xff) | (dsm_param[1] & 0xff) << 8;
+	param_size = (dsm_param[2] & 0xff) | (dsm_param[3] & 0xff) << 8;
+	if (param_size > MAX98390_DSM_PARAM_MAX_SIZE ||
+		param_start_addr < DSM_STBASS_HPF_B0_BYTE0) {
+		dev_err(component->dev,
+			"param fw is invalid.\n");
+		goto err_alloc;
+	}
 	dsm_param += MAX98390_DSM_PAYLOAD_OFFSET;
-	regmap_bulk_write(max98390->regmap, DSM_EQ_BQ1_B0_BYTE0,
-		dsm_param,
-		fw->size - MAX98390_DSM_PAYLOAD_OFFSET);
-	release_firmware(fw);
+	regmap_bulk_write(max98390->regmap, param_start_addr,
+		dsm_param, param_size);
 	regmap_write(max98390->regmap, MAX98390_R23E1_DSP_GLOBAL_EN, 0x01);
 
+err_alloc:
+	release_firmware(fw);
 err:
 	return ret;
 }
@@ -847,7 +861,6 @@ static int max98390_probe(struct snd_soc_component *component)
 
 	/* Dsm Setting */
 	regmap_write(max98390->regmap, DSM_VOL_CTRL, 0x94);
-	regmap_write(max98390->regmap, DSMIG_EN, 0x19);
 	regmap_write(max98390->regmap, MAX98390_R203A_AMP_EN, 0x80);
 	if (max98390->ref_rdc_value) {
 		regmap_write(max98390->regmap, DSM_TPROT_RECIP_RDC_ROOM_BYTE0,
diff --git a/sound/soc/codecs/max98390.h b/sound/soc/codecs/max98390.h
index f59cb114d957..5f444e7779b0 100644
--- a/sound/soc/codecs/max98390.h
+++ b/sound/soc/codecs/max98390.h
@@ -650,7 +650,8 @@
 
 /* DSM register offset */
 #define MAX98390_DSM_PAYLOAD_OFFSET 16
-#define MAX98390_DSM_PAYLOAD_OFFSET_2 495
+#define MAX98390_DSM_PARAM_MAX_SIZE 770
+#define MAX98390_DSM_PARAM_MIN_SIZE 670
 
 struct max98390_priv {
 	struct regmap *regmap;
-- 
2.17.1

Re: [PATCH] ASoC: max98390: Fix potential crash during param fw loading

[Takashi Iwai]

On Wed, 03 Jun 2020 13:18:19 +0200,
Steve Lee wrote:
> 
> @@ -847,7 +861,6 @@ static int max98390_probe(struct snd_soc_component *component)
>  
>  	/* Dsm Setting */
>  	regmap_write(max98390->regmap, DSM_VOL_CTRL, 0x94);
> -	regmap_write(max98390->regmap, DSMIG_EN, 0x19);

Is this change intentional?
It wasn't mentioned in the patch description.


thanks,

Takashi

RE: [PATCH] ASoC: max98390: Fix potential crash during param fw loading

[Steve Lee]

> -----Original Message-----
> From: Takashi Iwai <tiwai@suse.de>
> Sent: Wednesday, June 3, 2020 8:24 PM
> To: Steve Lee <SteveS.Lee@maximintegrated.com>
> Cc: lgirdwood@gmail.com; broonie@kernel.org; perex@perex.cz;
> tiwai@suse.com; ckeepax@opensource.cirrus.com; geert@linux-m68k.org;
> rf@opensource.wolfsonmicro.com; shumingf@realtek.com;
> srinivas.kandagatla@linaro.org; krzk@kernel.org; dmurphy@ti.com;
> jack.yu@realtek.com; nuno.sa@analog.com; linux-kernel@vger.kernel.org;
> alsa-devel@alsa-project.org; ryan.lee.maxim@gmail.com; Ryan Lee
> <RyanS.Lee@maximintegrated.com>; steves.lee.maxim@gmail.com
> Subject: Re: [PATCH] ASoC: max98390: Fix potential crash during param fw
> loading
> 
> EXTERNAL EMAIL
> 
> 
> 
> On Wed, 03 Jun 2020 13:18:19 +0200,
> Steve Lee wrote:
> >
> > @@ -847,7 +861,6 @@ static int max98390_probe(struct snd_soc_component
> *component)
> >
> >       /* Dsm Setting */
> >       regmap_write(max98390->regmap, DSM_VOL_CTRL, 0x94);
> > -     regmap_write(max98390->regmap, DSMIG_EN, 0x19);
> 
> Is this change intentional?
> It wasn't mentioned in the patch description.

 It should be another change. I will re-send the patch.

> 
> 
> thanks,
> 
> Takashi

Re: [PATCH] ASoC: max98390: Fix potential crash during param fw loading

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 746 bytes --]

On Wed, Jun 03, 2020 at 08:18:19PM +0900, Steve Lee wrote:

> +	param_start_addr = (dsm_param[0] & 0xff) | (dsm_param[1] & 0xff) << 8;
> +	param_size = (dsm_param[2] & 0xff) | (dsm_param[3] & 0xff) << 8;
> +	if (param_size > MAX98390_DSM_PARAM_MAX_SIZE ||
> +		param_start_addr < DSM_STBASS_HPF_B0_BYTE0) {
> +		dev_err(component->dev,
> +			"param fw is invalid.\n");
> +		goto err_alloc;
> +	}

This is now reading the size out of the header of the file which is good
but it should also validate that the file is big enough to have this
much data in it, otherwise it's possible to read beyond the end of the
firmware file (eg, if it got truncated somehow).  Previously the code
used the size of the file read from disk so that wasn't an issue.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH] ASoC: max98390: Fix potential crash during param fw loading

[Steve Lee]

> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: Wednesday, June 3, 2020 8:32 PM
> To: Steve Lee <SteveS.Lee@maximintegrated.com>
> Cc: lgirdwood@gmail.com; perex@perex.cz; tiwai@suse.com;
> ckeepax@opensource.cirrus.com; geert@linux-m68k.org;
> rf@opensource.wolfsonmicro.com; shumingf@realtek.com;
> srinivas.kandagatla@linaro.org; krzk@kernel.org; dmurphy@ti.com;
> jack.yu@realtek.com; nuno.sa@analog.com; linux-kernel@vger.kernel.org;
> alsa-devel@alsa-project.org; ryan.lee.maxim@gmail.com; Ryan Lee
> <RyanS.Lee@maximintegrated.com>; steves.lee.maxim@gmail.com
> Subject: Re: [PATCH] ASoC: max98390: Fix potential crash during param fw
> loading
> 
> On Wed, Jun 03, 2020 at 08:18:19PM +0900, Steve Lee wrote:
> 
> > +	param_start_addr = (dsm_param[0] & 0xff) | (dsm_param[1] & 0xff) <<
> 8;
> > +	param_size = (dsm_param[2] & 0xff) | (dsm_param[3] & 0xff) << 8;
> > +	if (param_size > MAX98390_DSM_PARAM_MAX_SIZE ||
> > +		param_start_addr < DSM_STBASS_HPF_B0_BYTE0) {
> > +		dev_err(component->dev,
> > +			"param fw is invalid.\n");
> > +		goto err_alloc;
> > +	}
> 
> This is now reading the size out of the header of the file which is good but it
> should also validate that the file is big enough to have this much data in it,
> otherwise it's possible to read beyond the end of the firmware file (eg, if it got
> truncated somehow).  Previously the code used the size of the file read from disk
> so that wasn't an issue.

 Thanks for quick comment. Can this case cover by below line?
+	if (fw->size < MAX98390_DSM_PARAM_MIN_SIZE) {
+		dev_err(component->dev,
+			"param fw is invalid.\n");
+		goto err_alloc;
+	}
 

Re: [PATCH] ASoC: max98390: Fix potential crash during param fw loading

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 866 bytes --]

On Wed, Jun 03, 2020 at 11:37:44AM +0000, Steve Lee wrote:

> > This is now reading the size out of the header of the file which is good but it
> > should also validate that the file is big enough to have this much data in it,
> > otherwise it's possible to read beyond the end of the firmware file (eg, if it got
> > truncated somehow).  Previously the code used the size of the file read from disk
> > so that wasn't an issue.

>  Thanks for quick comment. Can this case cover by below line?
> +	if (fw->size < MAX98390_DSM_PARAM_MIN_SIZE) {
> +		dev_err(component->dev,
> +			"param fw is invalid.\n");
> +		goto err_alloc;
> +	}

No, that doesn't cover all of it - the case I'm concerned about is the
case where we've got enough data for the header but the payload is
truncated.  You need a check that param_size + _PAYLOAD_OFFSET is less
than fw->size as well.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 484 bytes --]

RE: [PATCH] ASoC: max98390: Fix potential crash during param fw loading

[Steve Lee]

> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: Wednesday, June 3, 2020 8:43 PM
> To: Steve Lee <SteveS.Lee@maximintegrated.com>
> Cc: lgirdwood@gmail.com; perex@perex.cz; tiwai@suse.com;
> ckeepax@opensource.cirrus.com; geert@linux-m68k.org;
> rf@opensource.wolfsonmicro.com; shumingf@realtek.com;
> srinivas.kandagatla@linaro.org; krzk@kernel.org; dmurphy@ti.com;
> jack.yu@realtek.com; nuno.sa@analog.com; linux-kernel@vger.kernel.org;
> alsa-devel@alsa-project.org; ryan.lee.maxim@gmail.com; Ryan Lee
> <RyanS.Lee@maximintegrated.com>; steves.lee.maxim@gmail.com
> Subject: Re: [PATCH] ASoC: max98390: Fix potential crash during param fw
> loading
> 
> On Wed, Jun 03, 2020 at 11:37:44AM +0000, Steve Lee wrote:
> 
> > > This is now reading the size out of the header of the file which is
> > > good but it should also validate that the file is big enough to have
> > > this much data in it, otherwise it's possible to read beyond the end
> > > of the firmware file (eg, if it got truncated somehow).  Previously
> > > the code used the size of the file read from disk so that wasn't an issue.
> 
> >  Thanks for quick comment. Can this case cover by below line?
> > +	if (fw->size < MAX98390_DSM_PARAM_MIN_SIZE) {
> > +		dev_err(component->dev,
> > +			"param fw is invalid.\n");
> > +		goto err_alloc;
> > +	}
> 
> No, that doesn't cover all of it - the case I'm concerned about is the case where
> we've got enough data for the header but the payload is truncated.  You need a
> check that param_size + _PAYLOAD_OFFSET is less than fw->size as well.

  Yes, I will update this and good enough.