netdev - Re: [Intel-wired-lan] [PATCH] igb: add parameter to ignore nvm checksum validation

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: <20190517163643.7tlch7xqplxohoq7@zorba>
Date:   Fri, 17 May 2019 09:36:43 -0700
From:   Daniel Walker <danielwa@...co.com>
To:     Alexander Duyck <alexander.duyck@...il.com>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        "Nikunj Kela (nkela)" <nkela@...co.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
        "xe-linux-external(mailer list)" <xe-linux-external@...co.com>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [Intel-wired-lan] [PATCH] igb: add parameter to ignore nvm
 checksum validation

On Fri, May 17, 2019 at 08:16:34AM -0700, Alexander Duyck wrote:
> On Thu, May 16, 2019 at 6:48 PM Florian Fainelli <f.fainelli@...il.com> wrote:
> >
> >
> >
> > On 5/16/2019 6:03 PM, Daniel Walker wrote:
> > > On Thu, May 16, 2019 at 03:02:18PM -0700, Florian Fainelli wrote:
> > >> On 5/16/19 12:55 PM, Nikunj Kela (nkela) wrote:
> > >>>
> > >>>
> > >>> On 5/16/19, 12:35 PM, "Jeff Kirsher" <jeffrey.t.kirsher@...el.com> wrote:
> > >>>
> > >>>     On Wed, 2019-05-08 at 23:14 +0000, Nikunj Kela wrote:
> > >>>    >> Some of the broken NICs don't have EEPROM programmed correctly. It
> > >>>    >> results
> > >>>    >> in probe to fail. This change adds a module parameter that can be
> > >>>    >> used to
> > >>>    >> ignore nvm checksum validation.
> > >>>    >>
> > >>>    >> Cc: xe-linux-external@...co.com
> > >>>    >> Signed-off-by: Nikunj Kela <nkela@...co.com>
> > >>>    >> ---
> > >>>    >>  drivers/net/ethernet/intel/igb/igb_main.c | 28
> > >>>    >> ++++++++++++++++++++++------
> > >>>    >>  1 file changed, 22 insertions(+), 6 deletions(-)
> > >>>
> > >>>     >NAK for two reasons.  First, module parameters are not desirable
> > >>>     >because their individual to one driver and a global solution should be
> > >>>     >found so that all networking device drivers can use the solution.  This
> > >>>     >will keep the interface to change/setup/modify networking drivers
> > >>>     >consistent for all drivers.
> > >>>
> > >>>
> > >>>     >Second and more importantly, if your NIC is broken, fix it.  Do not try
> > >>>     >and create a software workaround so that you can continue to use a
> > >>>     >broken NIC.  There are methods/tools available to properly reprogram
> > >>>     >the EEPROM on a NIC, which is the right solution for your issue.
> > >>>
> > >>> I am proposing this as a debug parameter. Obviously, we need to fix EEPROM but this helps us continuing the development while manufacturing fixes NIC.
> > >>
> > >> Then why even bother with sending this upstream?
> > >
> > > It seems rather drastic to disable the entire driver because the checksum
> > > doesn't match. It really should be a warning, even a big warning, to let people
> > > know something is wrong, but disabling the whole driver doesn't make sense.
> >
> > You could generate a random Ethernet MAC address if you don't have a
> > valid one, a lot of drivers do that, and that's a fairly reasonable
> > behavior. At some point in your product development someone will
> > certainly verify that the provisioned MAC address matches the network
> > interface's MAC address.
> > --
> > Florian
> 
> The thing is the EEPROM contains much more than just the MAC address.
> There ends up being configuration for some of the PCIe interface in
> the hardware as well as PHY configuration. If that is somehow mangled
> we shouldn't be bringing up the part because there are one or more
> pieces of the device configuration that are likely wrong.
> 
> The checksum is being used to make sure the EEPROM is valid, without
> that we would need to go through and validate each individual section
> of the EEPROM before enabling the the portions of the device related
> to it. The concern is that this will become a slippery slope where we
> eventually have to code all the configuration of the EEPROM into the
> driver itself.
 

I don't think you can say because the checksum is valid that all data contained
inside is also valid. You can have a valid checksum , and someone screwed up the
data prior to the checksum getting computed.


> We need to make the checksum a hard stop. If the part is broken then
> it needs to be addressed. Workarounds just end up being used and
> forgotten, which makes it that much harder to support the product.
> Better to mark the part as being broken, and get it fixed now, than to
> have parts start shipping that require workarounds in order to
> function.o

I don't think it's realistic to define the development process for large
corporations like Cisco, or like what your doing , to define the development
process for all corporations and products which may use intel parts. It's better
to be flexible.

Daniel