> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-boun...@osuosl.org> On Behalf Of Jacob
> Keller
> Sent: Saturday, May 25, 2024 4:37 AM
> To: Kitszel, Przemyslaw <przemyslaw.kits...@intel.com>; Intel Wired LAN
> <intel-wired-...@lists.osuosl.org>; Paul Menzel <pmen...@molgen.mpg.de>
> Cc: Keller, Jacob E <jacob.e.kel...@intel.com>; Greenwalt, Paul
> <paul.greenw...@intel.com>
> Subject: [Intel-wired-lan] [PATCH iwl-net v4] ice: fix iteration of TLVs in
> Preserved Fields Area
>
> The ice_get_pfa_module_tlv() function iterates over the Type-Length-Value
> structures in the Preserved Fields Area (PFA) of the NVM. This is used by the
> driver to access data such as the Part Board Assembly identifier.
>
> The function uses simple logic to iterate over the PFA. First, the pointer to
> the PFA in the NVM is read. Then the total length of the PFA is read from the
> first word.
>
> A pointer to the first TLV is initialized, and a simple loop iterates over
> each TLV. The pointer is moved forward through the NVM until it exceeds the
> PFA area.
>
> The logic seems sound, but it is missing a key detail. The Preserved Fields
> Area length includes one additional final word. This is documented in the
> device data sheet as a dummy word which contains 0xFFFF. All NVMs have this
> extra word.
>
> If the driver tries to scan for a TLV that is not in the PFA, it will read
> past the size of the PFA. It reads and interprets the last dummy word of the
> PFA as a TLV with type 0xFFFF. It then reads the word following the PFA as a
> length.
>
> The PFA resides within the Shadow RAM portion of the NVM, which is relatively
> small. All of its offsets are within a 16-bit size. The PFA pointer and TLV
> pointer are stored by the driver as 16-bit values.
>
> In almost all cases, the word following the PFA will be such that
> interpreting it as a length will result in 16-bit arithmetic overflow. Once
> overflowed, the new next_tlv value is now below the maximum offset of the
> PFA. Thus, the driver will continue to iterate the data as TLVs. > In the
> worst case, the driver hits on a sequence of reads which loop back to reading
> the same offsets in an endless loop.
>
> To fix this, we need to correct the loop iteration check to account for this
> extra word at the end of the PFA. This alone is sufficient to resolve the
> known cases of this issue in the field. However, it is plausible that an NVM
> could be misconfigured or have corrupt data which results in the same kind of
> overflow. Protect against this by using check_add_overflow when calculating
> both the maximum offset of the TLVs, and when calculating the next_tlv offset
> at the end of each loop iteration. This ensures that the driver will not get
> stuck in an infinite > loop when scanning the PFA.
>
> Fixes: e961b679fb0b ("ice: add board identifier info to devlink .info_get")
> Co-developed-by: Paul Greenwalt <paul.greenw...@intel.com>
> Signed-off-by: Paul Greenwalt <paul.greenw...@intel.com>
> Signed-off-by: Jacob Keller <jacob.e.kel...@intel.com>
> ---
> Changes in v4:
> - Update title and description for true root cause
> - Correct driver logic to account for PFA length being 1 larger than TLVs
> - Link to v3:
> https://lore.kernel.org/r/20240517-iwl-net-2024-05-16-fix-nvm-tlv-issue-v3-1-f46c53cfb...@intel.com
>
> Changes in v3:
> - Fix missing {
> - Fix missing pfa_ptr variable to dev_warn()
> - Add Fixes tag
> - Link to v2:
> https://lore.kernel.org/r/20240517-iwl-net-2024-05-16-fix-nvm-tlv-issue-v2-1-fdee184ec...@intel.com
>
> Changes in v2:
> - Use check_add_overflow instead of increasing the variables to u32
> - Upgrade log messages to dev_warn()
> - Link to v1:
> https://lore.kernel.org/r/20240516-iwl-net-2024-05-16-fix-nvm-tlv-issue-v1-1-ecbb6a759...@intel.com
> ---
> drivers/net/ethernet/intel/ice/ice_nvm.c | 28 +++++++++++++++++++++-------
> 1 file changed, 21 insertions(+), 7 deletions(-)
>
Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pu...@intel.com> (A
Contingent worker at Intel)
