Re: [PATCH net] nfc: nci: fix uninit-value in the RF discover/activated NTF handlers

[Sam P]

On 25/06/2026 10:18, Simon Horman wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://netdev-ai.bots.linux.dev/sashiko/
> ---
> nfc: nci: fix uninit-value in the RF discover/activated NTF handlers
>
> This zero-initialises the on-stack nci_rf_discover_ntf and
> nci_rf_intf_activated_ntf structs in the RF discover and interface
> activated NTF handlers so that the technology- and activation-specific
> unions read back as zero when the corresponding length fields are zero.
>
> Link: https://lore.kernel.org/netdev/20260623172109.1105965-2-horms@xxxxxxxxxx/
>
> >   - activated: with rf_tech_specific_params_len == 0 the same union is read
> >     via nci_target_auto_activated(); with activation_params_len == 0 the
> >     activation_params union is read by nci_store_ats_nfc_iso_dep() into
> >     ndev->target_ats.
>
> [Low]
> Does the third scenario in this paragraph match the code?
>
> Looking at nci_store_ats_nfc_iso_dep():
>
>      static int nci_store_ats_nfc_iso_dep(struct nci_dev *ndev,
>                           const struct nci_rf_intf_activated_ntf *ntf)
>      {
>          ndev->target_ats_len = 0;
>
>          if (ntf->activation_params_len <= 0)
>              return NCI_STATUS_OK;
>
>          if (ntf->activation_params.nfca_poll_iso_dep.rats_res_len > NFC_ATS_MAXSIZE) {
>          ...
>
> The function returns NCI_STATUS_OK before any access to the
> activation_params union when activation_params_len is 0, so the union
> is not read in that case.
>
> The sibling nci_store_general_bytes_nfc_dep() has the same early return
> on activation_params_len <= 0.
>
> The two rf_tech_specific_params_len == 0 cases listed earlier in the
> commit message do match the code (nci_add_new_protocol() and
> nci_target_auto_activated() unconditionally pass
> &ntf.rf_tech_specific_params even when rf_tech_specific_params_len is
> 0), and the KMSAN trace quoted in the commit message only mentions the
> nci_add_new_protocol() path.
>
> The ntf = {} change still looks correct and necessary for the
> rf_tech_specific_params union. Could the third bullet be dropped or
> adjusted so the commit message reflects only the paths that actually
> read uninitialised bytes?

Thanks for sharing the review.

This is valid feedback, I'll fix up the commit message in a v2.