Re: [PATCH v4] ASoC: Intel: Skylake: Add DSP firmware manifest parsing

[Vinod Koul <vinod.koul@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 1393 bytes --]

On Fri, May 13, 2016 at 01:14:17PM +0100, Mark Brown wrote:
> On Fri, May 13, 2016 at 05:25:47PM +0530, Vinod Koul wrote:
> 
> > +	/* Get the FW pointer to derive ADSP header */
> > +	buf = ctx->fw->data;
> 
> > +	adsp_hdr = (struct adsp_fw_hdr *)(buf + SKL_ADSP_FW_BIN_HDR_OFFSET);
> 
> > +	mod_entry = (struct adsp_module_entry *)
> > +		(buf + SKL_ADSP_FW_BIN_HDR_OFFSET + adsp_hdr->header_len);
> 
> What if we somehow managed to end up with a zero length firmware (or
> something smaller than these headers)?

I recall that firmware core checks if file is zero sized. But then it won't
help us here if the size is less than SKL_ADSP_FW_BIN_HDR_OFFSET, so will
add proper checks in this case as well

> 
> > +		/*
> > +		 * we check if current pointer is larger than file size from
> > +		 * base value to check excceding the file while parsing
> > +		 */
> > +		if ((const char *)mod_entry >= buf + ctx->fw->size) {
> > +			dev_err(ctx->dev,
> > +				"Exceeds file bound: Entry %d Ptr %p\n",
> > +				i, mod_entry);
> > +
> > +			return -EIO;
> > +		}
> 
> This checks the start of the entry but it still lets us read beyond the
> end of the file.

Yup we need to check before incrementing, will fix that

Btw wanted to ask you on the update for this. Do you prefer that is sent
after merge window or anytime is fine by you :)

Thanks
-- 
~Vinod

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]