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

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

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

On Tue, Apr 12, 2016 at 05:12:07AM +0100, Mark Brown wrote:
> On Sun, Apr 03, 2016 at 03:19:14AM +0530, Vinod Koul wrote:
> 
> > +	for (i = 0; i < num; i++) {
> > +		if (!strcmp(uuid, tbl[i].uuid)) {
> 
> Do we really want to be working with UUIDs in string format all the
> time?  It seems error prone and as far as I can see at least some of the
> time we get them in in binary format.

I agree that it would be saner to handle uuid in bin format and print to
strings when string are required.

> > +	sprintf(buf, "%08X", entry->uuid.id_1);
> > +	sprintf(buf+strlen(buf), "-");
> 
> Coding style, spaces around +.  I'd also like to be a bit more confident
> that this can't result in a buffer overflow, sprintf() isn't reassuring
> especially in conjunction with buf being passed in from elsewhere with
> no length information.

Okay I think we cna remove this if don't use strings

> > +	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);
> > +
> > +	num_entry = adsp_hdr->num_module_entries;
> 
> > +	for (i = 0; i < num_entry; i++, mod_entry++) {
> > +		get_canonical_uuid(uuid_str, mod_entry);
> 
> We're not checking that we're not walking off the end of the file here.

Will fix

Thanks
-- 
~Vinod

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

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