| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 1 Nov 2023 14:13:05 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Christian Marangi <ansuelsmth@...il.com>
Cc: "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Heiner Kallweit <hkallweit1@...il.com>,
Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
Robert Marko <robimarko@...il.com>
Subject: Re: [net-next PATCH v2 1/2] net: phy: aquantia: add firmware load
support
On Wed, Nov 01, 2023 at 01:36:07PM +0100, Christian Marangi wrote:
> From: Robert Marko <robimarko@...il.com>
>
> Aquantia PHY-s require firmware to be loaded before they start operating.
> It can be automatically loaded in case when there is a SPI-NOR connected
> to Aquantia PHY-s or can be loaded from the host via MDIO.
>
> This patch adds support for loading the firmware via MDIO as in most cases
> there is no SPI-NOR being used to save on cost.
> Firmware loading code itself is ported from mainline U-boot with cleanups.
>
> The firmware has mixed values both in big and little endian.
> PHY core itself is big-endian but it expects values to be in little-endian.
> The firmware is little-endian but CRC-16 value for it is stored at the end
> of firmware in big-endian.
>
> It seems the PHY does the conversion internally from firmware that is
> little-endian to the PHY that is big-endian on using the mailbox
> but mailbox returns a big-endian CRC-16 to verify the written data
> integrity.
>
> Co-developed-by: Christian Marangi <ansuelsmth@...il.com>
> Signed-off-by: Robert Marko <robimarko@...il.com>
> Signed-off-by: Christian Marangi <ansuelsmth@...il.com>
> ---
> Changes v2:
> - Move out of RFC
Actually, since we are in the merge window, RFC would be correct.
> - Address sanity check for offsets
> - Add additional comments on firmware load check
> - Fix some typo
> - Capitalize CRC in comments
> - Rename load_sysfs to load_fs
>
> drivers/net/phy/Kconfig | 1 +
> drivers/net/phy/aquantia_main.c | 304 ++++++++++++++++++++++++++++++++
> 2 files changed, 305 insertions(+)
>
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 421d2b62918f..46c7194efcea 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -98,6 +98,7 @@ config ADIN1100_PHY
>
> config AQUANTIA_PHY
> tristate "Aquantia PHYs"
> + select CRC_CCITT
> help
> Currently supports the Aquantia AQ1202, AQ2104, AQR105, AQR405
>
> diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
> index 334a6904ca5a..0f1b8d75cca0 100644
> --- a/drivers/net/phy/aquantia_main.c
> +++ b/drivers/net/phy/aquantia_main.c
> @@ -12,6 +12,10 @@
> #include <linux/delay.h>
> #include <linux/bitfield.h>
> #include <linux/phy.h>
> +#include <linux/of.h>
> +#include <linux/firmware.h>
> +#include <linux/crc-ccitt.h>
> +#include <linux/nvmem-consumer.h>
>
> #include "aquantia.h"
>
> @@ -92,10 +96,40 @@
> #define MDIO_C22EXT_STAT_SGMII_TX_RUNT_FRAMES 0xd31b
>
> /* Vendor specific 1, MDIO_MMD_VEND1 */
> +#define VEND1_GLOBAL_SC 0x0
> +#define VEND1_GLOBAL_SC_SOFT_RESET BIT(15)
> +#define VEND1_GLOBAL_SC_LOW_POWER BIT(11)
> +
> #define VEND1_GLOBAL_FW_ID 0x0020
> #define VEND1_GLOBAL_FW_ID_MAJOR GENMASK(15, 8)
> #define VEND1_GLOBAL_FW_ID_MINOR GENMASK(7, 0)
>
> +#define VEND1_GLOBAL_MAILBOX_INTERFACE1 0x0200
> +#define VEND1_GLOBAL_MAILBOX_INTERFACE1_EXECUTE BIT(15)
> +#define VEND1_GLOBAL_MAILBOX_INTERFACE1_WRITE BIT(14)
> +#define VEND1_GLOBAL_MAILBOX_INTERFACE1_CRC_RESET BIT(12)
> +#define VEND1_GLOBAL_MAILBOX_INTERFACE1_BUSY BIT(8)
> +
> +#define VEND1_GLOBAL_MAILBOX_INTERFACE2 0x0201
> +#define VEND1_GLOBAL_MAILBOX_INTERFACE3 0x0202
> +#define VEND1_GLOBAL_MAILBOX_INTERFACE3_MSW_ADDR_MASK GENMASK(15, 0)
> +#define VEND1_GLOBAL_MAILBOX_INTERFACE3_MSW_ADDR(x) FIELD_PREP(VEND1_GLOBAL_MAILBOX_INTERFACE3_MSW_ADDR_MASK, (u16)((x) >> 16))
> +#define VEND1_GLOBAL_MAILBOX_INTERFACE4 0x0203
> +#define VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR_MASK GENMASK(15, 2)
> +#define VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR(x) FIELD_PREP(VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR_MASK, (u16)(x))
> +
> +#define VEND1_GLOBAL_MAILBOX_INTERFACE5 0x0204
> +#define VEND1_GLOBAL_MAILBOX_INTERFACE5_MSW_DATA_MASK GENMASK(15, 0)
> +#define VEND1_GLOBAL_MAILBOX_INTERFACE5_MSW_DATA(x) FIELD_PREP(VEND1_GLOBAL_MAILBOX_INTERFACE5_MSW_DATA_MASK, (u16)((x) >> 16))
> +#define VEND1_GLOBAL_MAILBOX_INTERFACE6 0x0205
> +#define VEND1_GLOBAL_MAILBOX_INTERFACE6_LSW_DATA_MASK GENMASK(15, 0)
> +#define VEND1_GLOBAL_MAILBOX_INTERFACE6_LSW_DATA(x) FIELD_PREP(VEND1_GLOBAL_MAILBOX_INTERFACE6_LSW_DATA_MASK, (u16)(x))
> +
> +#define VEND1_GLOBAL_CONTROL2 0xc001
> +#define VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_RST BIT(15)
> +#define VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_OVD BIT(6)
> +#define VEND1_GLOBAL_CONTROL2_UP_RUN_STALL BIT(0)
> +
> #define VEND1_GLOBAL_GEN_STAT2 0xc831
> #define VEND1_GLOBAL_GEN_STAT2_OP_IN_PROG BIT(15)
>
> @@ -152,6 +186,30 @@
> #define AQR107_OP_IN_PROG_SLEEP 1000
> #define AQR107_OP_IN_PROG_TIMEOUT 100000
>
> +#define UP_RESET_SLEEP 100
> +
> +/* addresses of memory segments in the phy */
> +#define DRAM_BASE_ADDR 0x3FFE0000
> +#define IRAM_BASE_ADDR 0x40000000
> +
> +/* firmware image format constants */
> +#define VERSION_STRING_SIZE 0x40
> +#define VERSION_STRING_OFFSET 0x0200
> +/* primary offset is written at an offset from the start of the fw blob */
> +#define PRIMARY_OFFSET_OFFSET 0x8
> +/* primary offset needs to be then added to a base offset */
> +#define PRIMARY_OFFSET_SHIFT 12
> +#define PRIMARY_OFFSET(x) ((x) << PRIMARY_OFFSET_SHIFT)
> +#define HEADER_OFFSET 0x300
> +
> +struct aqr_fw_header {
> + u32 padding;
> + u8 iram_offset[3];
> + u8 iram_size[3];
> + u8 dram_offset[3];
> + u8 dram_size[3];
> +} __packed;
> +
> struct aqr107_hw_stat {
> const char *name;
> int reg;
> @@ -677,6 +735,166 @@ static int aqr107_wait_processor_intensive_op(struct phy_device *phydev)
> return 0;
> }
>
> +/* load data into the phy's memory */
> +static int aquantia_load_memory(struct phy_device *phydev, u32 addr,
> + const u8 *data, size_t len)
> +{
> + for (pos = 0; pos < len; pos += min(sizeof(u32), len - pos)) {
> + u32 word = 0;
> +
> + memcpy(&word, data + pos, min(sizeof(u32), len - pos));
Rather than do a memcpy, use the get_unaligned_ macros. They might map
to a memcpy(), but some architectures can do unaligned accesses
without problems.
> +static int aqr_fw_boot(struct phy_device *phydev, const u8 *data, size_t size)
> +{
> + const struct aqr_fw_header *header;
> + u32 iram_offset = 0, iram_size = 0;
> + u32 dram_offset = 0, dram_size = 0;
> + char version[VERSION_STRING_SIZE];
> + u16 calculated_crc, read_crc;
> + u32 primary_offset = 0;
> + int ret;
> +
> + /* extract saved CRC at the end of the fw */
> + memcpy(&read_crc, data + size - 2, sizeof(read_crc));
Say size == 1. You just had a buffer underrun.
> + /* CRC is saved in big-endian as PHY is BE */
> + read_crc = be16_to_cpu(read_crc);
> + calculated_crc = crc_ccitt_false(0, data, size - 2);
> + if (read_crc != calculated_crc) {
> + phydev_err(phydev, "bad firmware CRC: file 0x%04x calculated 0x%04x\n",
> + read_crc, calculated_crc);
> + return -EINVAL;
> + }
> +
> + /* Get the primary offset to extract DRAM and IRAM sections. */
> + memcpy(&primary_offset, data + PRIMARY_OFFSET_OFFSET, sizeof(u16));
What if PRIMARY_OFFSET_OFFSET + sizeof(u16) is greater than size? A
buffer overrun.
Assume the firmware is evil and is trying to hack you. Always test
everything.
I would suggest some helpers, something like
int aqr_fw_get_u16(const u8 *data, size_t size, size_t offset, u16 *value)
Check that offset + sizeof(u16) is within the firmware, and if not return -EINVAL.
Otherwise set *value to the u16 from the firmware and return 0.
This is where Rust would be nice :-)
Andrew
---
pw-bot: cr
Powered by blists - more mailing lists