netdev - Re: [PATCH v4] igb: Assign random MAC address instead of fail in case of invalid one

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: <f16ef33a4b12cebae2e2300a509014cd5de4a0d2.camel@gmail.com>
Date:   Thu, 02 Jun 2022 08:57:41 -0700
From:   Alexander H Duyck <alexander.duyck@...il.com>
To:     Lixue Liang <lianglixuehao@....com>, pmenzel@...gen.mpg.de
Cc:     anthony.l.nguyen@...el.com, intel-wired-lan@...ts.osuosl.org,
        jesse.brandeburg@...el.com, kuba@...nel.org,
        lianglixue@...atwall.com.cn, netdev@...r.kernel.org
Subject: Re: [PATCH v4] igb: Assign random MAC address instead of fail in
 case of invalid one

On Wed, 2022-06-01 at 15:04 +0000, Lixue Liang wrote:
> From: Lixue Liang <lianglixue@...atwall.com.cn>
> 
> In some cases, when the user uses igb_set_eeprom to modify the MAC
> address to be invalid, the igb driver will fail to load. If there is no
> network card device, the user must modify it to a valid MAC address by
> other means.
> 
> Since the MAC address can be modified, then add a random valid MAC address
> to replace the invalid MAC address in the driver can be workable, it can
> continue to finish the loading, and output the relevant log reminder.
> 
> Signed-off-by: Lixue Liang <lianglixue@...atwall.com.cn>
> ---
> Changelog:
> * v4:
>   - Change the igb_mian in the title to igb
>   - Fix dev_err message: replace "already assigned random MAC address" 
>     with "Invalid MAC address. Assigned random MAC address" 
>   Suggested-by Tony <anthony.l.nguyen@...el.com>
> 
> * v3:
>   - Add space after comma in commit message 
>   - Correct spelling of MAC address
>   Suggested-by Paul <pmenzel@...gen.mpg.de>
> 
> * v2:
>   - Change memcpy to ether_addr_copy
>   - Change dev_info to dev_err
>   - Fix the description of the commit message
>   - Change eth_random_addr to eth_hw_addr_random
>   Reported-by: kernel test robot <lkp@...el.com>
> 
>  drivers/net/ethernet/intel/igb/igb_main.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 34b33b21e0dc..5e3b162e50ac 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -3359,9 +3359,10 @@ static int igb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	eth_hw_addr_set(netdev, hw->mac.addr);
>  
>  	if (!is_valid_ether_addr(netdev->dev_addr)) {
> -		dev_err(&pdev->dev, "Invalid MAC Address\n");
> -		err = -EIO;
> -		goto err_eeprom;
> +		eth_hw_addr_random(netdev);
> +		ether_addr_copy(hw->mac.addr, netdev->dev_addr);
> +		dev_err(&pdev->dev,
> +			"Invalid MAC address. Assigned random MAC address\n");
>  	}
>  
>  	igb_set_default_mac_filter(adapter);

Losing the MAC address is one of the least destructive things you can
do by poking the EEPROM manually. There are settings in there for other
parts of the EEPROM for the NIC that can just as easily prevent the
driver from loading, or worse yet even prevent it from appearing on the
PCIe bus in some cases. So I don't see the user induced EEPROM
corruption as a good justification for this patch as the user shouldn't
be poking the EEPROM if they cannot do so without breaking things.

With that said I would be okay with adding this with the provision that
there is a module parameter to turn on this funcitonality. The
justification would be that the user is expecting to have a corrupted
EEPROM because they are working with some pre-production board or
uninitialized sample. This way if somebody is wanting to update the
EEPROM on a bad board they can use the kernel to do it, but they have
to explicitly enable this mode and not just have the fact that their
EEPROM is corrupted hidden as error messages don't necessarily get
peoples attention unless they are seeing some other issue.