Hi Brain,
> -----Original Message-----
> From: Brian Norris [mailto:[email protected]]
> Sent: 2017年4月14日 1:50
> To: Xinming Hu
> Cc: Linux Wireless; Kalle Valo; Dmitry Torokhov; [email protected];
> Amitkumar Karwar; Cathy Luo; Ganapathi Bhat
> Subject: [EXT] Re: Re: [PATCH v3 4/4] mwifiex: pcie: extract wifi part from
> combo firmware during function level reset
>
> External Email
>
> ----------------------------------------------------------------------
> Hi,
>
> On Thu, Apr 13, 2017 at 06:47:59AM +0000, Xinming Hu wrote:
> > > -----Original Message-----
> > > From: Brian Norris [mailto:[email protected]]
> > > Sent: 2017年4月11日 9:37
> > > To: Xinming Hu
> > > Cc: Linux Wireless; Kalle Valo; Dmitry Torokhov; [email protected];
> > > Amitkumar Karwar; Cathy Luo; Xinming Hu; Ganapathi Bhat
> > > Subject: [EXT] Re: [PATCH v3 4/4] mwifiex: pcie: extract wifi part
> > > from combo firmware during function level reset
> > >
> > > External Email
> > >
>
> > > On Mon, Apr 10, 2017 at 09:09:34AM +0000, Xinming Hu wrote:
> > > > + while (1) {
> > > > + if (offset + sizeof(fwdata.header) >= firmware_len) {
> > > > + mwifiex_dbg(adapter, ERROR,
> > > > + "extract wifi-only firmware
> > > > failure!");
> > > > + ret = -1;
> > > > + goto done;
> > > > + }
> > > > +
> > > > + memcpy(&fwdata.header, firmware + offset,
> > > > + sizeof(fwdata.header));
> > >
> > > Do you actually need to copy this? Can't you just keep a pointer to the
> location?
> > > e.g.
> > >
> > > const struct mwifiex_fw_data *fwdata; ...
> > > fwdata = firmware + offset;
> > >
> >
> > Ok.
> >
> > > > + dnld_cmd = le32_to_cpu(fwdata.header.dnld_cmd);
> > > > + data_len = le32_to_cpu(fwdata.header.data_length);
> > > > +
> > > > + switch (dnld_cmd) {
> > > > + case MWIFIEX_FW_DNLD_CMD_1:
> > > > + if (!cmd7_before) {
> > > > + mwifiex_dbg(adapter, ERROR,
> > > > + "no cmd7 before cmd1!");
> > > > + ret = -1;
> > > > + goto done;
> > > > + }
> > > > + offset += data_len + sizeof(fwdata.header);
> > >
> > > Technically, we need an overflow check to make sure that 'data_len'
> > > isn't some bogus value that overflows 'offset'.
> > >
> >
> > There is the sanity check for the offset after bypass CMD1/5/7 in the
> > start of while-loop, enhanced in v4 as, if (offset >= firmware_len)
>
> That's not an enhancement!! That's a *weaker* condition, and it's wrong.
> If 'offset == firmware_len - 1', then we'll still be out of bounds when we
> read
> the next header at 'offset + {1, 2, 3, ...}'.
>
> My point was that adding 'data_len' might actually overflow the u32, not that
> the bounds check ('offset + sizeof(...header) >= firmware_len') was wrong.
>
> For this kind of overflow check, you need to do the check here, not after
> you've
> saved the new offset.
>
> e.g., suppose data_len = 0xfffffff0. Then:
>
> offset += data_len + sizeof(fwdata.header); =>
> offset += 0xfffffff0 + 16;
> =>
> offset += 0;
>
> and then...we have an infinite loop. Changing the bounds check at the start of
> the loop does nothing to help this.
>
> Something like this (put before the addition) is sufficient, I think:
>
> if (offset + data_len + sizeof(fwdata.header) < data_len)
> ... error ...
>
Thanks for the explain.
According to the firmware download protocol, every CMD should not exceed
MWIFIEX_UPLD_SIZE.
we can add a sanity check , like,
if (data_len > MWIFIEX_UPLD_SIZE - sizeof(fwdata->header))
*error*
Thanks
Simon
> Or this would actually all be slightly cleaner if you just did this outside
> the
> 'switch':
>
> // Presuming you already did the check for
> // offset + sizeof(fwdata.header) >= firmware_len
> offset += sizeof(fwdata.header);
>
> Then inside this 'case', you have:
>
> if (offset + data_len < data_len)
> ... error out ...
> offset += data_len;
>
> > > > + break;
> > > > + case MWIFIEX_FW_DNLD_CMD_5:
> > > > + offset += data_len + sizeof(fwdata.header);
> > >
> > > Same here.
> > >
> > > > + break;
> > > > + case MWIFIEX_FW_DNLD_CMD_6:
> > >
> > > Can we have a comment, either here or above this function, to
> > > describe what this sequence is? Or particularly, what is this terminating
> condition? "CMD_6"
> > > doesn't really tell me that this is the start of the Wifi blob.
> > >
> > > > + offset += data_len + sizeof(fwdata.header);
> > >
> >
> > The sequence have been added to function comments in v4.
> >
> > > You don't check for overflow here. Check this before returning this
> > > (either here, or in the 'done' label).
> > >
> >
> > Yes, add sanity check for the offset in end of CMD6.
> >
> > > > + ret = offset;
> > > > + goto done;
> > > > + case MWIFIEX_FW_DNLD_CMD_7:
> > > > + if (!cmd7_before)
> > >
> > > ^^ This 'if' isn't really needed.
> >
> > Done.
> >
> > >
> > > > + cmd7_before = true;
> > > > + offset += sizeof(fwdata.header);
> > > > + break;
> > > > + default:
> > > > + mwifiex_dbg(adapter, ERROR, "unknown dnld_cmd
> > > > %d\n",
> > > > + dnld_cmd);
> > > > + ret = -1;
> > > > + goto done;
> > > > + }
> > > > + }
> > > > +
> > > > +done:
> > > > + return ret;
> > > > +}
>
> [...]
>
> Brian
