RE: Re: Re: Re: [PATCH v3 4/4] mwifiex: pcie: extract wifi part from combo firmware during function level reset

[Xinming Hu]

Hi Brain,

> -Original Message-
> From: Brian Norris [mailto:briannor...@chromium.org]
> Sent: 2017年4月15日 0:56
> To: Xinming Hu
> Cc: Linux Wireless; Kalle Valo; Dmitry Torokhov; raja...@google.com;
> Amitkumar Karwar; Cathy Luo; Ganapathi Bhat
> Subject: [EXT] Re: Re: Re: [PATCH v3 4/4] mwifiex: pcie: extract wifi part 
> from
> combo firmware during function level reset
> 
> External Email
> 
> --
> Hi,
> 
> On Fri, Apr 14, 2017 at 03:28:28AM +, Xinming Hu wrote:
> > 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*
> 
> I was primarily interested in protecting the kernel itself. Once the kernel 
> starts
> parsing the firmware, we have to make sure a bad firmware file won't end up
> with us looping infinitely, reading/writing invalid memory, or otherwise
> exposing security or stability issues. I wasn't necessarily interested in 
> validating
> every requirement of the end-point device. For example, we're not bothering
> checking the CRCs. I figured that this was all the job of your Wifi card's 
> boot
> ROM.
> 
> So, we *can* implement checks like this, but I'd really hope we don't need 
> this
> particular one, because your card should be taking care of that.
> 

Got it, we will keep in mind to check the possible overflow in future, either 
using general
protect or under limit by our device requirement.

> Please consider reviewing my latest submission.
> 

Sure.

Thanks,
Simon
> Regards,
> Brian

Re: Re: Re: [PATCH v3 4/4] mwifiex: pcie: extract wifi part from combo firmware during function level reset

[Brian Norris]

Hi,

On Fri, Apr 14, 2017 at 03:28:28AM +, Xinming Hu wrote:
> 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*

I was primarily interested in protecting the kernel itself. Once the
kernel starts parsing the firmware, we have to make sure a bad firmware
file won't end up with us looping infinitely, reading/writing invalid
memory, or otherwise exposing security or stability issues. I wasn't
necessarily interested in validating every requirement of the end-point
device. For example, we're not bothering checking the CRCs. I figured that
this was all the job of your Wifi card's boot ROM.

So, we *can* implement checks like this, but I'd really hope we don't
need this particular one, because your card should be taking care of
that.

Please consider reviewing my latest submission.

Regards,
Brian

RE: Re: Re: [PATCH v3 4/4] mwifiex: pcie: extract wifi part from combo firmware during function level reset

[Xinming Hu]

Hi Brain,

> -Original Message-
> From: Brian Norris [mailto:briannor...@chromium.org]
> Sent: 2017年4月14日 1:50
> To: Xinming Hu
> Cc: Linux Wireless; Kalle Valo; Dmitry Torokhov; raja...@google.com;
> 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 +, Xinming Hu wrote:
> > > -Original Message-
> > > From: Brian Norris [mailto:briannor...@chromium.org]
> > > Sent: 2017年4月11日 9:37
> > > To: Xinming Hu
> > > Cc: Linux Wireless; Kalle Valo; Dmitry Torokhov; raja...@google.com;
> > > 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 +, 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(, 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 = 0xfff0. Then:
> 
>   offset += data_len + sizeof(fwdata.header); =>
>   offset += 0xfff0 + 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)
>   

Re: Re: [PATCH v3 4/4] mwifiex: pcie: extract wifi part from combo firmware during function level reset

[Brian Norris]

Hi,

On Thu, Apr 13, 2017 at 06:47:59AM +, Xinming Hu wrote:
> > -Original Message-
> > From: Brian Norris [mailto:briannor...@chromium.org]
> > Sent: 2017年4月11日 9:37
> > To: Xinming Hu
> > Cc: Linux Wireless; Kalle Valo; Dmitry Torokhov; raja...@google.com;
> > 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 +, 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(, 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 = 0xfff0. Then:

offset += data_len + sizeof(fwdata.header);
=>
offset += 0xfff0 + 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 ...

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

RE: Re: [PATCH v3 4/4] mwifiex: pcie: extract wifi part from combo firmware during function level reset

[Xinming Hu]

Hi Brain,

> -Original Message-
> From: Brian Norris [mailto:briannor...@chromium.org]
> Sent: 2017年4月11日 9:37
> To: Xinming Hu
> Cc: Linux Wireless; Kalle Valo; Dmitry Torokhov; raja...@google.com;
> 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
> 
> --
> Hi,
> 
> On Mon, Apr 10, 2017 at 09:09:34AM +, Xinming Hu wrote:
> > From: Xinming Hu 
> >
> > A seperate wifi-only firmware was download during pcie function level reset.
> > It is in fact the tail part of wifi/bt combo firmware. Per Brian's and
> > Dmitry's suggestion, this patch extract the wifi part from combo firmware.
> >
> > After that, we can discard the redudant image in linux-firmware repo.
> >
> > Signed-off-by: Xinming Hu 
> > Signed-off-by: Ganapathi Bhat 
> > Signed-off-by: Cathy Luo 
> > ---
> > v2: extract wifi part from combo firmware(Dimtry and Brain)
> > add more description(Kalle)
> > v3: same as v2
> > ---
> >  drivers/net/wireless/marvell/mwifiex/fw.h   | 18 +++
> >  drivers/net/wireless/marvell/mwifiex/pcie.c | 83
> > ++---
> > drivers/net/wireless/marvell/mwifiex/pcie.h |  2 +
> >  3 files changed, 96 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h
> > b/drivers/net/wireless/marvell/mwifiex/fw.h
> > index 0b68374..6cf9ab9 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/fw.h
> > +++ b/drivers/net/wireless/marvell/mwifiex/fw.h
> > @@ -43,6 +43,24 @@ struct tx_packet_hdr {
> > struct rfc_1042_hdr rfc1042_hdr;
> >  } __packed;
> >
> > +struct mwifiex_fw_header {
> > +   __le32 dnld_cmd;
> > +   __le32 base_addr;
> > +   __le32 data_length;
> > +   __le32 crc;
> > +} __packed;
> > +
> > +struct mwifiex_fw_data {
> > +   struct mwifiex_fw_header header;
> > +   __le32 seq_num;
> > +   u8 data[1];
> > +} __packed;
> > +
> > +#define MWIFIEX_FW_DNLD_CMD_1 0x1
> > +#define MWIFIEX_FW_DNLD_CMD_5 0x5
> > +#define MWIFIEX_FW_DNLD_CMD_6 0x6
> > +#define MWIFIEX_FW_DNLD_CMD_7 0x7
> > +
> >  #define B_SUPPORTED_RATES   5
> >  #define G_SUPPORTED_RATES   9
> >  #define BG_SUPPORTED_RATES  13
> > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > index a07cb0a..ebf00d9 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > @@ -1956,6 +1956,63 @@ static int mwifiex_pcie_event_complete(struct
> mwifiex_adapter *adapter,
> > return ret;
> >  }
> >
> > +/* Extract wifi part from wifi-bt combo firmware image.
> > + */
> > +
> > +static int mwifiex_extract_wifi_fw(struct mwifiex_adapter *adapter,
> > +  u8 *firmware, u32 firmware_len) {
> 
> Can you make 'firmware' const? Also, to help below, it might work better as
> 'const void *'.
> 

OK, Thanks for the review.

> > +   struct mwifiex_fw_data fwdata;
> > +   u32 offset = 0, data_len, dnld_cmd;
> > +   int ret = 0;
> > +   bool cmd7_before = false;
> > +
> > +   while (1) {
> > +   if (offset + sizeof(fwdata.header) >= firmware_len) {
> > +   mwifiex_dbg(adapter, ERROR,
> > +   "extract wifi-only firmware failure!");
> > +   ret = -1;
> > +   goto done;
> > +   }
> > +
> > +   memcpy(, 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)

> > +   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