linux-kernel - Re: [PATCH v6 3/3] net: phy: Add gmiitorgmii converter support

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, 16 Aug 2016 18:53:20 +0200
From:	Andrew Lunn <andrew@...n.ch>
To:	Kedareswara rao Appana <appana.durga.rao@...inx.com>
Cc:	robh+dt@...nel.org, mark.rutland@....com, michal.simek@...inx.com,
	soren.brinkmann@...inx.com, appanad@...inx.com,
	f.fainelli@...il.com, punnaia@...inx.com,
	devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH v6 3/3] net: phy: Add gmiitorgmii converter support

> +static int xgmiitorgmii_read_status(struct phy_device *phydev)
> +{
> +	struct gmii2rgmii *priv = phydev->priv;
> +	u16 val = 0;
> +
> +	priv->phy_drv->read_status(phydev);

This can return an error, in which case phydev->speed should not be
trusted.

I've not thought locking all the way through yet. I don't think you
need a lock here, but i need to think about it.

> +
> +	val = mdiobus_read(phydev->mdio.bus, priv->addr, XILINX_GMII2RGMII_REG);

You should check for an error here.

> +	val &= XILINX_GMII2RGMII_SPEED_MASK;
> +
> +	if (phydev->speed == SPEED_1000)
> +		val |= BMCR_SPEED1000;
> +	else if (phydev->speed == SPEED_100)
> +		val |= BMCR_SPEED100;
> +	else
> +		val |= BMCR_SPEED10;

What happens if for example the PHY is an aquantia and has negotiated
SPEED_2500? Some Marvell PHYs can also do odd speeds, like 200Mbps.
You probably want to return an error, rather than silently have things
go wrong.

> +
> +	mdiobus_write(phydev->mdio.bus, priv->addr, XILINX_GMII2RGMII_REG, val);

This can also return an error.

> +	return 0;
> +}
> +
> +int xgmiitorgmii_probe(struct mdio_device *mdiodev)
> +{
> +	struct device *dev = &mdiodev->dev;
> +	struct device_node *np = dev->of_node, *phy_node;
> +	struct gmii2rgmii *priv;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	phy_node = of_parse_phandle(np, "phy-handle", 0);
> +	if (IS_ERR(phy_node)) {
> +		dev_err(dev, "Couldn't parse phy-handle\n");
> +		return -ENODEV;
> +	}
> +
> +	priv->phy_dev = of_phy_find_device(phy_node);
> +	if (!priv->phy_dev) {
> +		dev_info(dev, "Couldn't find phydev\n");
> +		return -EPROBE_DEFER;
> +	}
> +
> +	priv->addr = mdiodev->addr;
> +	priv->phy_drv = priv->phy_dev->drv;
> +	memcpy(&priv->conv_phy_drv, priv->phy_dev->drv,
> +	       sizeof(struct phy_driver));
> +	priv->conv_phy_drv.read_status = xgmiitorgmii_read_status;
> +	priv->phy_dev->priv = priv;
> +	priv->phy_dev->drv = &priv->conv_phy_drv;

So from this point onward, the phy driver depends on the memory
allocated by this driver. If this driver goes away, freeing its
memory, the next call to read_status() is going to have a problem.

Also, i think this assignment should take the phy lock, just to be
safe.

There also needs to be some thought into what happens if the phy
driver is unloaded. Should this driver take a reference on the phy
driver to prevent that?

       Andrew