netdev - Re: [PATCH net] net: lan966x: fix kernel oops on ioctl when I/F is down

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-next>] [day] [month] [year] [list]

Message-ID: <1ee5aec079e3eefff8475017ff1044bf@walle.cc>
Date:   Sat, 26 Mar 2022 14:59:53 +0100
From:   Michael Walle <michael@...le.cc>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Horatiu Vultur <horatiu.vultur@...rochip.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, UNGLinuxDriver@...rochip.com,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] net: lan966x: fix kernel oops on ioctl when I/F is
 down

Am 2022-03-26 03:17, schrieb Andrew Lunn:
> On Sat, Mar 26, 2022 at 01:02:51AM +0100, Michael Walle wrote:
>> A SIOCGMIIPHY ioctl will cause a kernel oops when the interface is 
>> down.
>> Fix it by checking the state and if it's no running, return an error.
> 
> s/no/not/
> 
> I don't think it is just SIOCGMIIPHY. phy_has_hwtstamp(dev->phydev) is
> probably also an issue. The phy is connected in open, and disconnected
> in stop. So dev->phydev is not valid outside of that time.

phy_has_hwtstamp() handles NULL gracefully. And I guess the MAC 
timestamp
handling is working if there is no phydev. Not sure if the interface
has to be up though.

> But i'm also not sure it is guaranteed to be valid while the interface
> is up. The driver uses phylink, so there could be an SFP attached to a
> port, in which case, dev->phydev will not be set.

I wonder if we should use phylink_mii_ioctl() here. Maybe as a seperate
patch for the net-next if its open again?

> So rather than testing of running, it would be better to test if the
> phydev is NULL or not.

What about the following:

static int lan966x_port_ioctl(struct net_device *dev, struct ifreq *ifr,
			     int cmd)
{
	struct lan966x_port *port = netdev_priv(dev);

	if (!phy_has_hwtstamp(dev->phydev) && port->lan966x->ptp) {
		switch (cmd) {
		case SIOCSHWTSTAMP:
			return lan966x_ptp_hwtstamp_set(port, ifr);
		case SIOCGHWTSTAMP:
			return lan966x_ptp_hwtstamp_get(port, ifr);
		}
	}

	if (!dev->phydev)
		return -ENODEV;

	return phy_mii_ioctl(dev->phydev, ifr, cmd);
}

-michael