| Message ID | 1491815374-6555-4-git-send-email-huxinming820@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
Hi, On Mon, Apr 10, 2017 at 09:09:34AM +0000, Xinming Hu wrote: > From: Xinming Hu <huxm@marvell.com> > > 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 <huxm@marvell.com> > Signed-off-by: Ganapathi Bhat <gbhat@marvell.com> > Signed-off-by: Cathy Luo <cluo@marvell.com> > --- > 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 *'. > + 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(&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; > + 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'. > + 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); You don't check for overflow here. Check this before returning this (either here, or in the 'done' label). > + ret = offset; > + goto done; > + case MWIFIEX_FW_DNLD_CMD_7: > + if (!cmd7_before) ^^ This 'if' isn't really needed. > + 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; > +} > + > /* > * This function downloads the firmware to the card. > * > @@ -1971,7 +2028,7 @@ static int mwifiex_prog_fw_w_helper(struct mwifiex_adapter *adapter, > u32 firmware_len = fw->fw_len; > u32 offset = 0; > struct sk_buff *skb; > - u32 txlen, tx_blocks = 0, tries, len; > + u32 txlen, tx_blocks = 0, tries, len, val; > u32 block_retry_cnt = 0; > struct pcie_service_card *card = adapter->card; > const struct mwifiex_pcie_card_reg *reg = card->pcie.reg; > @@ -1998,6 +2055,24 @@ static int mwifiex_prog_fw_w_helper(struct mwifiex_adapter *adapter, > goto done; > } > > + ret = mwifiex_read_reg(adapter, PCIE_SCRATCH_13_REG, &val); > + if (ret) { > + mwifiex_dbg(adapter, FATAL, "Failed to read scratch register 13\n"); > + goto done; > + } > + > + /* PCIE FLR case: extract wifi part from combo firmware*/ > + if (val == MWIFIEX_PCIE_FLR_HAPPENS) { > + ret = mwifiex_extract_wifi_fw(adapter, firmware, firmware_len); > + if (ret < 0) { > + mwifiex_dbg(adapter, ERROR, "Failed to extract wifi fw\n"); > + goto done; > + } > + offset = ret; > + mwifiex_dbg(adapter, MSG, > + "info: dnld wifi firmware from %d bytes\n", offset); > + } > + > /* Perform firmware data transfer */ > do { > u32 ireg_intr = 0; > @@ -3060,12 +3135,6 @@ static void mwifiex_pcie_up_dev(struct mwifiex_adapter *adapter) > struct pci_dev *pdev = card->dev; > const struct mwifiex_pcie_card_reg *reg = card->pcie.reg; > > - /* Bluetooth is not on pcie interface. Download Wifi only firmware > - * during pcie FLR, so that bluetooth part of firmware which is > - * already running doesn't get affected. > - */ > - strcpy(adapter->fw_name, PCIE8997_DEFAULT_WIFIFW_NAME); Now that there's no users, delete PCIE8997_DEFAULT_WIFIFW_NAME from pcie.h. Brian > - > /* tx_buf_size might be changed to 3584 by firmware during > * data transfer, we should reset it to default size. > */ > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.h b/drivers/net/wireless/marvell/mwifiex/pcie.h > index 7e2450c..54aecda 100644 > --- a/drivers/net/wireless/marvell/mwifiex/pcie.h > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.h > @@ -120,6 +120,8 @@ > #define MWIFIEX_SLEEP_COOKIE_SIZE 4 > #define MWIFIEX_MAX_DELAY_COUNT 100 > > +#define MWIFIEX_PCIE_FLR_HAPPENS 0xFEDCBABA > + > struct mwifiex_pcie_card_reg { > u16 cmd_addr_lo; > u16 cmd_addr_hi; > -- > 1.8.1.4 >
Hi, One more thing, since you need to update this patchset anyway: On Mon, Apr 10, 2017 at 09:09:34AM +0000, Xinming Hu wrote: > From: Xinming Hu <huxm@marvell.com> > > A seperate wifi-only firmware was download during pcie function level reset. You might consider running checkpatch.pl on your submissions. It's helpful in general, but in this case, it would actually catch a spelling error: s/seperate/separate/ > 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 <huxm@marvell.com> > Signed-off-by: Ganapathi Bhat <gbhat@marvell.com> > Signed-off-by: Cathy Luo <cluo@marvell.com> Brian
Hi Brain, > -----Original Message----- > From: Brian Norris [mailto:briannorris@chromium.org] > Sent: 2017年4月11日 9:37 > To: Xinming Hu > Cc: Linux Wireless; Kalle Valo; Dmitry Torokhov; rajatja@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 +0000, Xinming Hu wrote: > > From: Xinming Hu <huxm@marvell.com> > > > > 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 <huxm@marvell.com> > > Signed-off-by: Ganapathi Bhat <gbhat@marvell.com> > > Signed-off-by: Cathy Luo <cluo@marvell.com> > > --- > > 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(&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) > > + 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; > > +} > > + > > /* > > * This function downloads the firmware to the card. > > * > > @@ -1971,7 +2028,7 @@ static int mwifiex_prog_fw_w_helper(struct > mwifiex_adapter *adapter, > > u32 firmware_len = fw->fw_len; > > u32 offset = 0; > > struct sk_buff *skb; > > - u32 txlen, tx_blocks = 0, tries, len; > > + u32 txlen, tx_blocks = 0, tries, len, val; > > u32 block_retry_cnt = 0; > > struct pcie_service_card *card = adapter->card; > > const struct mwifiex_pcie_card_reg *reg = card->pcie.reg; @@ -1998,6 > > +2055,24 @@ static int mwifiex_prog_fw_w_helper(struct mwifiex_adapter > *adapter, > > goto done; > > } > > > > + ret = mwifiex_read_reg(adapter, PCIE_SCRATCH_13_REG, &val); > > + if (ret) { > > + mwifiex_dbg(adapter, FATAL, "Failed to read scratch register 13\n"); > > + goto done; > > + } > > + > > + /* PCIE FLR case: extract wifi part from combo firmware*/ > > + if (val == MWIFIEX_PCIE_FLR_HAPPENS) { > > + ret = mwifiex_extract_wifi_fw(adapter, firmware, firmware_len); > > + if (ret < 0) { > > + mwifiex_dbg(adapter, ERROR, "Failed to extract wifi fw\n"); > > + goto done; > > + } > > + offset = ret; > > + mwifiex_dbg(adapter, MSG, > > + "info: dnld wifi firmware from %d bytes\n", offset); > > + } > > + > > /* Perform firmware data transfer */ > > do { > > u32 ireg_intr = 0; > > @@ -3060,12 +3135,6 @@ static void mwifiex_pcie_up_dev(struct > mwifiex_adapter *adapter) > > struct pci_dev *pdev = card->dev; > > const struct mwifiex_pcie_card_reg *reg = card->pcie.reg; > > > > - /* Bluetooth is not on pcie interface. Download Wifi only firmware > > - * during pcie FLR, so that bluetooth part of firmware which is > > - * already running doesn't get affected. > > - */ > > - strcpy(adapter->fw_name, PCIE8997_DEFAULT_WIFIFW_NAME); > > Now that there's no users, delete PCIE8997_DEFAULT_WIFIFW_NAME from > pcie.h. Removed in V4. > > Brian > > > - > > /* tx_buf_size might be changed to 3584 by firmware during > > * data transfer, we should reset it to default size. > > */ > > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.h > > b/drivers/net/wireless/marvell/mwifiex/pcie.h > > index 7e2450c..54aecda 100644 > > --- a/drivers/net/wireless/marvell/mwifiex/pcie.h > > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.h > > @@ -120,6 +120,8 @@ > > #define MWIFIEX_SLEEP_COOKIE_SIZE 4 > > #define MWIFIEX_MAX_DELAY_COUNT 100 > > > > +#define MWIFIEX_PCIE_FLR_HAPPENS 0xFEDCBABA > > + > > struct mwifiex_pcie_card_reg { > > u16 cmd_addr_lo; > > u16 cmd_addr_hi; > > -- > > 1.8.1.4 > >
Hi, On Thu, Apr 13, 2017 at 06:47:59AM +0000, Xinming Hu wrote: > > -----Original Message----- > > From: Brian Norris [mailto:briannorris@chromium.org] > > Sent: 2017年4月11日 9:37 > > To: Xinming Hu > > Cc: Linux Wireless; Kalle Valo; Dmitry Torokhov; rajatja@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 +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 ... 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
Hi Brain, > -----Original Message----- > From: Brian Norris [mailto:briannorris@chromium.org] > Sent: 2017年4月14日 1:50 > To: Xinming Hu > Cc: Linux Wireless; Kalle Valo; Dmitry Torokhov; rajatja@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 +0000, Xinming Hu wrote: > > > -----Original Message----- > > > From: Brian Norris [mailto:briannorris@chromium.org] > > > Sent: 2017年4月11日 9:37 > > > To: Xinming Hu > > > Cc: Linux Wireless; Kalle Valo; Dmitry Torokhov; rajatja@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 +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
Hi, On Fri, Apr 14, 2017 at 03:28:28AM +0000, 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
Hi Brain, > -----Original Message----- > From: Brian Norris [mailto:briannorris@chromium.org] > Sent: 2017年4月15日 0:56 > To: Xinming Hu > Cc: Linux Wireless; Kalle Valo; Dmitry Torokhov; rajatja@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 +0000, 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
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) { + 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(&fwdata.header, firmware + offset, + sizeof(fwdata.header)); + 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); + break; + case MWIFIEX_FW_DNLD_CMD_5: + offset += data_len + sizeof(fwdata.header); + break; + case MWIFIEX_FW_DNLD_CMD_6: + offset += data_len + sizeof(fwdata.header); + ret = offset; + goto done; + case MWIFIEX_FW_DNLD_CMD_7: + if (!cmd7_before) + 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; +} + /* * This function downloads the firmware to the card. * @@ -1971,7 +2028,7 @@ static int mwifiex_prog_fw_w_helper(struct mwifiex_adapter *adapter, u32 firmware_len = fw->fw_len; u32 offset = 0; struct sk_buff *skb; - u32 txlen, tx_blocks = 0, tries, len; + u32 txlen, tx_blocks = 0, tries, len, val; u32 block_retry_cnt = 0; struct pcie_service_card *card = adapter->card; const struct mwifiex_pcie_card_reg *reg = card->pcie.reg; @@ -1998,6 +2055,24 @@ static int mwifiex_prog_fw_w_helper(struct mwifiex_adapter *adapter, goto done; } + ret = mwifiex_read_reg(adapter, PCIE_SCRATCH_13_REG, &val); + if (ret) { + mwifiex_dbg(adapter, FATAL, "Failed to read scratch register 13\n"); + goto done; + } + + /* PCIE FLR case: extract wifi part from combo firmware*/ + if (val == MWIFIEX_PCIE_FLR_HAPPENS) { + ret = mwifiex_extract_wifi_fw(adapter, firmware, firmware_len); + if (ret < 0) { + mwifiex_dbg(adapter, ERROR, "Failed to extract wifi fw\n"); + goto done; + } + offset = ret; + mwifiex_dbg(adapter, MSG, + "info: dnld wifi firmware from %d bytes\n", offset); + } + /* Perform firmware data transfer */ do { u32 ireg_intr = 0; @@ -3060,12 +3135,6 @@ static void mwifiex_pcie_up_dev(struct mwifiex_adapter *adapter) struct pci_dev *pdev = card->dev; const struct mwifiex_pcie_card_reg *reg = card->pcie.reg; - /* Bluetooth is not on pcie interface. Download Wifi only firmware - * during pcie FLR, so that bluetooth part of firmware which is - * already running doesn't get affected. - */ - strcpy(adapter->fw_name, PCIE8997_DEFAULT_WIFIFW_NAME); - /* tx_buf_size might be changed to 3584 by firmware during * data transfer, we should reset it to default size. */ diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.h b/drivers/net/wireless/marvell/mwifiex/pcie.h index 7e2450c..54aecda 100644 --- a/drivers/net/wireless/marvell/mwifiex/pcie.h +++ b/drivers/net/wireless/marvell/mwifiex/pcie.h @@ -120,6 +120,8 @@ #define MWIFIEX_SLEEP_COOKIE_SIZE 4 #define MWIFIEX_MAX_DELAY_COUNT 100 +#define MWIFIEX_PCIE_FLR_HAPPENS 0xFEDCBABA + struct mwifiex_pcie_card_reg { u16 cmd_addr_lo; u16 cmd_addr_hi;