netdev - Re: [PATCH net-next 0/4] net: dsa: b53: Clean up CPU/IMP ports

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:   Fri, 17 Sep 2021 15:31:08 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Rafał Miłecki <zajec5@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        bcm-kernel-feedback-list@...adcom.com,
        Rafał Miłecki <rafal@...ecki.pl>
Subject: Re: [PATCH net-next 0/4] net: dsa: b53: Clean up CPU/IMP ports

On Fri, Sep 17, 2021 at 02:21:16PM +0200, Andrew Lunn wrote:
> > > That DSA_PORT_TYPE_UNUSED would probably require investigating DSA & b53
> > > behaviour *and* discussing it with DSA maintainer to make sure we don't
> > > abuse that.
> >
> > How absent are these ports in hardware? For DSA_PORT_TYPE_UNUSED we do
> > register a devlink port, but if those ports are really not present in
> > hardware, I'm thinking maybe the easiest way would be to supply a
> > ds->disabled_port_mask before dsa_register_switch(), and DSA will simply
> > skip those ports when allocating the dp, the devlink_port etc. So you
> > will literally have nothing for them.
>
> The basic idea seems O.K, we just need to be careful.
>
> We have code like:
>
> static inline bool dsa_is_dsa_port(struct dsa_switch *ds, int p)
> {
> 	return dsa_to_port(ds, p)->type == DSA_PORT_TYPE_DSA;
> }
>
> static inline bool dsa_is_user_port(struct dsa_switch *ds, int p)
> {
> 	return dsa_to_port(ds, p)->type == DSA_PORT_TYPE_USER;
> }
>
> dsa_to_port(ds, p) will return NULL, and then bad things will happen.
>
> Maybe it would be safer to add DSA_PORT_TYPE_PHANTOM and do allocate
> the dp?

I think DSA is not yet large enough for us to take the defeatist
position of inserting bits of code that simply protect from other bits
of code.

Especially after these patches:
https://patchwork.kernel.org/project/netdevbpf/cover/20210810161448.1879192-1-vladimir.oltean@nxp.com/
the DSA core should basically never iterate over ports using
	for (port = 0; port < ds->num_ports; port++)
but instead using
	dsa_switch_for_each_port(dp, ds)
which basically ensures that we never dereference dsa_to_port(ds, p) for
a p with no associated dp (the "dp" from "dsa_switch_for_each_port" is
taken from a list).

Conversely, driver code is "mostly" event-driven, so DSA should only call
dsa_switch_ops methods for ports where the dp structure does exist.
What we would need to audit are just the drivers which set ds->disabled_port_mask
(b53, maybe ksz switches?). I guess that would be on the developer who
makes the change in those respective drivers to ensure that the "dp"
which is searched for does in fact exist.

My 2 cents anyway, I might be missing some trickier cases.