Hi Brain, > -----Original Message----- > From: Brian Norris [mailto:briannorris@xxxxxxxxxxxx] > Sent: 2017年4月14日 1:50 > To: Xinming Hu > Cc: Linux Wireless; Kalle Valo; Dmitry Torokhov; rajatja@xxxxxxxxxx; > 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:briannorris@xxxxxxxxxxxx] > > > Sent: 2017年4月11日 9:37 > > > To: Xinming Hu > > > Cc: Linux Wireless; Kalle Valo; Dmitry Torokhov; rajatja@xxxxxxxxxx; > > > 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
