| 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: <20210514230152.GL12395@shell.armlinux.org.uk>
Date: Sat, 15 May 2021 00:01:52 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Ansuel Smith <ansuelsmth@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>,
Vivien Didelot <vivien.didelot@...il.com>,
Florian Fainelli <f.fainelli@...il.com>,
Vladimir Oltean <olteanv@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Heiner Kallweit <hkallweit1@...il.com>, netdev@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v6 05/25] net: dsa: qca8k: handle error with
qca8k_read operation
On Fri, May 14, 2021 at 10:59:55PM +0200, Ansuel Smith wrote:
> -static void
> +static int
> qca8k_fdb_read(struct qca8k_priv *priv, struct qca8k_fdb *fdb)
> {
> - u32 reg[4];
> + u32 reg[4], val;
val is unsigned.
> int i;
>
> /* load the ARL table into an array */
> - for (i = 0; i < 4; i++)
> - reg[i] = qca8k_read(priv, QCA8K_REG_ATU_DATA0 + (i * 4));
> + for (i = 0; i < 4; i++) {
> + val = qca8k_read(priv, QCA8K_REG_ATU_DATA0 + (i * 4));
> + if (val < 0)
> + return val;
So this return statement will never be reached.
> @@ -374,6 +386,8 @@ qca8k_fdb_access(struct qca8k_priv *priv, enum qca8k_fdb_cmd cmd, int port)
> /* Check for table full violation when adding an entry */
> if (cmd == QCA8K_FDB_LOAD) {
> reg = qca8k_read(priv, QCA8K_REG_ATU_FUNC);
> + if (reg < 0)
> + return reg;
"reg" here is also a u32, and therefore unsigned, so this will have no
effect.
> if (reg & QCA8K_ATU_FUNC_FULL)
> return -1;
> }
> @@ -388,10 +402,10 @@ qca8k_fdb_next(struct qca8k_priv *priv, struct qca8k_fdb *fdb, int port)
>
> qca8k_fdb_write(priv, fdb->vid, fdb->port_mask, fdb->mac, fdb->aging);
> ret = qca8k_fdb_access(priv, QCA8K_FDB_NEXT, port);
> - if (ret >= 0)
> - qca8k_fdb_read(priv, fdb);
> + if (ret < 0)
> + return ret;
This looks fine to me.
>
> - return ret;
> + return qca8k_fdb_read(priv, fdb);
> }
>
> static int
> @@ -449,6 +463,8 @@ qca8k_vlan_access(struct qca8k_priv *priv, enum qca8k_vlan_cmd cmd, u16 vid)
> /* Check for table full violation when adding an entry */
> if (cmd == QCA8K_VLAN_LOAD) {
> reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC1);
> + if (reg < 0)
> + return reg;
reg is unsigned... unreachable.
> if (reg & QCA8K_VTU_FUNC1_FULL)
> return -ENOMEM;
> }
> @@ -475,6 +491,8 @@ qca8k_vlan_add(struct qca8k_priv *priv, u8 port, u16 vid, bool untagged)
> goto out;
>
> reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC0);
> + if (reg < 0)
> + return reg;
reg is unsigned... unreachable.
> reg |= QCA8K_VTU_FUNC0_VALID | QCA8K_VTU_FUNC0_IVL_EN;
> reg &= ~(QCA8K_VTU_FUNC0_EG_MODE_MASK << QCA8K_VTU_FUNC0_EG_MODE_S(port));
> if (untagged)
> @@ -506,6 +524,8 @@ qca8k_vlan_del(struct qca8k_priv *priv, u8 port, u16 vid)
> goto out;
>
> reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC0);
> + if (reg < 0)
> + return reg;
reg is unsigned... unreachable.
> reg &= ~(3 << QCA8K_VTU_FUNC0_EG_MODE_S(port));
> reg |= QCA8K_VTU_FUNC0_EG_MODE_NOT <<
> QCA8K_VTU_FUNC0_EG_MODE_S(port);
> @@ -621,8 +641,11 @@ qca8k_mdio_read(struct qca8k_priv *priv, int port, u32 regnum)
> QCA8K_MDIO_MASTER_BUSY))
> return -ETIMEDOUT;
>
> - val = (qca8k_read(priv, QCA8K_MDIO_MASTER_CTRL) &
> - QCA8K_MDIO_MASTER_DATA_MASK);
> + val = qca8k_read(priv, QCA8K_MDIO_MASTER_CTRL);
> + if (val < 0)
> + return val;
val is unsigned... unreachable.
> +
> + val &= QCA8K_MDIO_MASTER_DATA_MASK;
>
> return val;
> }
> @@ -978,6 +1001,8 @@ qca8k_phylink_mac_link_state(struct dsa_switch *ds, int port,
> u32 reg;
>
> reg = qca8k_read(priv, QCA8K_REG_PORT_STATUS(port));
> + if (reg < 0)
> + return reg;
reg is unsigned... unreachable.
>
> state->link = !!(reg & QCA8K_PORT_STATUS_LINK_UP);
> state->an_complete = state->link;
> @@ -1078,18 +1103,26 @@ qca8k_get_ethtool_stats(struct dsa_switch *ds, int port,
> {
> struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
> const struct qca8k_mib_desc *mib;
> - u32 reg, i;
> + u32 reg, i, val;
> u64 hi;
>
> for (i = 0; i < ARRAY_SIZE(ar8327_mib); i++) {
> mib = &ar8327_mib[i];
> reg = QCA8K_PORT_MIB_COUNTER(port) + mib->offset;
>
> - data[i] = qca8k_read(priv, reg);
> + val = qca8k_read(priv, reg);
> + if (val < 0)
> + continue;
val is unsigned... unreachable....
> +
> if (mib->size == 2) {
> hi = qca8k_read(priv, reg + 4);
> - data[i] |= hi << 32;
> + if (hi < 0)
> + continue;
hi is a u64, so this condition is always false.
> }
> +
> + data[i] = val;
> + if (mib->size == 2)
> + data[i] |= hi << 32;
> }
> }
>
> @@ -1107,18 +1140,25 @@ qca8k_set_mac_eee(struct dsa_switch *ds, int port, struct ethtool_eee *eee)
> {
> struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
> u32 lpi_en = QCA8K_REG_EEE_CTRL_LPI_EN(port);
> + int ret = 0;
No need to zero-initialise this.
> u32 reg;
>
> mutex_lock(&priv->reg_mutex);
> reg = qca8k_read(priv, QCA8K_REG_EEE_CTRL);
> + if (reg < 0) {
> + ret = reg;
> + goto exit;
> + }
> +
> if (eee->eee_enabled)
> reg |= lpi_en;
> else
> reg &= ~lpi_en;
> qca8k_write(priv, QCA8K_REG_EEE_CTRL, reg);
> - mutex_unlock(&priv->reg_mutex);
>
> - return 0;
> +exit:
> + mutex_unlock(&priv->reg_mutex);
> + return ret;
> }
>
> static int
> @@ -1443,6 +1483,9 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
>
> /* read the switches ID register */
> id = qca8k_read(priv, QCA8K_REG_MASK_CTRL);
> + if (id < 0)
> + return id;
id is unsigned ...
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Powered by blists - more mailing lists