Re: [PATCH net-next v2 06/10] net: ethtool: Add infrastructure for reporting cable test results

[Andrew Lunn <andrew@lunn.ch>]

> > +int ethnl_cable_test_alloc(struct phy_device *phydev)
> > +{
> > +	int err = -ENOMEM;
> > +
> > +	phydev->skb = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
> > +	if (!phydev->skb)
> > +		goto out;
> > +
> > +	phydev->ehdr = ethnl_bcastmsg_put(phydev->skb,
> > +					  ETHTOOL_MSG_CABLE_TEST_NTF);
> > +	if (!phydev->ehdr) {
> > +		err = -EINVAL;
> 
> This should be -EMSGSIZE.
> 
> > +		goto out;
> > +	}
> > +
> > +	err = ethnl_fill_reply_header(phydev->skb, phydev->attached_dev,
> > +				      ETHTOOL_A_CABLE_TEST_NTF_HEADER);
> > +	if (err)
> > +		goto out;
> > +
> > +	err = nla_put_u8(phydev->skb, ETHTOOL_A_CABLE_TEST_NTF_STATUS,
> > +			 ETHTOOL_A_CABLE_TEST_NTF_STATUS_COMPLETED);
> > +	if (err)
> > +		goto out;
> > +
> > +	phydev->nest = nla_nest_start(phydev->skb,
> > +				      ETHTOOL_A_CABLE_TEST_NTF_NEST);
> > +	if (!phydev->nest)
> > +		goto out;
> 
> If nla_nest_start() fails, we still have 0 in err.
> 
> > +
> > +	return 0;
> > +
> > +out:
> > +	nlmsg_free(phydev->skb);
> > +	return err;
> > +}
> > +EXPORT_SYMBOL_GPL(ethnl_cable_test_alloc);
> 
> Do you think it would make sense to set phydev->skb to NULL on failure
> and also in ethnl_cable_test_free() and ethnl_cable_test_finished() so
> that if driver messes up, it hits null pointer dereference which is much
> easier to debug than use after free?

Hi Michal

The use after free is not that hard to debug, i had to do it myself :-)

But yes, i can poison phydev->skb.

    Andrew