linux-kernel - Re: [PATCH net-next v1 1/6] net: usb: lan78xx: Add error handling to lan78xx_get

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]

Message-ID: <c7ddac7d-debc-44eb-9c43-7746ad3edb55@intel.com>
Date: Mon, 16 Dec 2024 14:07:37 +0100
From: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
To: Oleksij Rempel <o.rempel@...gutronix.de>, "David S. Miller"
	<davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
	<kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Woojung Huh
	<woojung.huh@...rochip.com>, Andrew Lunn <andrew+netdev@...n.ch>
CC: <kernel@...gutronix.de>, <linux-kernel@...r.kernel.org>,
	<netdev@...r.kernel.org>, <UNGLinuxDriver@...rochip.com>, Phil Elwell
	<phil@...pberrypi.org>
Subject: Re: [PATCH net-next v1 1/6] net: usb: lan78xx: Add error handling to
 lan78xx_get_regs



On 12/16/2024 1:09 PM, Oleksij Rempel wrote:
> Update `lan78xx_get_regs` to handle errors during register and PHY
> reads. Log warnings for failed reads and exit the function early if an
> error occurs. Drop all previously logged registers to signal
> inconsistent readings to the user space. This ensures that invalid data
> is not returned to users.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@...gutronix.de>
> ---
>   drivers/net/usb/lan78xx.c | 36 ++++++++++++++++++++++++++++++------
>   1 file changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index 4661d131b190..270345fcad65 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -2108,20 +2108,44 @@ static void
>   lan78xx_get_regs(struct net_device *netdev, struct ethtool_regs *regs,
>   		 void *buf)
>   {
> -	u32 *data = buf;
> -	int i, j;
>   	struct lan78xx_net *dev = netdev_priv(netdev);
> +	unsigned int data_count = 0;
> +	u32 *data = buf;
> +	int i, j, ret;
>   
>   	/* Read Device/MAC registers */
> -	for (i = 0; i < ARRAY_SIZE(lan78xx_regs); i++)
> -		lan78xx_read_reg(dev, lan78xx_regs[i], &data[i]);
> +	for (i = 0; i < ARRAY_SIZE(lan78xx_regs); i++) {
> +		ret = lan78xx_read_reg(dev, lan78xx_regs[i], &data[i]);
> +		if (ret < 0) {
> +			netdev_warn(dev->net,
> +				    "failed to read register 0x%08x\n",
> +				    lan78xx_regs[i]);
> +			goto clean_data;
> +		}
> +
> +		data_count++;
> +	}
>   
>   	if (!netdev->phydev)
>   		return;
>   
>   	/* Read PHY registers */
> -	for (j = 0; j < 32; i++, j++)
> -		data[i] = phy_read(netdev->phydev, j);
> +	for (j = 0; j < 32; i++, j++) {

Maybe during this refactor is it worth to add some #define with
number of registers to be read?

> +		ret = phy_read(netdev->phydev, j);
> +		if (ret < 0) {
> +			netdev_warn(dev->net,
> +				    "failed to read PHY register 0x%02x\n", j);
> +			goto clean_data;
> +		}
> +
> +		data[i] = ret;
> +		data_count++;
> +	}
> +
> +	return;
> +
> +clean_data:
> +	memset(data, 0, data_count * sizeof(u32));
>   }
>   
>   static const struct ethtool_ops lan78xx_ethtool_ops = {