netdev - Re: [PATCH net] net: dsa: sja1105: fix use-after-free after calling of_find_compatible

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 for Android: free password hash cracker in your pocket

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

Message-ID: <YR5eMeKzcuYtB6Tk@lunn.ch>
Date:   Thu, 19 Aug 2021 15:35:45 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Saravana Kannan <saravanak@...gle.com>
Cc:     Alvin Šipraga <ALSI@...g-olufsen.dk>,
        Vladimir Oltean <olteanv@...il.com>,
        Vladimir Oltean <vladimir.oltean@....com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Frank Rowand <frowand.list@...il.com>,
        Rob Herring <robh+dt@...nel.org>
Subject: Re: [PATCH net] net: dsa: sja1105: fix use-after-free after calling
 of_find_compatible_node, or worse

> (2) is what is happening in this case. fw_devlink=on sees that
> "switch" implements the "switch_intc" and "switch" hasn't finished
> probing yet. So it has no way of knowing that switch_intc is actually
> ready. And even if switch_intc was registered as part of switch's
> probe() by the time the PHYs are added, switch_intc could get
> deregistered if the probe fails at a later point. So until probe()
> returns 0, fw_devlink can't be fully sure the supplier (switch_intc)
> is ready. Which is good in general because you won't have to
> forcefully unbind (if that is even handled correctly in the first
> place) the consumers of a device if it fails probe() half way through
> registering a few services.

There are actually a few different circular references with the way
switches work. Take for example:

&fec1 {
        phy-mode = "rmii";
        pinctrl-names = "default";
        pinctrl-0 = <&pinctrl_fec1>;
        status = "okay";

        fixed-link {
                speed = <100>;
                full-duplex;
        };

        mdio1: mdio {
                #address-cells = <1>;
                #size-cells = <0>;
                clock-frequency = <12500000>;
                suppress-preamble;
                status = "okay";

                switch0: switch0@0 {
                        compatible = "marvell,mv88e6190";
                        pinctrl-0 = <&pinctrl_gpio_switch0>;
                        pinctrl-names = "default";
                        reg = <0>;
                        eeprom-length = <65536>;
                        interrupt-parent = <&gpio3>;
                        interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
                        interrupt-controller;
                        #interrupt-cells = <2>;

                        ports {
                                #address-cells = <1>;
                                #size-cells = <0>;

                                port@0 {
                                        reg = <0>;
                                        label = "cpu";
                                        ethernet = <&fec1>;

                                        fixed-link {
                                                speed = <100>;
                                                full-duplex;
                                        };
                                };

FEC is an ethernet controller. It has an MDIO bus, and on the bus is
an Ethernet switch. port 0 of the Ethernet switch is connected to the
FEC ethernet controller.

While the FEC probes, it will at some point register its MDIO bus. At
that point, the MDIO bus is probed, the switch is found, and
registered with the switch core. The switch core looks for the port
with an ethernet property and goes looking for that ethernet
interface. But that this point in time, the FEC probe has only got as
far as registering the MDIO bus. The interface itself is not
registered. So finding the interface fails, and we go into
EPROBE_DEFER for probing the switch.

It is pretty hard to solve. An Ethernet interface can be used by the
kernel itself, e.g. NFS root. At the point you call register_netdev()
in the probe function, to register the interface with the core, it
needs to be fully ready to go.  The networking stack can start using
the interface before register_netdev() even returns. So you cannot
first register the interface and then register the MDIO bus.

I once looked to see if it was possible to tell the driver core to not
even bother probing a bus as soon as it is registered, go straight to
defer probe handling. Because this is one case we know it cannot
work. But it does not seem possible.

      Andrew