On Tue, 29 Apr 2025 11:00:19 +0200
Paolo Abeni <pabeni@xxxxxxxxxx> wrote:
> On 4/22/25 4:56 PM, Kory Maincent wrote:
> > +/**
> > + * pse_control_find_phy_by_id - Find PHY attached to the pse control id
> > + * @pcdev: a pointer to the PSE
> > + * @id: index of the PSE control
> > + *
> > + * Return: PHY device pointer or NULL
> > + */
> > +static struct phy_device *
> > +pse_control_find_phy_by_id(struct pse_controller_dev *pcdev, int id)
> > +{
> > + struct pse_control *psec;
> > +
> > + mutex_lock(&pse_list_mutex);
> > + list_for_each_entry(psec, &pcdev->pse_control_head, list) {
> > + if (psec->id == id) {
> > + mutex_unlock(&pse_list_mutex);
>
> AFAICS at this point 'psec' could be freed and the next statement could
> cause UaF.
>
> It looks like you should acquire a reference to the pse control?
Oh indeed, thanks for spotting this issue!
I first though this would be sufficient:
phydev = psec->attached_phydev;
mutex_unlock(&pse_list_mutex);
return phydev;
But in fact the ethnl_pse_send_ntf(phydev, notifs, &extack) is called without
RTNL lock, therefore the phydev could be freed and we could also have an UaF on
the phydev pointer. I will add a rtnl_lock here. Maybe a
get_device(&phydev->mdio.dev) is sufficient but not sure.
Also we really need to throw the ASSERT_RTNL() in the phy_detach function,
because there are still paths where the phy is freed without rtnl. I have a
patch for it, I will try to send it in RFC soon.
Regards,
--
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
