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

[Simon Horman <horms@kernel.org>]

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://sashiko.dev

Simon says:

The error type issue below - the first issue - does seem of
concern in the context of this patch.

While I think the other issues, which are flagged as pre-existing,
can be considered in the context of possible follow-up.

---
nfc: nci: add data_len bound checks to activation parameter extractors

This patch adds data_len bounds checking to the NCI activation parameter
extractors to prevent slab out-of-bounds reads. It introduces a data_len
parameter to guard against empty regions before consuming inner length
bytes.

> diff --git a/net/nfc/nci/ntf.c b/net/nfc/nci/ntf.c
> index c96512bb86531..753f4cf087489 100644
> --- a/net/nfc/nci/ntf.c
> +++ b/net/nfc/nci/ntf.c
> @@ -525,15 +525,19 @@ static int nci_rf_discover_ntf_packet(struct nci_dev *ndev,
>  
>  static int nci_extract_activation_params_iso_dep(struct nci_dev *ndev,
>  						 struct nci_rf_intf_activated_ntf *ntf,
> -						 const __u8 *data)
> +						 const __u8 *data, __u8 data_len)
>  {
>  	struct activation_params_nfca_poll_iso_dep *nfca_poll;
>  	struct activation_params_nfcb_poll_iso_dep *nfcb_poll;
>  
>  	switch (ntf->activation_rf_tech_and_mode) {
>  	case NCI_NFC_A_PASSIVE_POLL_MODE:
> +		if (data_len < 1)
> +			return -EINVAL;

[Severity: Medium]
Is the correct error type being returned here?

nci_extract_activation_params_iso_dep() is called by
nci_rf_intf_activated_ntf_packet() and its return value is assigned to err,
which is later passed to nci_req_complete(ndev, err).

nci_req_complete() expects an NCI_STATUS_* positive integer constant.
If the POSIX error code -EINVAL (-22) is returned, nci_to_errno() will
truncate it to 234. It will then be incorrectly translated to -ENOSYS
through the default case of its switch statement.

Should a valid NCI status code like NCI_STATUS_INVALID_PARAM or
NCI_STATUS_RF_PROTOCOL_ERROR be used instead? The same behavior appears
in the updated nci_extract_activation_params_nfc_dep() below.

>  		nfca_poll = &ntf->activation_params.nfca_poll_iso_dep;
>  		nfca_poll->rats_res_len = min_t(__u8, *data++, NFC_ATS_MAXSIZE);

[ ... ]

> @@ -806,12 +822,14 @@ static int nci_rf_intf_activated_ntf_packet(struct nci_dev *ndev,
>  		switch (ntf.rf_interface) {
>  		case NCI_RF_INTERFACE_ISO_DEP:
>  			err = nci_extract_activation_params_iso_dep(ndev,
> -								    &ntf, data);
> +								    &ntf, data,
> +								    ntf.activation_params_len);

[Severity: Critical]
This isn't a bug introduced by this patch, but could the parsing of
atr_res_len allow for a stack memory disclosure?

In nci_rf_intf_activated_ntf_packet(), the ntf structure is allocated on
the stack without zero-initialization.

If a malformed packet specifies an activation_params_len >= 1 but the inner
atr_res_len is clamped to less than 15 (NFC_ATR_RES_GT_OFFSET), would this
cause an integer underflow?

net/nfc/nci/ntf.c:nci_store_general_bytes_nfc_dep() {
    ...
    ndev->remote_gb_len = min_t(__u8,
        (ntf->activation_params.poll_nfc_dep.atr_res_len
            - NFC_ATR_RES_GT_OFFSET),
        NFC_ATR_RES_GB_MAXSIZE);
    ...
    memcpy(ndev->remote_gb,
           (ntf->activation_params.poll_nfc_dep.atr_res
            + NFC_ATR_RES_GT_OFFSET),
           ndev->remote_gb_len);
}

Because __u8 is unsigned, a negative result from the subtraction underflows
to a large positive value, which min_t() then clamps to
NFC_ATR_RES_GB_MAXSIZE (47).

Could this result in copying up to 47 uninitialized stack bytes into
ndev->remote_gb, which might later be broadcast to userspace?

[Severity: High]
This is a pre-existing issue, but is there also a 1-byte stack disclosure
in the NFC-A passive poll parameter parsing?

In nci_extract_rf_params_nfca_passive_poll(), sel_res is only initialized
if sel_res_len != 0:

net/nfc/nci/ntf.c:nci_extract_rf_params_nfca_passive_poll() {
    if (nfca_poll->sel_res_len != 0) {
        if (data_len < 1)
            return ERR_PTR(-EINVAL);
        nfca_poll->sel_res = *data++;
    }
}

If sel_res_len is 0, nfca_poll->sel_res remains uninitialized. Because
nfca_poll points into the local uninitialized ntf stack variable in
nci_rf_intf_activated_ntf_packet(), it retains whatever garbage was on
the stack.

Later, nci_add_new_protocol() unconditionally copies this uninitialized byte:

net/nfc/nci/ntf.c:nci_add_new_protocol() {
    ...
    target->sel_res = nfca_poll->sel_res;
    ...
}

When userspace queries the targets, this byte is sent out unconditionally:

net/nfc/netlink.c:nfc_genl_send_target() {
    ...
    if (nla_put_u8(msg, NFC_ATTR_TARGET_SEL_RES, target->sel_res))
    ...
}

Would it be safer to zero-initialize the ntf structure at the start of
nci_rf_intf_activated_ntf_packet(), or set sel_res to 0 when sel_res_len
is 0?

>  			break;
>  
>  		case NCI_RF_INTERFACE_NFC_DEP: