Message ID | 20240517-iwl-net-2024-05-16-fix-nvm-tlv-issue-v2-1-fdee184ece86@intel.com |
---|---|
State | Superseded |
Headers | show |
Series | [iwl-net,v2] ice: avoid infinite loop if NVM has invalid TLV length | expand |
On 5/17/2024 4:08 PM, Jacob Keller wrote: > The ice_get_pfa_module_tlv() function iterates over the TLVs in the > Preserved Fields Area (PFA) of the NVM. This is used to access data such as > the Part Board Assembly identifier. > > Some NVMs in the wild have been found with incorrect TLV lengths including > at least one which reports a TLV length of 0xFFFF. When trying to read the > PBA from such an NVM, the driver will compute a new offset for the next_tlv > which is lower, due to overflowing the 16-bit next_tlv variable. > > In the best case, the driver will incorrectly interpret values until it > finds one which has an offset greater than the PFA area without > overflowing. In the worst case, the values in the NVM result in an infinite > loop as the misinterpreted lengths result in checking offsets which are > valid within the PFA, and which ultimately point in an infinite loop. > > Fix this by using check_add_overflow when calculating the NVM offsets, and > bailing if we ever overflow. Additionally, use check_add_overflow when > calculating the initial maximum PFA size. > > This ensures that we bail immediately on encountering any TLV who's length > would have caused the naive addition to overflow, rather than entering an > infinite loop or otherwise misinterpreting NVM values. > > Co-developed-by: Paul Greenwalt <paul.greenwalt@intel.com> > Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com> > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> Fixes: e961b679fb0b ("ice: add board identifier info to devlink .info_get") I'll ensure this gets added to the commit when queuing, as I forgot to add it when re-writing this commit message.
On 5/17/2024 4:08 PM, Jacob Keller wrote: > - next_tlv = next_tlv + tlv_len + 2; > + if (check_add_overflow(next_tlv, 2, &next_tlv) || > + check_add_overflow(next_tlv, tlv_len, &next_tlv)) > + dev_warn(ice_hw_to_dev(hw), "Failed to locate PFA TLV module of type %u due to arithmetic overflow. The PFA length is %u. The last TLV has length of %u\n", > + module_type, pfa_len, tlv_len); > + return -EINVAL; > + } This hunk is missing a { which I had fixed locally but forgot to ammend. I'll have it fixed in a v3 later.. :( Thanks, Jake
diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c index 84eab92dc03c..53dbc9000652 100644 --- a/drivers/net/ethernet/intel/ice/ice_nvm.c +++ b/drivers/net/ethernet/intel/ice/ice_nvm.c @@ -440,8 +440,7 @@ int ice_get_pfa_module_tlv(struct ice_hw *hw, u16 *module_tlv, u16 *module_tlv_len, u16 module_type) { - u16 pfa_len, pfa_ptr; - u16 next_tlv; + u16 pfa_len, pfa_ptr, next_tlv, max_pfa; int status; status = ice_read_sr_word(hw, ICE_SR_PFA_PTR, &pfa_ptr); @@ -454,11 +453,18 @@ ice_get_pfa_module_tlv(struct ice_hw *hw, u16 *module_tlv, u16 *module_tlv_len, ice_debug(hw, ICE_DBG_INIT, "Failed to read PFA length.\n"); return status; } + + if (check_add_overflow(pfa_ptr, pfa_len, &max_pfa)) { + dev_warn(ice_hw_to_dev(hw), "PFA starts at offset %u. PFA length of %u causes 16-bit arithmetic overflow.\n", + pfa_len); + return -EINVAL; + } + /* Starting with first TLV after PFA length, iterate through the list * of TLVs to find the requested one. */ next_tlv = pfa_ptr + 1; - while (next_tlv < pfa_ptr + pfa_len) { + while (next_tlv < max_pfa) { u16 tlv_sub_module_type; u16 tlv_len; @@ -485,7 +491,12 @@ ice_get_pfa_module_tlv(struct ice_hw *hw, u16 *module_tlv, u16 *module_tlv_len, /* Check next TLV, i.e. current TLV pointer + length + 2 words * (for current TLV's type and length) */ - next_tlv = next_tlv + tlv_len + 2; + if (check_add_overflow(next_tlv, 2, &next_tlv) || + check_add_overflow(next_tlv, tlv_len, &next_tlv)) + dev_warn(ice_hw_to_dev(hw), "Failed to locate PFA TLV module of type %u due to arithmetic overflow. The PFA length is %u. The last TLV has length of %u\n", + module_type, pfa_len, tlv_len); + return -EINVAL; + } } /* Module does not exist */ return -ENOENT;