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

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]

Date:   Tue, 5 May 2020 15:19:36 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Michal Kubecek <mkubecek@...e.cz>
Cc:     David Miller <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Chris Healy <cphealy@...il.com>, michael@...le.cc
Subject: Re: [PATCH net-next v2 06/10] net: ethtool: Add infrastructure for
 reporting cable test results

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