| 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: <20260119224805.hjvh5xdjfhd6c6kf@skbuf>
Date: Tue, 20 Jan 2026 00:48:05 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Linus Walleij <linusw@...nel.org>
Cc: Andrew Lunn <andrew@...n.ch>, "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Woojung Huh <woojung.huh@...rochip.com>,
UNGLinuxDriver@...rochip.com, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2 4/4] net: dsa: ks8995: Implement port
isolation
On Mon, Jan 19, 2026 at 03:30:08PM +0100, Linus Walleij wrote:
> It is unsound to not have proper port isolation on a
> switch which supports it.
>
> Set each port as isolated by default in the setup callback
> and de-isolate and isolate the ports in the bridge join/leave
> callbacks.
>
> Fixes: a7fe8b266f65 ("net: dsa: ks8995: Add basic switch set-up")
> Reported-by: Vladimir Oltean <olteanv@...il.com>
> Signed-off-by: Linus Walleij <linusw@...nel.org>
> ---
> drivers/net/dsa/ks8995.c | 131 ++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 129 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/dsa/ks8995.c b/drivers/net/dsa/ks8995.c
> index 060bc8303a14..574e14743a36 100644
> --- a/drivers/net/dsa/ks8995.c
> +++ b/drivers/net/dsa/ks8995.c
> @@ -80,6 +80,11 @@
> #define KS8995_PC0_TAG_REM BIT(1) /* Enable tag removal on port */
> #define KS8995_PC0_PRIO_EN BIT(0) /* Enable priority handling */
>
> +#define KS8995_PC1_SNIFF_PORT BIT(7) /* This port is a sniffer port */
> +#define KS8995_PC1_RCV_SNIFF BIT(6) /* Packets received goes to sniffer port(s) */
> +#define KS8995_PC1_XMIT_SNIFF BIT(5) /* Packets transmitted goes to sniffer port(s) */
> +#define KS8995_PC1_PORT_VLAN GENMASK(4, 0) /* Port isolation mask */
> +
> #define KS8995_PC2_TXEN BIT(2) /* Enable TX on port */
> #define KS8995_PC2_RXEN BIT(1) /* Enable RX on port */
> #define KS8995_PC2_LEARN_DIS BIT(0) /* Disable learning on port */
> @@ -441,6 +446,44 @@ dsa_tag_protocol ks8995_get_tag_protocol(struct dsa_switch *ds,
>
> static int ks8995_setup(struct dsa_switch *ds)
> {
> + struct ks8995_switch *ks = ds->priv;
> + int ret;
> + u8 val;
> + int i;
> +
> + /* Isolate all user ports so they can only send packets to itself and the CPU port */
I would refrain from using "isolation" related terminology for user port
separation, because of the naming collision with the BR_ISOLATED bridge
port flag (from "man bridge", isolated bridge ports "will be able to
communicate with non-isolated ports only").
> + for (i = 0; i < KS8995_CPU_PORT; i++) {
> + ret = ks8995_read_reg(ks, KS8995_REG_PC(i, KS8995_REG_PC1), &val);
> + if (ret) {
> + dev_err(ks->dev, "failed to read KS8995_REG_PC1 on port %d\n", i);
> + return ret;
> + }
> +
> + val &= ~KS8995_PC1_PORT_VLAN;
> + val |= (BIT(i) | BIT(KS8995_CPU_PORT));
> +
> + ret = ks8995_write_reg(ks, KS8995_REG_PC(i, KS8995_REG_PC1), val);
Do you actually need to perform a register read at probe time, or could
you just call ks8995_write_reg() with known good values for the sniff
port bits too?
> + if (ret) {
> + dev_err(ks->dev, "failed to write KS8995_REG_PC1 on port %d\n", i);
> + return ret;
> + }
> + }
> +
> + /* The CPU port should be able to talk to all ports */
> + ret = ks8995_read_reg(ks, KS8995_REG_PC(KS8995_CPU_PORT, KS8995_REG_PC1), &val);
> + if (ret) {
> + dev_err(ks->dev, "failed to read KS8995_REG_PC1 on CPU port\n");
> + return ret;
> + }
> +
> + val |= KS8995_PC1_PORT_VLAN;
Writing this value enables hairpinning (reflection of forwarded traffic)
on the CPU port, because KS8995_PC1_PORT_VLAN (GENMASK(4, 0)) includes
KS8995_CPU_PORT (4).
> +
> + ret = ks8995_write_reg(ks, KS8995_REG_PC(KS8995_CPU_PORT, KS8995_REG_PC1), val);
> + if (ret) {
> + dev_err(ks->dev, "failed to write KS8995_REG_PC1 on CPU port\n");
> + return ret;
> + }
> +
> return 0;
> }
>
> @@ -466,8 +509,44 @@ static int ks8995_port_bridge_join(struct dsa_switch *ds, int port,
> bool *tx_fwd_offload,
> struct netlink_ext_ack *extack)
> {
> + struct ks8995_switch *ks = ds->priv;
> + u8 port_bitmap = 0;
> + int ret;
> + u8 val;
> + int i;
> +
> + /* De-isolate this port from any other port on the bridge */
> + port_bitmap |= BIT(port);
A bit strange to unconditionally modify the initialization value of a
variable rather than just assign BIT(port) as its initializer.
> + for (i = 0; i < KS8995_CPU_PORT; i++) {
> + if (i == port)
> + continue;
> + if (!dsa_port_offloads_bridge(dsa_to_port(ds, i), &bridge))
> + continue;
dsa_to_port() has an embedded loop over ports inside, so actually this
loop iterates ds->num_ports^2 times. It is recommended that you use the
dsa_switch_for_each_user_port() iterator and that gives you "dp"
directly (i becomes dp->index).
> + port_bitmap |= BIT(i);
> + }
> +
> + /* Update all affected ports with the new bitmask */
> + for (i = 0; i < KS8995_CPU_PORT; i++) {
> + if (!dsa_port_offloads_bridge(dsa_to_port(ds, i), &bridge))
> + continue;
> +
> + ret = ks8995_read_reg(ks, KS8995_REG_PC(i, KS8995_REG_PC1), &val);
> + if (ret) {
> + dev_err(ks->dev, "failed to read KS8995_REG_PC1 on port %d\n", i);
> + return ret;
> + }
> +
> + val |= port_bitmap;
Same hairpinning problem. When a new port joins a bridge, the existing
ports start enabling forwarding to themselves as well (the port_bitmap
written to port i contains BIT(i) set).
> +
> + ret = ks8995_write_reg(ks, KS8995_REG_PC(i, KS8995_REG_PC1), val);
> + if (ret) {
> + dev_err(ks->dev, "failed to write KS8995_REG_PC1 on port %d\n", i);
> + return ret;
> + }
> + }
> +
> /* port_stp_state_set() will be called after to put the port in
> - * appropriate state so there is no need to do anything.
> + * appropriate state.
> */
>
> return 0;
> @@ -476,8 +555,56 @@ static int ks8995_port_bridge_join(struct dsa_switch *ds, int port,
> static void ks8995_port_bridge_leave(struct dsa_switch *ds, int port,
> struct dsa_bridge bridge)
> {
> + struct ks8995_switch *ks = ds->priv;
> + u8 port_bitmap = 0;
> + int ret;
> + u8 val;
> + int i;
> +
> + /* Isolate this port from any other port on the bridge */
> + for (i = 0; i < KS8995_CPU_PORT; i++) {
> + /* Current port handled last */
> + if (i == port)
> + continue;
> + /* Not on this bridge */
> + if (!dsa_port_offloads_bridge(dsa_to_port(ds, i), &bridge))
> + continue;
> +
> + ret = ks8995_read_reg(ks, KS8995_REG_PC(i, KS8995_REG_PC1), &val);
> + if (ret) {
> + dev_err(ks->dev, "failed to read KS8995_REG_PC1 on port %d\n", i);
> + return;
> + }
> +
> + val &= ~BIT(port);
> +
> + ret = ks8995_write_reg(ks, KS8995_REG_PC(i, KS8995_REG_PC1), val);
> + if (ret) {
> + dev_err(ks->dev, "failed to write KS8995_REG_PC1 on port %d\n", i);
> + return;
> + }
> +
> + /* Accumulate this port for access by current */
> + port_bitmap |= BIT(i);
> + }
> +
> + /* Isolate this port from all other ports formerly on the bridge */
> + ret = ks8995_read_reg(ks, KS8995_REG_PC(port, KS8995_REG_PC1), &val);
> + if (ret) {
> + dev_err(ks->dev, "failed to read KS8995_REG_PC1 on port %d\n", port);
> + return;
> + }
> +
> + val &= ~port_bitmap;
> +
> + ret = ks8995_write_reg(ks, KS8995_REG_PC(port, KS8995_REG_PC1), val);
> + if (ret) {
> + dev_err(ks->dev, "failed to write KS8995_REG_PC1 on port %d\n", port);
> + return;
> + }
> +
The register layout seems identical with the one from ksz8_cfg_port_member(),
but that being said, I don't think I have a problem with the KS8995
driver continuing to be maintained separately (for now).
> /* port_stp_state_set() will be called after to put the port in
> - * forwarding state so there is no need to do anything.
> + * forwarding state.
> */
> }
>
>
> --
> 2.52.0
>
Powered by blists - more mailing lists