Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Date:   Mon, 20 Nov 2023 13:42:05 +0000
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Lukas Funke <lukas.funke-oss@weidmueller.com>
Cc:     Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Lukas Funke <lukas.funke@weidmueller.com>,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] net: phy: Fix potential null pointer access
Message-ID: <ZVtiLUpsOQhd2nm+@shell.armlinux.org.uk>
References: <20231120093256.3642327-1-lukas.funke-oss@weidmueller.com>
 <ZVssJrplePACN3of@shell.armlinux.org.uk>
 <2508da6b-a099-4271-a1d0-04cfe5d39daf@weidmueller.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <2508da6b-a099-4271-a1d0-04cfe5d39daf@weidmueller.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Mon, Nov 20, 2023 at 01:38:06PM +0100, Lukas Funke wrote:
> Hi Russel,
> 
> On 20.11.2023 10:51, Russell King (Oracle) wrote:
> > On Mon, Nov 20, 2023 at 10:32:54AM +0100, Lukas Funke wrote:
> > > From: Lukas Funke <lukas.funke@weidmueller.com>
> > > 
> > > When there is no driver associated with the phydev, there will be a
> > > nullptr access. The commit checks if the phydev driver is set before
> > > access.
> > 
> > What's the call path that we encounter a NULL drv pointer?
> 
> 
> The patch is a bit older and the path is reconstructed from my memory:
> 
> macb_phylink_connect -> phylink_of_phy_connect -> of_phy_connect ->
> phy_connect_direct -> phy_request_interrupt
> 
> It happend when we used the Xilinx gmii2rgmii phy driver. We did a
> missconfiguration in the dt and bumped into the nullpointer exception. Since
> other functions like phy_aneg_done() also check for driver existence I
> thought it would be a good addition.

So how does this happen in the path you indicate?

phy_connect_direct() calls phy_attach_direct() before calling
phy_request_interrupt(). If phy_attach_direct() needs to succeed for
us to get to call phy_request_interrupt().

phy_attach_direct() checks to see whether the phydev is bound to a
driver. If it isn't, it binds it to the appropriate genphy driver.
As part of that binding, phydev->drv is guaranteed to be set
(by phy_probe(), which will be called via d->driver->probe() if
using one of the genphy drivers.

You mention using the gmii2rgmii driver, which does mess with
phydev->drv, but I can't see a way in that driver where we would
end up with a NULL pointer there.

I think a bit mroe information is needed to describe how this comes
about - and that needs to go in the commit message, so the reason
for this patch is properly documented.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!