Re: [PATCH net] net: dsa: sja1105: fix use-after-free after calling of_find_compatible_node, or worse

[Saravana Kannan <saravanak@google.com>]

On Thu, Aug 19, 2021 at 6:08 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Thu, Aug 19, 2021 at 6:42 AM Alvin Šipraga <ALSI@bang-olufsen.dk> wrote:
> >
> > Hi Saravana,
> >
> > Thanks for your lucid analysis, it's very much appreciated. Please find
> > my replies inline - but in summary, I don't think there is anything more
> > I can ask of you right now.
>
> You are welcome.
>
> >
> > On 8/19/21 5:28 AM, Saravana Kannan wrote:
> > > On Wed, Aug 18, 2021 at 3:18 AM Alvin Šipraga <ALSI@bang-olufsen.dk> wrote:
> > >>
> > >> Hi Saravana,
> > >>
> > >> On 8/18/21 4:46 AM, Saravana Kannan wrote:
> > >>> On Tue, Aug 17, 2021 at 3:31 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> > >>>>
> > >>>> Hi Alvin,
> > >>>>
> > >>>> On Tue, Aug 17, 2021 at 09:25:28PM +0000, Alvin Šipraga wrote:
> > >>>>> I have an observation that's slightly out of the scope of your patch,
> > >>>>> but I'll post here on the off chance that you find it relevant.
> > >>>>> Apologies if it's out of place.
> > >>>>>
> > >>>>> Do these integrated NXP PHYs use a specific PHY driver, or do they just
> > >>>>> use the Generic PHY driver?
> > >>>>
> > >>>> They refuse to probe at all with the Generic PHY driver. I have been
> > >>>> caught off guard a few times now when I had a kernel built with
> > >>>> CONFIG_NXP_C45_TJA11XX_PHY=n and their probing returns -22 in that case.
> > >>>>
> > >>>>> If the former is the case, do you experience that the PHY driver fails
> > >>>>> to get probed during mdiobus registration if the kernel uses
> > >>>>> fw_devlink=on?
> > >>>>
> > >>>> I don't test with "fw_devlink=on" in /proc/cmdline, this is the first
> > >>>> time I do it. It behaves exactly as you say.
> > >>>>
> > >>>>>
> > >>>>> In my case I am writing a new subdriver for realtek-smi, a DSA driver
> > >>>>> which registers an internal MDIO bus analogously to sja1105, which is
> > >>>>> why I'm asking. I noticed a deferred probe of the PHY driver because the
> > >>>>> supplier (ethernet-switch) is not ready - presumably because all of this
> > >>>>> is happening in the probe of the switch driver. See below:
> > >>>>>
> > >>>>> [   83.653213] device_add:3270: device: 'SMI-0': device_add
> > >>>>> [   83.653905] device_pm_add:136: PM: Adding info for No Bus:SMI-0
> > >>>>> [   83.654055] device_add:3270: device: 'platform:ethernet-switch--mdio_bus:SMI-0': device_add
> > >>>>> [   83.654224] device_link_add:843: mdio_bus SMI-0: Linked as a sync state only consumer to ethernet-switch
> > >>>>> [   83.654291] libphy: SMI slave MII: probed
> > >>>>> ...
> > >>>>> [   83.659809] device_add:3270: device: 'SMI-0:00': device_add
> > >>>>> [   83.659883] bus_add_device:447: bus: 'mdio_bus': add device SMI-0:00
> > >>>>> [   83.659970] device_pm_add:136: PM: Adding info for mdio_bus:SMI-0:00
> > >>>>> [   83.660122] device_add:3270: device: 'platform:ethernet-switch--mdio_bus:SMI-0:00': device_add
> > >>>>> [   83.660274] devices_kset_move_last:2701: devices_kset: Moving SMI-0:00 to end of list
> > >>>>> [   83.660282] device_pm_move_last:203: PM: Moving mdio_bus:SMI-0:00 to end of list
> > >>>>> [   83.660293] device_link_add:859: mdio_bus SMI-0:00: Linked as a consumer to ethernet-switch
> > >>>>> [   83.660350] __driver_probe_device:736: bus: 'mdio_bus': __driver_probe_device: matched device SMI-0:00 with driver RTL8365MB-VC Gigabit Ethernet
> > >>>>> [   83.660365] device_links_check_suppliers:1001: mdio_bus SMI-0:00: probe deferral - supplier ethernet-switch not ready
> > >>>>> [   83.660376] driver_deferred_probe_add:138: mdio_bus SMI-0:00: Added to deferred list
> > >>>>
> > >>>> So it's a circular dependency? Switch cannot finish probing because it
> > >>>> cannot connect to PHY, which cannot probe because switch has not
> > >>>> finished probing, which....
> > >>>
> > >>> Hi Vladimir/Alvin,
> > >>>
> > >>> If there's a cyclic dependency between two devices, then fw_devlink=on
> > >>> is smart enough to notice that. Once it notices a cycle, it knows that
> > >>> it can't tell which one is the real dependency and which one is the
> > >>> false dependency and so stops enforcing ordering between the devices
> > >>> in the cycle.
> > >>>
> > >>> But fw_devlink doesn't understand all the properties yet. Just most of
> > >>> them and I'm always trying to add more. So when it only understands
> > >>> the property that's causing the false dependency but not the property
> > >>> that causes the real dependency, it can cause issues like this where
> > >>> fw_devlink=on enforces the false dependency and the driver/code
> > >>> enforces the real dependency. These are generally easy to fix -- you
> > >>> just need to teach fw_devlink how to parse more properties.
> > >>>
> > >>> This is just a preliminary analysis since I don't have all the info
> > >>> yet -- so I could be wrong. With that said, I happened to be working
> > >>> on adding fw_devlink support for phy-handle property and I think it
> > >>> should fix your issue with fw_devlink=on. Can you give [1] a shot?
> > >>
> > >> I tried [1] but it did not seem to have any effect.
> > >>
> > >>>
> > >>> If it doesn't fix it, can one of you please point me to an upstream
> > >>> dts (not dtsi) file for a platform in which you see this issue? And
> > >>> ideally also the DT nodes and their drivers that are involved in this
> > >>> cycle? With that info, I should be able to root cause this if the
> > >>> patch above doesn't already fix it.
> > >>
> > >> I'm working with a non-upstream dts - maybe Vladimir is using an
> > >> upstream one? The pattern among the drivers/dts is common between our
> > >> two cases.
> > >
> > > Ideally, I can get a fully upstream example where this issue is
> > > happening so that I can look at the actual code that's hitting this
> > > issue and be sure my analysis is right.
> >
> > Due to NDA issues at work, I am unable to publish the DTS right now. But
> > based on your analysis below, I believe that the problem I am
> > experiencing is exactly for the reasons you describe.
> >
> > >
> > >>
> > >> But for the sake of this discussion, my dts is pretty much the same as
> > >> what you will find in arch/arm/boot/dts/gemini-dlink-dir-685.dts. The
> > >> nodes of interest from that dts file are below, and the driver is in
> > >> drivers/net/ds/{realtek-smi-core.c,rtl8366rb.c}. It's expected that the
> > >> Realtek PHY driver in drivers/net/phy/realtek.c will get probed as part
> > >> of the mdiobus registration, but that never happens. See my previous
> > >> reply for a debug log.
> > >
> > > Your DTS might be similar to this, but the driver code also matters
> > > for me to be sure. Anyway, I took a look at this, but my analysis
> > > below is going to be sketchy because I'm not looking at the actual
> > > code that's reproducing this issue.
> >
> > The driver code I am working with is pretty much the same in the probe
> > path (the driver is modelled on rtl8366rb.c and hooks into
> > realtek-smi-core.c), so it was not a waste of your time to analyze the
> > upstream case. Thanks a lot.
>
> Good to know it's useful.
>
> >
> > >
> > > Assuming this issue actually happens with the example you pointed to
> > > (I don't know this yet), here's what is happening:
> > >
> > > The main problem is that the parent device switch seems to be assuming
> > > it's child/grandchild devices (mdiobus/PHYs) will have probed
> > > successfully as soon as they are added. This assumption is not true
> > > and can be broken for multiple reasons such as:
> > >
> > > 1. The driver for the child devices (PHYs in this case) could be
> > > loaded as a module after the parent (switch) is probed. So when the
> > > devices are added, the PHYs would not be probed.
> > > 2. The child devices could defer probe because one of their suppliers
> > > isn't ready yet. Either because of fw_devlink=on or the framework
> > > itself returning -EPROBE_DEFER.
> > > 3. The child devices could be getting probed asynchronously. So the
> > > device_add() would kick off a thread to probe the child devices in a
> > > separate thread.
> > >
> > > (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.
> >
> > Right, that makes perfect sense.
> >
> > >
> > > I don't fully understand the networking frameworks, but I think
> > > Vladimir might have a point in his earlier reply [1]. If you can make
> > > the switch driver not assume its child PHYs are ready during the
> > > switch's probe and instead have the switch check if the PHYs are ready
> > > when the switch is "opened" that'd be better.
> >
> > I am pretty new to the DSA subsystem,
>
> I know absolutely nothing about DSA. Heck, I don't even know what it
> stands for. It's just about as informative as "XYZ" to me :)
>
> > but it appears to me that DSA is
> > making this assumption: I don't think DSA switches have a concept of
> > "open", as everything is handled in the dsa_register_switch() call in a
> > DSA driver's probe function. And as Vladimir pointed out, this is also
> > where phylink_of_phy_connect() is being called.
>
> Yeah, I noticed that part.
>
> > A quick look at the
> > other DSA drivers suggests that mv88e6xxx may also suffer from case (2)
> > above, so it seems like a more general issue.
> >
>
> I obviously want fw_devlink=on to be used by everyone and not have
> people use fw_devlink=permissive. In this scenario, I think
> fw_devlink=on is doing the right thing too. So this is where I'm
> hoping the network experts can jump in and fix the general DSA issue
> and I can help with the parts I understand.

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:
1. I'm hoping the PHYs are the only consumers of this switch.
2. All of them have to probe successfully before the switch will
register itself.
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.

But, for now, I want to see if it fixes your issues.

-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,