netdev - Re: [PATCH] igb_ethtool: fix efficiency issues in igb_set

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-next>] [day] [month] [year] [list]

Message-ID: <7963e252-05cb-349e-5902-c4e38f7e9405@intel.com>
Date:   Tue, 10 May 2022 12:43:26 -0700
From:   Jesse Brandeburg <jesse.brandeburg@...el.com>
To:     lianglixue <lixue.liang5086@...il.com>,
        <anthony.l.nguyen@...el.com>, <kuba@...nel.org>,
        <intel-wired-lan@...ts.osuosl.org>
CC:     lianglixue <lianglixue@...atwall.com.cn>,
        Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH] igb_ethtool: fix efficiency issues in igb_set_eeprom

Please include netdev mailing list when mailing the maintainers of netdev.

On 5/9/2022 6:21 PM, lianglixue wrote:
> Modify the value of eeprom in igb_set_eeprom. If the mac address
> of i210 is changed to illegal, then in igp_probe in the
> igb_main file, is_valid_eth_addr (netdev->dev_addr) exits
> with an error, causing the igb driver to fail to load,
> such as ethtool -E eth0 magic 0x15338086 offset 0 value 0x01.

This interface is very much equivalent to a nail and hammer, and if you 
decide to hammer the nail through your foot that's your right.

This has been this way forever (in more than just igb) and we haven't 
changed this interface to "protect" the user, so I'm really not sure 
what the value of the change is now.

> In this way, the igb driver can no longer be loaded,
> and the legal mac address cannot be recovered through ethtool;
> add is_valid_eth_addr to igb_set_eeprom to determine
> whether it is legal to rewrite, so as to avoid driver
> errors due to illegal mac addresses.
> 
> Signed-off-by: lianglixue <lianglixue@...atwall.com.cn>
> ---
>   drivers/net/ethernet/intel/igb/igb_ethtool.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> index 2a5782063f4c..30554fd684db 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> @@ -798,6 +798,13 @@ static int igb_set_eeprom(struct net_device *netdev,
>   	if (eeprom->magic != (hw->vendor_id | (hw->device_id << 16)))
>   		return -EFAULT;
>   
> +	if (hw->mac.type == e1000_i210 && eeprom->offset == 0) {

before you go reading "bytes" length of bytes in the next line, you need 
to be checking the length of the write in this if as well, don't you? 
Why is this check only valid for i210? Is that the only one you know the 
eeprom format for?

> +		if (!is_valid_ether_addr(bytes)) {

this will read six bytes, but is before the length checks below.

> +			dev_err(&adapter->pdev->dev, "Invalid MAC Address for i210\n");
> +			return -EINVAL;
> +		}
> +	}
> +
>   	max_len = hw->nvm.word_size * 2;
>   
>   	first_word = eeprom->offset >> 1;

This change might still be useful, since it is obvious that several 
possible values of the first six bytes are invalid. Please respin and 
respond to the notes above.