| 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
| ||
|
Message-ID: <65427400.5d0a0220.41c58.0ded@mx.google.com>
Date: Wed, 1 Nov 2023 16:51:26 +0100
From: Christian Marangi <ansuelsmth@...il.com>
To: Andrew Lunn <andrew@...n.ch>
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 02:13:05PM +0100, Andrew Lunn wrote:
> 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.
>
My bad!
> > - 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.
>
I don't think this is doable for this loop, think we would end up in
some funny situation where for the last run we have to copy less than
u32. (get_unaligned would always take u32 of data and that would end up
reading more than requested) Am I wrong?
Aside from this, in the other part of the code I can use the macro and
skip having to convert them.
> > +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
--
Ansuel
Powered by blists - more mailing lists