netdev - Re: [PATCH] net: phy: micrel: Remove unnecessary comparison in lan8814_handle

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:   Thu, 5 May 2022 14:47:51 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Jiabing Wan <wanjiabing@...o.com>
Cc:     Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net: phy: micrel: Remove unnecessary comparison in
 lan8814_handle_interrupt

> Yes, I actually check the lanphy_read_page_reg and I notice 'data' is
> declared
> as a 'u32' variable. So I think the comparison is meaningless. But the
> return type is int.
> 
> 1960  static int lanphy_read_page_reg(struct phy_device *phydev, int page,
> u32 addr)
> 1961  {
> 1962      u32 data;
> 1963
> 1964      phy_lock_mdio_bus(phydev);
> 1965      __phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, page);
> 1966      __phy_write(phydev, LAN_EXT_PAGE_ACCESS_ADDRESS_DATA, addr);
> 1967      __phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL,
> 1968              (page | LAN_EXT_PAGE_ACCESS_CTRL_EP_FUNC));
> 1969      data = __phy_read(phydev, LAN_EXT_PAGE_ACCESS_ADDRESS_DATA);
> 1970      phy_unlock_mdio_bus(phydev);
> 1971
> 1972      return data;
> 1973  }
> > 
> > So the real problem here is, tsu_irq_status is defined as u16, when in
> > fact it should be an int.
> 
> Should the 'data' in lanphy_read_page_reg be declared by 'int'?

Yes.

Another one of those learning over time. If you find a bug, look
around and you will probably find the same bug in other places nearby.

This is actually a pretty common issue we have with Ethernet PHY
drivers, the sign bit getting thrown away. Developers look at the
datasheet and see 16 bit registers, and so use u16, and forget about
the error code. Maybe somebody can write a coccicheck script looking
for calls to and of the phy_read() variants and the result value is
assigned to an unsigned int?

> Finally, I also find other variable, for example, 'u16 addr' in
> lan8814_probe.
> I think they all should be declared by 'int'.

addr should never be used as a return type, so can never carry an
error code. Also, PHYs only have 32 registers, so address is never
greater than 0x1f. So this is O.K.

	Andrew