Re: [PATCH v2] nfc: nci: add data_len bound checks to activation parameter extractors

Thanks for the review. v3 is posted as a new top-level thread:
https://lore.kernel.org/all/[email protected]/
Point-by-point below.

> [Severity: Medium] Is the correct error type being returned here?
> [...] -EINVAL [...] nci_to_errno() will truncate it [...] -ENOSYS [...]
> Should a valid NCI status code [...] be used instead?

Correct. These extractors return into the int 'err' in
nci_rf_intf_activated_ntf_packet(), which is handed to nci_req_complete()
and later run through nci_to_errno(). nci_to_errno() takes a __u8, so the
-EINVAL is truncated (to 234) at that argument and falls through to the
default case as -ENOSYS. The contract here is an NCI_STATUS_* code: 'err'
is initialized to NCI_STATUS_OK and the functions' own default cases
already return NCI_STATUS_RF_PROTOCOL_ERROR.

v3 returns NCI_STATUS_RF_PROTOCOL_ERROR from the new short-region guards,
which matches the sibling default cases and maps to -EPROTO via
nci_to_errno(). NCI_STATUS_INVALID_PARAM is equally valid (also -EPROTO) --
I went with RF_PROTOCOL_ERROR for consistency with the surrounding code;
glad to switch if you prefer INVALID_PARAM. v3 is otherwise identical to v2
and is posted as a new top-level thread.

> [Severity: Critical] [...] atr_res_len [...] integer underflow [...]
> up to 47 uninitialized stack bytes into ndev->remote_gb [...] broadcast
> to userspace?

The uninitialized-stack read is real: in nci_store_general_bytes_nfc_dep()
an atr_res_len below NFC_ATR_RES_GT_OFFSET makes min_t() clamp to
NFC_ATR_RES_GB_MAXSIZE and memcpy() copies stale bytes of the on-stack
atr_res[] into ndev->remote_gb. I could not, however, confirm the
"broadcast to userspace" part: remote_gb has no netlink attribute (there is
no NFC_ATTR_TARGET_GENERAL_BYTES), and its only path out is LLCP, which is
gated by the 3-byte llcp_magic memcmp on those same uninitialized bytes --
so a deterministic leak of the 47 bytes isn't reachable that way. It's
still a genuine uninitialized read worth closing; see below.

> [Severity: High] [...] sel_res [...] uninitialized [...] nci_add_new_protocol()
> [...] nfc_genl_send_target() [...] sent out unconditionally [...]

Confirmed, and this one does reach userspace: with sel_res_len == 0,
nci_extract_rf_params_nfca_passive_poll() leaves sel_res unwritten in the
on-stack ntf, nci_add_new_protocol() copies it unconditionally, and
nfc_genl_send_target() puts it into NFC_ATTR_TARGET_SEL_RES on
NFC_CMD_GET_TARGET -- a one-byte stack disclosure (the discover-NTF path
reaches it without the err==NCI_STATUS_OK gate).

Both pre-existing issues are closed by zero-initializing the on-stack
notification structs:

    struct nci_rf_discover_ntf ntf = {};        /* nci_rf_discover_ntf_packet */
    struct nci_rf_intf_activated_ntf ntf = {};  /* nci_rf_intf_activated_ntf_packet */

I'll send that as a separate patch rather than fold it into this one, since
it's an independent change in different functions.