RE: [PATCH] ASoC: max98390: Fix potential crash during param fw loading
> -Original Message-
> From: Mark Brown
> Sent: Wednesday, June 3, 2020 8:43 PM
> To: Steve Lee
> Cc: lgirdw...@gmail.com; pe...@perex.cz; ti...@suse.com;
> ckee...@opensource.cirrus.com; ge...@linux-m68k.org;
> r...@opensource.wolfsonmicro.com; shumi...@realtek.com;
> srinivas.kandaga...@linaro.org; k...@kernel.org; dmur...@ti.com;
> jack...@realtek.com; nuno...@analog.com; linux-kernel@vger.kernel.org;
> alsa-de...@alsa-project.org; ryan.lee.ma...@gmail.com; Ryan Lee
> ; steves.lee.ma...@gmail.com
> Subject: Re: [PATCH] ASoC: max98390: Fix potential crash during param fw
> loading
>
> On Wed, Jun 03, 2020 at 11:37:44AM +, 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.
Re: [PATCH] ASoC: max98390: Fix potential crash during param fw loading
On Wed, Jun 03, 2020 at 11:37:44AM +, 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.
signature.asc
Description: PGP signature
RE: [PATCH] ASoC: max98390: Fix potential crash during param fw loading
> -Original Message-
> From: Mark Brown
> Sent: Wednesday, June 3, 2020 8:32 PM
> To: Steve Lee
> Cc: lgirdw...@gmail.com; pe...@perex.cz; ti...@suse.com;
> ckee...@opensource.cirrus.com; ge...@linux-m68k.org;
> r...@opensource.wolfsonmicro.com; shumi...@realtek.com;
> srinivas.kandaga...@linaro.org; k...@kernel.org; dmur...@ti.com;
> jack...@realtek.com; nuno...@analog.com; linux-kernel@vger.kernel.org;
> alsa-de...@alsa-project.org; ryan.lee.ma...@gmail.com; Ryan Lee
> ; steves.lee.ma...@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
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.
signature.asc
Description: PGP signature
RE: [PATCH] ASoC: max98390: Fix potential crash during param fw loading
> -Original Message- > From: Takashi Iwai > Sent: Wednesday, June 3, 2020 8:24 PM > To: Steve Lee > Cc: lgirdw...@gmail.com; broo...@kernel.org; pe...@perex.cz; > ti...@suse.com; ckee...@opensource.cirrus.com; ge...@linux-m68k.org; > r...@opensource.wolfsonmicro.com; shumi...@realtek.com; > srinivas.kandaga...@linaro.org; k...@kernel.org; dmur...@ti.com; > jack...@realtek.com; nuno...@analog.com; linux-kernel@vger.kernel.org; > alsa-de...@alsa-project.org; ryan.lee.ma...@gmail.com; Ryan Lee > ; steves.lee.ma...@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
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
