netdev - Re: [PATCH net-next 10/15] ice: display some stored NVM versions via devlink info

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: <d1d73560-63d8-846e-e121-00daef7b2c94@intel.com>
Date:   Mon, 1 Feb 2021 15:09:00 -0800
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Tony Nguyen <anthony.l.nguyen@...el.com>, davem@...emloft.net,
        netdev@...r.kernel.org, sassmann@...hat.com,
        Tony Brelinski <tonyx.brelinski@...el.com>
Subject: Re: [PATCH net-next 10/15] ice: display some stored NVM versions via
 devlink info



On 2/1/2021 2:34 PM, Jakub Kicinski wrote:
> On Mon, 1 Feb 2021 13:40:27 -0800 Jacob Keller wrote:
>> On 1/29/2021 10:37 PM, Jakub Kicinski wrote:
>>> On Thu, 28 Jan 2021 16:43:27 -0800 Tony Nguyen wrote:  
>>>> When reporting the versions via devlink info, first read the device
>>>> capabilities. If there is a pending flash update, use this new function
>>>> to extract the inactive flash versions. Add the stored fields to the
>>>> flash version map structure so that they will be displayed when
>>>> available.  
>>>
>>> Why only report them when there is an update pending?
>>>
>>> The expectation was that you'd always report what you can and user 
>>> can tell the update is pending by comparing the fields.
>>
>> If there is no pending update, what is the expected behavior? We report
>> the currently active image version as both stored and running?
>>
>> In our case, the device has 2 copies of each of the 3 modules: NVM,
>> Netlist, and UNDI/OptionROM.
>>
>> For each module, the device has a bit that indicates whether it will
>> boot from the first or second bank of the image. When we update,
>> whichever bank is not active is erased, and then populated with the new
>> image contents. The bit indicating which bank to load is flipped. Once
>> the device is rebooted (EMP reset), then the new bank is loaded, and the
>> firmware performs some onetime initialization.
>>
>> So for us, in theory we have up to 2 versions within the device for each
>> bank: the version in the currently active bank, and a version in the
>> inactive bank. In the inactive case, it may or may not be valid
>> depending on if that banks contents were ever a valid image. On a fresh
>> card, this might be empty or filled with garbage.
>>
>> Presumably we do not want to report that we have "stored" a version
>> which is not going to be activated next time that we boot?
>>
>> The documentation indicated that stored should be the version which
>> *will* be activated.
>>
>> If I just blindly always reported what was inactive, then the following
>> scenarios exist:
>>
>> # Brand new card:
>>
>> running:
>>   fw.bundle_id: Version
>> stored
>>   fw.bundle_id: <zero or garbage>
>>
>> # Do an update:
>>
>> running:
>>   fw.bundle_id: Version
>> stored
>>   fw.bundle_id: NewVersion
>>
>> # reset/reboot
>>
>> running:
>>   fw.bundle_id: NewVersion
>> stored:
>>   fw.bundle_id: Version
>>
>>
>> I could get behind that if we do not have a pending update we report the
>> stored value as the same as the running value (i.e. from the active
>> bank), where as if we have a pending update that will be triggered we
>> would report the inactive bank. I didn't see the value in that before
>> because it seemed like "if you don't have a pending update, you don't
>> have a stored value, so just report the active version in the running
>> category")
>>
>> It's also plausibly useful to report the stored but not pending value in
>> some cases, but I really don't want to report zeros or garbage data on
>> accident. This would almost certainly lead to confusing support
>> conversations.
> 
> Very good points. Please see the documentation for example workflow:
> 
> https://www.kernel.org/doc/html/latest/networking/devlink/devlink-flash.html#firmware-version-management
> 
> The FW update agent should be able to rely on 'stored' for checking if
> flash update is needed.
> 
> If the FW update is not pending just report the same values as running.
> You should not report old version after 2 flashings (3rd output in your
> example) - that'd confuse the flow - as you said - the stored versions
> would not be what will get activated.
> 

Sure, ok. I can add that when implementing this in the v2 submission
(along with extracting the security revision patches to a separate series).

Thanks,
Jake