| Message ID | 20110616225904.GA2064@electric-eye.fr.zoreil.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
> -----Original Message----- > From: Francois Romieu [mailto:romieu@fr.zoreil.com] > Sent: Friday, June 17, 2011 6:59 AM > To: Hayeswang > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v2] net/r8169: update the new parser for > the new firmware > > Hayes Wang <hayeswang@realtek.com> : > > Update the parser for the new firmware which is embedded > some information. > > I have modified several things : > - s/u32/__le32/ in fw_info > - fix unsigned (size_t) comparisons > - more size checks before dereferencing fw_info > Thanks. They are fine. > The new firmware format should be the same. The old > r8168d-1.fw firmware proved usable when prefixed with : > > 0000000: 0000 0000 3031 0000 0000 0000 0000 0000 ....01.......... > 0000010: 0000 0000 0000 0000 0000 0000 0000 0000 ................ > 0000020: 0000 0000 3000 0000 7501 0000 a000 0000 ....0...u....... > > I realized after testing that netif_err could be abused with > non-string fw_info.version. :o/ > Excuse me. I don't understand what you want to express. Do you mean the situation of the old paser with the new firmware for checking the firmware? For the normal situation, the old paser would not use the new firmware. And I put zero in front of the new firmware to prevent the old paser from running it. That is all I do. If you don't mean that, I could promise the new firmware I release would contain the valid string unless someone modifies it. > Comments ? > Best Regards, Hayes -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
hayeswang <hayeswang@realtek.com> : > Francois Romieu : [...] > > The new firmware format should be the same. The old > > r8168d-1.fw firmware proved usable when prefixed with : > > > > 0000000: 0000 0000 3031 0000 0000 0000 0000 0000 ....01.......... > > 0000010: 0000 0000 0000 0000 0000 0000 0000 0000 ................ > > 0000020: 0000 0000 3000 0000 7501 0000 a000 0000 ....0...u....... > > > > I realized after testing that netif_err could be abused with > > non-string fw_info.version. :o/ > > > > Excuse me. I don't understand what you want to express. Do you mean the > situation of the old paser with the new firmware for checking the firmware? No. The code does a printk with a %s specifier on fw_info.version while there is no evidence that fw_info.version is 0 terminated. > For the normal situation, the old paser would not use the new firmware. > And I put zero in front of the new firmware to prevent the old paser from > running it. That is all I do. It is fine.
On Fri, 2011-06-17 at 08:39 +0200, Francois Romieu wrote: > hayeswang <hayeswang@realtek.com> : > > Francois Romieu : > [...] > > > The new firmware format should be the same. The old > > > r8168d-1.fw firmware proved usable when prefixed with : > > > > > > 0000000: 0000 0000 3031 0000 0000 0000 0000 0000 ....01.......... > > > 0000010: 0000 0000 0000 0000 0000 0000 0000 0000 ................ > > > 0000020: 0000 0000 3000 0000 7501 0000 a000 0000 ....0...u....... > > > > > > I realized after testing that netif_err could be abused with > > > non-string fw_info.version. :o/ > > > > > > > Excuse me. I don't understand what you want to express. Do you mean the > > situation of the old paser with the new firmware for checking the firmware? > > No. The code does a printk with a %s specifier on fw_info.version while there > is no evidence that fw_info.version is 0 terminated. I thought the idea of embedding a version here was to be able to report in in get_drvinfo, not to print it on load. Ben. > > For the normal situation, the old paser would not use the new firmware. > > And I put zero in front of the new firmware to prevent the old paser from > > running it. That is all I do. > > It is fine. >
Ben Hutchings <bhutchings@solarflare.com> : [...] > I thought the idea of embedding a version here was to be able to report > in in get_drvinfo, not to print it on load. Reporting the version for the new firmware format through ethtool is currently hacked on. Do you want the load time version / format messages removed ? I think they can be of some use - at least temporarily - but I won't mind removing them now.
On Fri, 2011-06-17 at 14:53 +0200, Francois Romieu wrote: > Ben Hutchings <bhutchings@solarflare.com> : > [...] > > I thought the idea of embedding a version here was to be able to report > > in in get_drvinfo, not to print it on load. > > Reporting the version for the new firmware format through ethtool is > currently hacked on. > > Do you want the load time version / format messages removed ? > > I think they can be of some use - at least temporarily - but I won't mind > removing them now. I don't care one way or the other. But it's more important to report the version through get_drvinfo. Ben.
diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c index 7310824..5a0a7c3 100644 --- a/drivers/net/r8169.c +++ b/drivers/net/r8169.c @@ -1741,21 +1741,82 @@ static void rtl_writephy_batch(struct rtl8169_private *tp, #define PHY_DELAY_MS 0xe0000000 #define PHY_WRITE_ERI_WORD 0xf0000000 +struct fw_info { + u32 magic; + char version[32]; + __le32 fw_start; + __le32 fw_len; + u8 chksum; +} __packed; + +struct fw_phy_code { + __le32 *code; + size_t size; +}; + +#define FW_OPCODE_SIZE sizeof(typeof(*((struct fw_phy_code *)0)->code)) + +static bool rtl_fw_format_ok(struct rtl8169_private *tp, struct net_device *dev, + const struct firmware *fw, struct fw_phy_code *pc) +{ + struct fw_info *fw_info = (struct fw_info *)fw->data; + bool rc = false; + + if (fw->size < FW_OPCODE_SIZE) + goto out; + + if (fw_info->magic == 0) { + size_t i, size, start; + u8 checksum = 0; + + if (fw->size < sizeof(*fw_info)) + goto out; + + for (i = 0; i < fw->size; i++) + checksum += fw->data[i]; + if (checksum != 0) + goto out; + + start = le32_to_cpu(fw_info->fw_start); + if (start > fw->size) + goto out; + + size = le32_to_cpu(fw_info->fw_len); + if (size > (fw->size - start) / FW_OPCODE_SIZE) + goto out; + + netif_info(tp, probe, dev, "firmware: %s\n", fw_info->version); + pc->code = (__le32 *)(fw->data + start); + pc->size = size; + } else { + if (fw->size % FW_OPCODE_SIZE) + goto out; + + netif_info(tp, probe, dev, "legacy firmware format detected\n"); + pc->code = (__le32 *)fw->data; + pc->size = fw->size / FW_OPCODE_SIZE; + } + + rc = true; +out: + return rc; +} + static void rtl_phy_write_fw(struct rtl8169_private *tp, const struct firmware *fw) { - __le32 *phytable = (__le32 *)fw->data; struct net_device *dev = tp->dev; - size_t index, fw_size = fw->size / sizeof(*phytable); + struct fw_phy_code phytable; u32 predata, count; + size_t i; - if (fw->size % sizeof(*phytable)) { - netif_err(tp, probe, dev, "odd sized firmware %zd\n", fw->size); + if (!rtl_fw_format_ok(tp, dev, fw, &phytable)) { + netif_err(tp, probe, dev, "invalid firwmare\n"); return; } - for (index = 0; index < fw_size; index++) { - u32 action = le32_to_cpu(phytable[index]); + for (i = 0; i < phytable.size; i++) { + u32 action = le32_to_cpu(phytable.code[i]); u32 regno = (action & 0x0fff0000) >> 16; switch(action & 0xf0000000) { @@ -1770,14 +1831,14 @@ rtl_phy_write_fw(struct rtl8169_private *tp, const struct firmware *fw) break; case PHY_BJMPN: - if (regno > index) { + if (regno > i) { netif_err(tp, probe, tp->dev, "Out of range of firmware\n"); return; } break; case PHY_READCOUNT_EQ_SKIP: - if (index + 2 >= fw_size) { + if (i + 2 >= phytable.size) { netif_err(tp, probe, tp->dev, "Out of range of firmware\n"); return; @@ -1786,7 +1847,7 @@ rtl_phy_write_fw(struct rtl8169_private *tp, const struct firmware *fw) case PHY_COMP_EQ_SKIPN: case PHY_COMP_NEQ_SKIPN: case PHY_SKIPN: - if (index + 1 + regno >= fw_size) { + if (i + 1 + regno >= phytable.size) { netif_err(tp, probe, tp->dev, "Out of range of firmware\n"); return; @@ -1806,8 +1867,8 @@ rtl_phy_write_fw(struct rtl8169_private *tp, const struct firmware *fw) predata = 0; count = 0; - for (index = 0; index < fw_size; ) { - u32 action = le32_to_cpu(phytable[index]); + for (i = 0; i < phytable.size; ) { + u32 action = le32_to_cpu(phytable.code[i]); u32 data = action & 0x0000ffff; u32 regno = (action & 0x0fff0000) >> 16; @@ -1818,54 +1879,54 @@ rtl_phy_write_fw(struct rtl8169_private *tp, const struct firmware *fw) case PHY_READ: predata = rtl_readphy(tp, regno); count++; - index++; + i++; break; case PHY_DATA_OR: predata |= data; - index++; + i++; break; case PHY_DATA_AND: predata &= data; - index++; + i++; break; case PHY_BJMPN: - index -= regno; + i -= regno; break; case PHY_READ_EFUSE: predata = rtl8168d_efuse_read(tp->mmio_addr, regno); - index++; + i++; break; case PHY_CLEAR_READCOUNT: count = 0; - index++; + i++; break; case PHY_WRITE: rtl_writephy(tp, regno, data); - index++; + i++; break; case PHY_READCOUNT_EQ_SKIP: - index += (count == data) ? 2 : 1; + i += (count == data) ? 2 : 1; break; case PHY_COMP_EQ_SKIPN: if (predata == data) - index += regno; - index++; + i += regno; + i++; break; case PHY_COMP_NEQ_SKIPN: if (predata != data) - index += regno; - index++; + i += regno; + i++; break; case PHY_WRITE_PREVIOUS: rtl_writephy(tp, regno, predata); - index++; + i++; break; case PHY_SKIPN: - index += regno + 1; + i += regno + 1; break; case PHY_DELAY_MS: mdelay(data); - index++; + i++; break; case PHY_READ_MAC_BYTE:
Hayes Wang <hayeswang@realtek.com> : > Update the parser for the new firmware which is embedded some information. I have modified several things : - s/u32/__le32/ in fw_info - fix unsigned (size_t) comparisons - more size checks before dereferencing fw_info The new firmware format should be the same. The old r8168d-1.fw firmware proved usable when prefixed with : 0000000: 0000 0000 3031 0000 0000 0000 0000 0000 ....01.......... 0000010: 0000 0000 0000 0000 0000 0000 0000 0000 ................ 0000020: 0000 0000 3000 0000 7501 0000 a000 0000 ....0...u....... I realized after testing that netif_err could be abused with non-string fw_info.version. :o/ Comments ? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html