linux-kernel - Re: [PATCH] igb: Add support for firmware update

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: <4f2ec2fb-5d31-4a90-9ef6-a036d16a5cb4@intel.com>
Date: Mon, 10 Jun 2024 15:04:06 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Richard chien <m8809301@...il.com>, <davem@...emloft.net>,
	<edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>
CC: <jesse.brandeburg@...el.com>, <anthony.l.nguyen@...el.com>,
	<intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, Richard chien <richard.chien@....com>
Subject: Re: [PATCH] igb: Add support for firmware update



On 6/9/2024 1:15 AM, Richard chien wrote:
> This patch adds support for firmware update to the in-tree igb driver and it is actually a port from the out-of-tree igb driver.
> In-band firmware update is one of the essential system maintenance tasks. To simplify this task, the Intel online firmware update
> utility provides a common interface that works across different Intel NICs running the igb/ixgbe/i40e drivers. Unfortunately, the
> in-tree igb and ixgbe drivers are unable to support this firmware update utility, causing problems for enterprise users who do not
> or cannot use out-of-distro drivers due to security and various other reasons (e.g. commercial Linux distros do not provide technical
> support for out-of-distro drivers). As a result, getting this feature into the in-tree igb driver is highly desirable.
> 
> Signed-off-by: Richard chien <richard.chien@....com>

The motivation to support updating flash with in-kernel drivers is good.

>  static int igb_set_eeprom(struct net_device *netdev,
> +                          struct ethtool_eeprom *eeprom, u8 *bytes)
> +{
> +        struct igb_adapter *adapter = netdev_priv(netdev);
> +        struct e1000_hw *hw = &adapter->hw;
> +        u16 *eeprom_buff;
> +        void *ptr;
> +        int max_len, first_word, last_word, ret_val = 0;
> +        struct e1000_nvm_access *nvm;
> +        u32 magic;
> +        u16 i;
> +
> +        if (eeprom->len == 0)
> +                return -EOPNOTSUPP;
> +
> +        magic = hw->vendor_id | (hw->device_id << 16);
> +        if (eeprom->magic && eeprom->magic != magic) {
> +                nvm = (struct e1000_nvm_access *)eeprom;
> +                ret_val = igb_nvmupd_command(hw, nvm, bytes);
> +                return ret_val;
> +        }
> +
However, this implementation is wrong. It is exposing the
ETHTOOL_GEEPROM and ETHTOOL_SEEPROM interface and abusing it to
implement a non-standard interface that is custom to the out-of-tree
Intel drivers to support the flash update utility.

This implementation was widely rejected when discovered in i40e and in
submissions for the  ice driver. It abuses the ETHTOOL_GEEPROM and
ETHTOOL_SEEPROM interface in order to allow tools to access the
hardware. The use violates the documented behavior of the ethtool
interface and breaks the intended functionality of ETHTOOL_GEEPROM and
ETHTOOL_SEEPROM.

The correct way to implement flash update is via the devlink dev flash
interface, using request_firmware, and implementing the entire update
process in the driver. The common portions of this could be done in a
shared module.

Attempting to support the broken legacy update that is supported by the
out-of-tree drivers is a non-starter for upstream. We (Intel) have known
this for some time, and this is why the patches and support have never
been published.