[PATCH net] nfc: nci: fix out-of-bounds read in nci_core_reset_rsp_packet()

[PATCH net] nfc: nci: fix out-of-bounds read in nci_core_reset_rsp_packet()

[Ömer Mete Kaya]

nci_core_reset_rsp_packet() guards with skb->len != 1 before reading
rsp->nci_ver (offset 1) and rsp->config_status (offset 2), but this
admits a 2-byte payload where config_status lies one byte past skb->len.

Fix by replacing the != 1 guard with an explicit >= 3 check before
accessing those fields. The 1-byte (NCI 1.x status-only) case is
handled first with an early return so nci_req_complete() is always
called regardless of payload length. nci_valid_size() in the caller
guarantees skb->len >= 1, so a zero-length payload never reaches this
handler.

Fixes: bcd684aace34 ("net/nfc/nci: Support NCI 2.x initial sequence")
Signed-off-by: Ömer Mete Kaya <omermetekaya0@gmail.com>
---
 net/nfc/nci/rsp.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/net/nfc/nci/rsp.c b/net/nfc/nci/rsp.c
index b0ab4f5ac..e52956bf3 100644
--- a/net/nfc/nci/rsp.c
+++ b/net/nfc/nci/rsp.c
@@ -32,16 +32,22 @@ static void nci_core_reset_rsp_packet(struct nci_dev *ndev,

 	pr_debug("status 0x%x\n", rsp->status);

-	/* Handle NCI 1.x ver */
-	if (skb->len != 1) {
-		if (rsp->status == NCI_STATUS_OK) {
-			ndev->nci_ver = rsp->nci_ver;
-			pr_debug("nci_ver 0x%x, config_status 0x%x\n",
-				 rsp->nci_ver, rsp->config_status);
-		}
-
+	/* NCI 2.x reset response carries nci_ver and config_status;
+	 * a 1-byte response is the NCI 1.x status-only form.
+	 * Require at least 3 bytes before reading those fields.
+	 */
+	if (skb->len == 1) {
 		nci_req_complete(ndev, rsp->status);
+		return;
 	}
+
+	if (skb->len >= 3 && rsp->status == NCI_STATUS_OK) {
+		ndev->nci_ver = rsp->nci_ver;
+		pr_debug("nci_ver 0x%x, config_status 0x%x\n",
+			 rsp->nci_ver, rsp->config_status);
+	}
+
+	nci_req_complete(ndev, rsp->status);
 }

 static u8 nci_core_init_rsp_packet_v1(struct nci_dev *ndev,
--
2.55.0

Re: [PATCH net] nfc: nci: fix out-of-bounds read in nci_core_reset_rsp_packet()

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net] nfc: nci: fix out-of-bounds read in nci_core_reset_rsp_packet()

[Ömer Mete Kaya]

On 10/1/26 01:54, netdev-bot+sinfo@kernel.org wrote:
> Hi!
> 
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
> 
>  - How the issue was discovered, e.g. hit in production, hit during
>    development, syzbot report, manual code inspection, LLM or static
>    analysis tool scan.

manual code inspection.

>  - Whether the issue was actually triggered, or is only theoretical
>    (e.g. found by code inspection). If it was triggered please include
>    the symptoms, like the stack trace or error messages.

theoretical/found by code inspection.

> Please do not repost the series just to address the above. Instead,
> reply to this email with the missing information, so that reviewers
> can take it into account. If the series needs another revision for
> other reasons, please include the information in the commit messages
> then.
> 
> The evaluation is done by an LLM so it may be wrong, if you think
> that is the case please reply and explain.

Re: [PATCH net] nfc: nci: fix out-of-bounds read in nci_core_reset_rsp_packet()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch breaks the NCI 2.x reset/init handshake.
- [Low] The new comment in nci_core_reset_rsp_packet() and the commit
  message both swap the NCI versions.
- [Low] The commit message overstates the impact of the bug it fixes.

--

nfc: nci: fix out-of-bounds read in nci_core_reset_rsp_packet()

This patch changes nci_core_reset_rsp_packet() so it only reads
rsp->nci_ver and rsp->config_status when the payload is at least 3 bytes.
It also adds an early nci_req_complete() and return for 1-byte responses.
With that, the reset request is completed for every payload length.

> nfc: nci: fix out-of-bounds read in nci_core_reset_rsp_packet()
>
> nci_core_reset_rsp_packet() guards with skb->len != 1 before reading
> rsp->nci_ver (offset 1) and rsp->config_status (offset 2), but this
> admits a 2-byte payload where config_status lies one byte past skb->len.

[Severity: Low]

Is "out-of-bounds read" the right description of the old behaviour? For a
2-byte payload, the only access past skb->len was rsp->config_status. That
value was only passed to pr_debug():

    pr_debug("nci_ver 0x%x, config_status 0x%x\n",
             rsp->nci_ver, rsp->config_status);

If DEBUG is not defined and the dynamic debug site is not enabled,
no_printk() never evaluates the arguments. If it is evaluated, the byte is
just past skb->len but still inside the skb head allocation. rsp->nci_ver
at offset 1 is in bounds for a 2-byte payload.

With the Fixes: tag, this framing invites a stable backport of a patch that
also changes the NCI 2.x reset flow (see below). Could the commit message
describe the impact more precisely?

> diff --git a/net/nfc/nci/rsp.c b/net/nfc/nci/rsp.c
> index b0ab4f5acbce8..e52956bf33de8 100644
> --- a/net/nfc/nci/rsp.c
> +++ b/net/nfc/nci/rsp.c
> @@ -32,16 +32,22 @@ static void nci_core_reset_rsp_packet(struct nci_dev *ndev,
>  
>  	pr_debug("status 0x%x\n", rsp->status);
>  
> -	/* Handle NCI 1.x ver */
> -	if (skb->len != 1) {
> -		if (rsp->status == NCI_STATUS_OK) {
> -			ndev->nci_ver = rsp->nci_ver;
> -			pr_debug("nci_ver 0x%x, config_status 0x%x\n",
> -				 rsp->nci_ver, rsp->config_status);
> -		}
> -
> +	/* NCI 2.x reset response carries nci_ver and config_status;
> +	 * a 1-byte response is the NCI 1.x status-only form.
> +	 * Require at least 3 bytes before reading those fields.
> +	 */

[Severity: Low]

Are the protocol versions swapped in this comment? The 3-byte response
(status, nci_ver, config_status) is the NCI 1.x form. The 1-byte
status-only response is NCI 2.x, and its version arrives later in
CORE_RESET_NTF.

The removed comment said "Handle NCI 1.x ver" above the branch that reads
rsp->nci_ver. nci_core_reset_ntf_packet() in net/nfc/nci/ntf.c is where
nci_ver gets set for NCI 2.x:

    /* Handle NCI 2.x core reset notification */
    ...
    ndev->nci_ver = ntf->nci_ver;

The selftest vectors in tools/testing/selftests/nci/nci_dev.c use the same
mapping:

    const __u8 nci_reset_rsp[] = {0x40, 0x00, 0x03, 0x00, 0x10, 0x01};
    const __u8 nci_reset_rsp_v2[] = {0x40, 0x00, 0x01, 0x00};

The commit message has the same mix-up in "The 1-byte (NCI 1.x
status-only) case".

> +	if (skb->len == 1) {
>  		nci_req_complete(ndev, rsp->status);
> +		return;
>  	}

[Severity: High]

Does this break the NCI 2.x reset sequence? The old code did not complete
the request on a 1-byte CORE_RESET_RSP. For NCI 2.x, the request stayed
pending until nci_core_reset_ntf_packet() had set nci_ver, manufact_id and
manufact_specific_info. That function then calls:

    nci_req_complete(ndev, NCI_STATUS_OK);

With this change, __nci_request(nci_reset_req) in nci_open_device() returns
as soon as the RSP arrives, before the NTF. nci_open_device() then picks
the CORE_INIT_CMD format from nci_ver, which is not set yet (it is 0 on
first open):

    if (ndev->nci_ver & NCI_VER_2_MASK)
        opt = &nci_init_v2_cmd;

As a result, an NCI 2.x controller gets an NCI 1.x CORE_INIT_CMD.
nci_core_init_rsp_packet() also uses nci_ver to choose the v1 or v2
parser, so the CORE_INIT_RSP would go to the v1 parser.

What happens when the late CORE_RESET_NTF arrives? nci_req_complete() only
checks req_status == NCI_REQ_PEND:

    if (ndev->req_status == NCI_REQ_PEND) {
        ndev->req_result = result;
        ndev->req_status = NCI_REQ_DONE;
        complete(&ndev->req_completion);
    }

So the NTF's completion would land on the next pending request, either
ops->setup() or CORE_INIT, before that request's own response has been
parsed. Every later request/response pair would then be off by one.

The NTF's write to ndev->nci_ver can also now race with the read in
nci_open_device(). The complete()/wait pair that used to order them is
gone.

The selftest covers this flow. virtual_dev_open_v2() writes
nci_reset_rsp_v2 and then nci_reset_ntf, and it expects nci_init_cmd_v2
next.

The commit message says nci_req_complete() should be "always called
regardless of payload length". Would it be enough to keep the original
completion behaviour and just add the skb->len >= 3 check before reading
rsp->nci_ver and rsp->config_status? A 1-byte RSP with a non-OK status
could still be completed right away, because no NTF follows it.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930224940.725976-1-omermetekaya0%40gmail.com