Re: [Intel-wired-lan] [PATCH net v2] i40e: limit the DDP profile count returned by the firmware

[Linkui Xiao]

Thanks for the review. Both findings are addressed in v3.

- [Medium] Incomplete fix in i40e_ddp_does_profile_exist() and
  i40e_ddp_does_profile_overlap()

  Agreed on the clamping half. Silently limiting the count and then
  returning 0 hands i40e_ddp_load() a definite answer derived from a list
  that was only partially read: the add path proceeds without having
  examined every profile the firmware reported, and the is_add == false
  path reports a profile as missing that may in fact be loaded. Both
  helpers already have an error return that i40e_ddp_load() turns into
  "Failed to fetch loaded profiles." and aborts the operation, so v3
  rejects the list instead: if the firmware reports more profiles than
  I40E_PROFILE_LIST_SIZE can hold, the helpers return -EIO and no scan
  happens at all.

  I did not plumb the response datalen out of i40e_aq_get_ddp_list().
  For this command the driver has to set desc.datalen to the buffer size
  it passes in, and i40e_aq_get_ddp_list() does not report the writeback
  descriptor back to its caller, so the length would have to come either
  from a new output parameter on that helper (declared in
  i40e_prototype.h) or from a wb_desc routed through cmd_details. Either
  way it adds a second firmware supplied number to validate on top of the
  one this patch is about, and if the firmware leaves the field at the
  requested 772 bytes the derived bound is exactly the bound we have
  today. That is a bigger change to the common AQ path than a net fix
  should carry, so I would rather bound the scan by the size of the
  buffer the driver owns.

  The remaining half - comparing against p_info[] slots the firmware
  never filled - cannot be detected independently of the count, because
  the driver is not told how many records were actually written. If the
  firmware reports a count of 16 or less while writing fewer records, the
  unread slots are indistinguishable from real entries. Zero
  initialization makes the outcome deterministic (an unwritten entry is
  all zeroes rather than stale stack) but does not make it correct;
  deriving the bound from the response length would be needed for that,
  and that is the larger helper change described above.

- [Medium, pre-existing] uninitialized buff[] handed to
  i40e_aq_get_ddp_list()

  Agreed, and fixed as suggested: buff[] is now zero initialized in both
  helpers. i40e_asq_send_command_atomic_exec() copies the full buff_size
  into the DMA bounce buffer and copies the full buff_size back on
  completion, so those 772 bytes of stack were both readable by the
  firmware and compared against afterwards. Both hunks touch these
  declarations and the root commit is the same (cdc594e00370), so it is
  handled here rather than as a separate patch; I can split it out if you
  would rather have it on its own.

v3 keeps the unsigned loop counter from v2, and drops the Reviewed-by tag,
as the code changed after the review.

pw-bot: cr