[v2] net: usb: lan78xx: fix use of improperly initialized dev->chipid in lan78xx_reset

[PATCH net v2] net: usb: lan78xx: fix use of improperly initialized dev->chipid in lan78xx_reset

Posted by I Viswanath 2 months ago

dev->chipid is used in lan78xx_init_mac_address before it's initialized:

lan78xx_reset() {
    lan78xx_init_mac_address()
        lan78xx_read_eeprom()
            lan78xx_read_raw_eeprom() <- dev->chipid is used here

    dev->chipid = ... <- dev->chipid is initialized correctly here
}

Reorder initialization so that dev->chipid is set before calling
lan78xx_init_mac_address().

Fixes: a0db7d10b76e ("lan78xx: Add to handle mux control per chip id")
Signed-off-by: I Viswanath <viswanathiyyappan@gmail.com>
---
v1:
Link: https://lore.kernel.org/netdev/20251001131409.155650-1-viswanathiyyappan@gmail.com/

v2:
- Add Fixes tag

 drivers/net/usb/lan78xx.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 42d35cc6b421..b4b086f86ed8 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -3247,10 +3247,6 @@ static int lan78xx_reset(struct lan78xx_net *dev)
 		}
 	} while (buf & HW_CFG_LRST_);
 
-	ret = lan78xx_init_mac_address(dev);
-	if (ret < 0)
-		return ret;
-
 	/* save DEVID for later usage */
 	ret = lan78xx_read_reg(dev, ID_REV, &buf);
 	if (ret < 0)
@@ -3259,6 +3255,10 @@ static int lan78xx_reset(struct lan78xx_net *dev)
 	dev->chipid = (buf & ID_REV_CHIP_ID_MASK_) >> 16;
 	dev->chiprev = buf & ID_REV_CHIP_REV_MASK_;
 
+	ret = lan78xx_init_mac_address(dev);
+	if (ret < 0)
+		return ret;
+
 	/* Respond to the IN token with a NAK */
 	ret = lan78xx_read_reg(dev, USB_CFG0, &buf);
 	if (ret < 0)
-- 
2.47.3

Re: [PATCH net v2] net: usb: lan78xx: fix use of improperly initialized dev->chipid in lan78xx_reset

Posted by Khalid Aziz 2 months ago

On 10/13/25 12:16 PM, I Viswanath wrote:
> dev->chipid is used in lan78xx_init_mac_address before it's initialized:
> 
> lan78xx_reset() {
>      lan78xx_init_mac_address()
>          lan78xx_read_eeprom()
>              lan78xx_read_raw_eeprom() <- dev->chipid is used here
> 
>      dev->chipid = ... <- dev->chipid is initialized correctly here
> }
> 
> Reorder initialization so that dev->chipid is set before calling
> lan78xx_init_mac_address().
> 
> Fixes: a0db7d10b76e ("lan78xx: Add to handle mux control per chip id")
> Signed-off-by: I Viswanath <viswanathiyyappan@gmail.com>
> ---
> v1:
> Link: https://lore.kernel.org/netdev/20251001131409.155650-1-viswanathiyyappan@gmail.com/
> 
> v2:
> - Add Fixes tag
> 
>   drivers/net/usb/lan78xx.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index 42d35cc6b421..b4b086f86ed8 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -3247,10 +3247,6 @@ static int lan78xx_reset(struct lan78xx_net *dev)
>   		}
>   	} while (buf & HW_CFG_LRST_);
>   
> -	ret = lan78xx_init_mac_address(dev);
> -	if (ret < 0)
> -		return ret;
> -
>   	/* save DEVID for later usage */
>   	ret = lan78xx_read_reg(dev, ID_REV, &buf);
>   	if (ret < 0)
> @@ -3259,6 +3255,10 @@ static int lan78xx_reset(struct lan78xx_net *dev)
>   	dev->chipid = (buf & ID_REV_CHIP_ID_MASK_) >> 16;
>   	dev->chiprev = buf & ID_REV_CHIP_REV_MASK_;
>   
> +	ret = lan78xx_init_mac_address(dev);
> +	if (ret < 0)
> +		return ret;
> +
>   	/* Respond to the IN token with a NAK */
>   	ret = lan78xx_read_reg(dev, USB_CFG0, &buf);
>   	if (ret < 0)

Looks good to me.

Reviewed-by: Khalid Aziz <khalid@kernel.org>

--
Khalid

Re: [PATCH net v2] net: usb: lan78xx: fix use of improperly initialized dev->chipid in lan78xx_reset

Posted by Khalid Aziz 2 months ago

On 10/13/25 12:16 PM, I Viswanath wrote:
> dev->chipid is used in lan78xx_init_mac_address before it's initialized:
> 
> lan78xx_reset() {
>      lan78xx_init_mac_address()
>          lan78xx_read_eeprom()
>              lan78xx_read_raw_eeprom() <- dev->chipid is used here
> 
>      dev->chipid = ... <- dev->chipid is initialized correctly here
> }
> 
> Reorder initialization so that dev->chipid is set before calling
> lan78xx_init_mac_address().
> 
> Fixes: a0db7d10b76e ("lan78xx: Add to handle mux control per chip id")

How did you determine this is the commit that introduced this bug?

 From what I can see, commit a0db7d10b76e does not touch lan78xx_reset() 
function. This bug was introduced when devid was replaced by chipid 
(commit 87177ba6e47e "lan78xx: replace devid to chipid & chiprev") or 
even earlier when the order of calls to lan78xx_init_mac_address() and 
lan78xx_read_reg() was introduced in lan78xx_reset() depending upon if 
lan78xx_init_mac_address() at that time used devid in its call sequence 
at the time.

--
Khalid

> Signed-off-by: I Viswanath <viswanathiyyappan@gmail.com>
> ---
> v1:
> Link: https://lore.kernel.org/netdev/20251001131409.155650-1-viswanathiyyappan@gmail.com/
> 
> v2:
> - Add Fixes tag
> 
>   drivers/net/usb/lan78xx.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index 42d35cc6b421..b4b086f86ed8 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -3247,10 +3247,6 @@ static int lan78xx_reset(struct lan78xx_net *dev)
>   		}
>   	} while (buf & HW_CFG_LRST_);
>   
> -	ret = lan78xx_init_mac_address(dev);
> -	if (ret < 0)
> -		return ret;
> -
>   	/* save DEVID for later usage */
>   	ret = lan78xx_read_reg(dev, ID_REV, &buf);
>   	if (ret < 0)
> @@ -3259,6 +3255,10 @@ static int lan78xx_reset(struct lan78xx_net *dev)
>   	dev->chipid = (buf & ID_REV_CHIP_ID_MASK_) >> 16;
>   	dev->chiprev = buf & ID_REV_CHIP_REV_MASK_;
>   
> +	ret = lan78xx_init_mac_address(dev);
> +	if (ret < 0)
> +		return ret;
> +
>   	/* Respond to the IN token with a NAK */
>   	ret = lan78xx_read_reg(dev, USB_CFG0, &buf);
>   	if (ret < 0)

Re: [PATCH net v2] net: usb: lan78xx: fix use of improperly initialized dev->chipid in lan78xx_reset

Posted by I Viswanath 2 months ago

On Wed, 15 Oct 2025 at 21:25, Khalid Aziz <khalid@kernel.org> wrote:

> How did you determine this is the commit that introduced this bug?
>
>  From what I can see, commit a0db7d10b76e does not touch lan78xx_reset()
> function. This bug was introduced when devid was replaced by chipid
> (commit 87177ba6e47e "lan78xx: replace devid to chipid & chiprev") or
> even earlier when the order of calls to lan78xx_init_mac_address() and
> lan78xx_read_reg() was introduced in lan78xx_reset() depending upon if
> lan78xx_init_mac_address() at that time used devid in its call sequence
> at the time.

The commit a0db7d10b76e introduced the dependency on devid to
lan78xx_read_raw_eeprom() and
lan78xx_read_eeprom() and ultimately lan78xx_init_mac_address() and
lan78xx_reset()

In lan78xx_init_mac_address()

Only lan78xx_read_eeprom() depends on devid as

lan78xx_read_reg() and lan78xx_write_reg() do not use devid

lan78xx_read_otp() depends on lan78xx_read_raw_otp() which depends
only on lan78xx_write_reg() and lan78xx_read_reg()
and hence doesn't use devid either

is_valid_ether_addr(), random_ether_addr() and ether_addr_copy() are
net core functions and do not care about driver specific data

The devid read exists in this commit (was added in ce85e13ad6ef4)

a0db7d10b76e was supposed to move the devid read before the
lan78xx_init_mac_address() because of the newly added
dependency but it was a tricky detail that the author failed to see

Thanks,
I Viswanath

Re: [PATCH net v2] net: usb: lan78xx: fix use of improperly initialized dev->chipid in lan78xx_reset

Posted by Khalid Aziz 2 months ago

On 10/15/25 10:51 AM, I Viswanath wrote:
> On Wed, 15 Oct 2025 at 21:25, Khalid Aziz <khalid@kernel.org> wrote:
> 
>> How did you determine this is the commit that introduced this bug?
>>
>>   From what I can see, commit a0db7d10b76e does not touch lan78xx_reset()
>> function. This bug was introduced when devid was replaced by chipid
>> (commit 87177ba6e47e "lan78xx: replace devid to chipid & chiprev") or
>> even earlier when the order of calls to lan78xx_init_mac_address() and
>> lan78xx_read_reg() was introduced in lan78xx_reset() depending upon if
>> lan78xx_init_mac_address() at that time used devid in its call sequence
>> at the time.
> 
> The commit a0db7d10b76e introduced the dependency on devid to
> lan78xx_read_raw_eeprom() and
> lan78xx_read_eeprom() and ultimately lan78xx_init_mac_address() and
> lan78xx_reset()
> 
> In lan78xx_init_mac_address()
> 
> Only lan78xx_read_eeprom() depends on devid as
> 
> lan78xx_read_reg() and lan78xx_write_reg() do not use devid
> 
> lan78xx_read_otp() depends on lan78xx_read_raw_otp() which depends
> only on lan78xx_write_reg() and lan78xx_read_reg()
> and hence doesn't use devid either
> 
> is_valid_ether_addr(), random_ether_addr() and ether_addr_copy() are
> net core functions and do not care about driver specific data
> 
> The devid read exists in this commit (was added in ce85e13ad6ef4)
> 
> a0db7d10b76e was supposed to move the devid read before the
> lan78xx_init_mac_address() because of the newly added
> dependency but it was a tricky detail that the author failed to see
> 
> Thanks,
> I Viswanath

Ah, I see. That makes sense.

--
Khalid

Re: [PATCH net v2] net: usb: lan78xx: fix use of improperly initialized dev->chipid in lan78xx_reset

Posted by Vadim Fedorenko 2 months ago

On 13/10/2025 19:16, I Viswanath wrote:
> dev->chipid is used in lan78xx_init_mac_address before it's initialized:
> 
> lan78xx_reset() {
>      lan78xx_init_mac_address()
>          lan78xx_read_eeprom()
>              lan78xx_read_raw_eeprom() <- dev->chipid is used here
> 
>      dev->chipid = ... <- dev->chipid is initialized correctly here
> }
> 
> Reorder initialization so that dev->chipid is set before calling
> lan78xx_init_mac_address().
> 
> Fixes: a0db7d10b76e ("lan78xx: Add to handle mux control per chip id")
> Signed-off-by: I Viswanath <viswanathiyyappan@gmail.com>

Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>