linux-kernel - Re: [PATCH net-next v1 5/9] net: dsa: qca: ar9331: add forwarding database support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 3 Apr 2021 17:25:16 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Oleksij Rempel <o.rempel@...gutronix.de>
Cc:     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>,
        Russell King <linux@...linux.org.uk>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-mips@...r.kernel.org
Subject: Re: [PATCH net-next v1 5/9] net: dsa: qca: ar9331: add forwarding
 database support

> +static int ar9331_sw_port_fdb_rmw(struct ar9331_sw_priv *priv,
> +				  const unsigned char *mac,
> +				  u8 port_mask_set,
> +				  u8 port_mask_clr)
> +{
> +	port_mask = FIELD_GET(AR9331_SW_AT_DES_PORT, f2);
> +	status = FIELD_GET(AR9331_SW_AT_STATUS, f2);
> +	if (status > 0 && status < AR9331_SW_AT_STATUS_STATIC) {
> +		dev_err_ratelimited(priv->dev, "%s: found existing dynamic entry on %x\n",
> +				    __func__, port_mask);
> +
> +		if (port_mask_set && port_mask_set != port_mask)
> +			dev_err_ratelimited(priv->dev, "%s: found existing dynamic entry on %x, replacing it with static on %x\n",
> +					    __func__, port_mask, port_mask_set);
> +		port_mask = 0;
> +	} else if (!status && !port_mask_set) {
> +		return 0;
> +	}

As a generate rule of thumb, use rate limiting where you have no
control of the number of prints, e.g. it is triggered by packet
processing, and there is potentially a lot of them, which could DOS
the box by a remote or unprivileged attacker.

FDB changes should not happen often. Yes, root my be able to DOS the
box by doing bridge fdb add commands in a loop, but only root should
be able to do that.

Plus, i'm not actually sure we should be issuing warnings here. What
does the bridge code do in this case? Is it silent and just does it,
or does it issue a warning?




> +
> +	port_mask_new = port_mask & ~port_mask_clr;
> +	port_mask_new |= port_mask_set;
> +
> +	if (port_mask_new == port_mask &&
> +	    status == AR9331_SW_AT_STATUS_STATIC) {
> +		dev_info(priv->dev, "%s: no need to overwrite existing valid entry on %x\n",
> +				    __func__, port_mask_new);

This one should probably be dev_dbg().

     Andrew