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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <14891624-655b-a71d-dc04-24404e0c2e1a@bang-olufsen.dk>
Date:   Sun, 22 Aug 2021 14:19:44 +0000
From:   Alvin Šipraga <ALSI@...g-olufsen.dk>
To:     Saravana Kannan <saravanak@...gle.com>
CC:     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>,
        Andrew Lunn <andrew@...n.ch>,
        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

Hi Saravana,

Thanks for the follow-up. I tested your change and it does the trick: 
there is no deferral and the PHY driver gets probed first-try during the 
mdiobus registration during the call to dsa_register_switch(). I tested 
with the switch, PHY, and tagging drivers all builtin, or all modules, 
and it worked in both cases.

On 8/20/21 6:52 PM, Saravana Kannan wrote:
> Hi Alvin,
> 
> Can you give this a shot to see if it fixes your issue? It basically
> delays the registration of dsa_register_switch() until all the
> consumers of this switch have probed. So it has a couple of caveats:

Hm, weren't the only consumers the PHYs themselves? It seems like the 
main effect of your change is that - by doing the actual 
dsa_register_switch() call after the switch driver probe - the 
ethernet-switch (provider) is already probed, thereby allowing the PHY 
(consumer) to probe immediately.

> 1. I'm hoping the PHYs are the only consumers of this switch.

In my case that is true, if you count the mdio_bus as well:

/sys/devices/platform/ethernet-switch# ls -l consumer\:*
lrwxrwxrwx    1 root     root             0 Aug 22 16:00 
consumer:mdio_bus:SMI-0 -> 
../../virtual/devlink/platform:ethernet-switch--mdio_bus:SMI-0
lrwxrwxrwx    1 root     root             0 Aug 22 16:00 
consumer:mdio_bus:SMI-0:00 -> 
../../virtual/devlink/platform:ethernet-switch--mdio_bus:SMI-0:00
lrwxrwxrwx    1 root     root             0 Aug 22 16:00 
consumer:mdio_bus:SMI-0:01 -> 
../../virtual/devlink/platform:ethernet-switch--mdio_bus:SMI-0:01
lrwxrwxrwx    1 root     root             0 Aug 22 16:00 
consumer:mdio_bus:SMI-0:02 -> 
../../virtual/devlink/platform:ethernet-switch--mdio_bus:SMI-0:02
lrwxrwxrwx    1 root     root             0 Aug 22 16:00 
consumer:mdio_bus:SMI-0:03 -> 
../../virtual/devlink/platform:ethernet-switch--mdio_bus:SMI-0:03


> 2. All of them have to probe successfully before the switch will
> register itself.

Yes.

> 3. If dsa_register_switch() fails, we can't defer the probe (because
> it already succeeded). But I'm not sure if it's a likely error code.

It's of course possible that dsa_register_switch() fails. Assuming 
fw_devlink is doing its job properly, I think the reason is most likely 
going to be something specific to the driver, such as a communication 
timeout with the switch hardware itself.

I get the impression that you don't necessarily regard this change as a 
proper fix, so I'm happy to do further tests if you choose to 
investigate further.

Kind regards,
Alvin

> 
> -Saravana
> 
> 
> +++ b/drivers/net/dsa/realtek-smi-core.c
> @@ -454,14 +454,16 @@ static int realtek_smi_probe(struct platform_device *pdev)
>          smi->ds->priv = smi;
> 
>          smi->ds->ops = var->ds_ops;
> -       ret = dsa_register_switch(smi->ds);
> -       if (ret) {
> -               dev_err(dev, "unable to register switch ret = %d\n", ret);
> -               return ret;
> -       }
>          return 0;
>   }
> 
> +static void realtek_smi_sync_state(struct device *dev)
> +{
> +       struct realtek_smi *smi = dev_get_drvdata(dev);
> +       if (dsa_register_switch(smi->ds))
> +               dev_err(dev, "unable to register switch ret = %d\n", ret);
> +}
> +
>   static int realtek_smi_remove(struct platform_device *pdev)
>   {
>          struct realtek_smi *smi = dev_get_drvdata(&pdev->dev);
> @@ -492,6 +494,7 @@ static struct platform_driver realtek_smi_driver = {
>          .driver = {
>                  .name = "realtek-smi",
>                  .of_match_table = of_match_ptr(realtek_smi_of_match),
> +               .sync_state = realtek_smi_sync_state,
>          },
>          .probe  = realtek_smi_probe,
>          .remove = realtek_smi_remove,
>