netdev - Re: [PATCH v2 net 4/4] net: dsa: mv88e6xxx: Limit rsvd2cpu policy to user ports on 6393X

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]

Message-ID: <20241219142915.didcxxlvxvnfhori@skbuf>
Date: Thu, 19 Dec 2024 16:29:15 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Tobias Waldekranz <tobias@...dekranz.com>
Cc: davem@...emloft.net, kuba@...nel.org, andrew@...n.ch,
	f.fainelli@...il.com, netdev@...r.kernel.org, linux@...linux.org.uk,
	chris.packham@...iedtelesis.co.nz, pabeni@...hat.com
Subject: Re: [PATCH v2 net 4/4] net: dsa: mv88e6xxx: Limit rsvd2cpu policy to
 user ports on 6393X

On Thu, Dec 19, 2024 at 01:30:43PM +0100, Tobias Waldekranz wrote:
> For packets with a DA in the IEEE reserved L2 group range, originating
> from a CPU, forward it as normal, rather than classifying it as
> management.
> 
> Example use-case:
> 
>      bridge (group_fwd_mask 0x4000)
>      / |  \
>  swp1 swp2 tap0
>    \   /
> (mv88e6xxx)
> 
> We've created a bridge with a non-zero group_fwd_mask (allowing LLDP
> in this example) containing a set of ports managed by mv88e6xxx and
> some foreign interface (e.g. an L2 VPN tunnel).
> 
> Since an LLDP packet coming in to the bridge from the other side of
> tap0 is eligable for tx forward offloading, a FORWARD frame destined
> for swp1 and swp2 would be send to the conduit interface.
> 
> Before this change, due to rsvd2cpu being enabled on the CPU port, the
> switch would try to trap it back to the CPU. Given that the CPU is
> trusted, instead assume that it indeed meant for the packet to be
> forwarded like any other.
> 
> Signed-off-by: Tobias Waldekranz <tobias@...dekranz.com>

Is it fair to say that commit d82f8ab0d874 ("net: dsa: tag_dsa: offload
the bridge forwarding process") broke this and that we need a Fixes: tag
for that and not earlier? Prior to that, I believe that rsvd2cpu would
not affect these forwarded reserved L2 multicast groups, because they
were sent with FROM_CPU.

> ---
>  drivers/net/dsa/mv88e6xxx/port.c | 31 +++++++++++++++++++++++++------
>  1 file changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
> index 56ed2f57fef8..bf6d558c112c 100644
> --- a/drivers/net/dsa/mv88e6xxx/port.c
> +++ b/drivers/net/dsa/mv88e6xxx/port.c
> @@ -1416,6 +1416,23 @@ static int mv88e6393x_port_policy_write_all(struct mv88e6xxx_chip *chip,
>  	return 0;
>  }
>  
> +static int mv88e6393x_port_policy_write_user(struct mv88e6xxx_chip *chip,
> +					     u16 pointer, u8 data)
> +{
> +	int err, port;
> +
> +	for (port = 0; port < mv88e6xxx_num_ports(chip); port++) {
> +		if (!dsa_is_user_port(chip->ds, port))
> +			continue;

Can you remember to convert this "dsa_to_port() called in a loop"
antipattern to dsa_switch_for_each_user_port() in net-next? I wanted to
ask you to do it now, but the blamed commit is in kernel 5.15, and
82b318983c51 ("net: dsa: introduce helpers for iterating through ports
using dp") made its appearance in 5.16.

> +
> +		err = mv88e6393x_port_policy_write(chip, port, pointer, data);
> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}
> +
>  int mv88e6393x_set_egress_port(struct mv88e6xxx_chip *chip,
>  			       enum mv88e6xxx_egress_direction direction,
>  			       int port)
> @@ -1457,26 +1474,28 @@ int mv88e6393x_port_mgmt_rsvd2cpu(struct mv88e6xxx_chip *chip)
>  	int err;
>  
>  	/* Consider the frames with reserved multicast destination
> -	 * addresses matching 01:80:c2:00:00:00 and
> -	 * 01:80:c2:00:00:02 as MGMT.
> +	 * addresses matching 01:80:c2:00:00:00 and 01:80:c2:00:00:02

Is the comment correct? LLDP is group 01:80:c2:00:00:0e. It doesn't make
it sound like what is done here should affect it.

> +	 * as MGMT when received on user ports. Forward as normal on
> +	 * CPU/DSA ports, to support bridges with non-zero
> +	 * group_fwd_masks.
>  	 */
>  	ptr = MV88E6393X_PORT_POLICY_MGMT_CTL_PTR_01C280000000XLO;
> -	err = mv88e6393x_port_policy_write_all(chip, ptr, 0xff);
> +	err = mv88e6393x_port_policy_write_user(chip, ptr, 0xff);
>  	if (err)
>  		return err;
>  
>  	ptr = MV88E6393X_PORT_POLICY_MGMT_CTL_PTR_01C280000000XHI;
> -	err = mv88e6393x_port_policy_write_all(chip, ptr, 0xff);
> +	err = mv88e6393x_port_policy_write_user(chip, ptr, 0xff);
>  	if (err)
>  		return err;
>  
>  	ptr = MV88E6393X_PORT_POLICY_MGMT_CTL_PTR_01C280000002XLO;
> -	err = mv88e6393x_port_policy_write_all(chip, ptr, 0xff);
> +	err = mv88e6393x_port_policy_write_user(chip, ptr, 0xff);
>  	if (err)
>  		return err;
>  
>  	ptr = MV88E6393X_PORT_POLICY_MGMT_CTL_PTR_01C280000002XHI;
> -	err = mv88e6393x_port_policy_write_all(chip, ptr, 0xff);
> +	err = mv88e6393x_port_policy_write_user(chip, ptr, 0xff);
>  	if (err)
>  		return err;
>  
> -- 
> 2.43.0
>