Re: [PATCH] nfc: pn533: validate response length before parsing DEP ATR_RES

[netdev-bot+sashiko@kernel.org]

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

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

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] In pn533_poll_dep_complete() (drivers/nfc/pn533/pn533.c, new
  check at line 1313) and pn533_in_dep_link_up_complete() (line 1936),…
- [Low] The commit message says 'pn533_rf_complete()-style handlers in
  this file already length-check resp->len before use'.
- [Low] pn533_activate_target_nfcdep() (drivers/nfc/pn533/pn533.c:1777)
  parses the InATR ATR_RES (struct pn533_cmd_activate_response, 16-byte…
- [Low] The patch says it fixes an out-of-bounds read whose bytes reach
  userspace through netlink (nfc_targets_found() ->…

--

nfc: pn533: validate response length before parsing DEP ATR_RES

This commit adds a check that resp->len is at least
sizeof(struct pn533_cmd_jump_dep_response) to pn533_poll_dep_complete()
and pn533_in_dep_link_up_complete(). The check runs before the response
status is read. It is meant to stop the handlers from reading the fixed
17-byte ATR_RES header, including nfcid3t[], past the end of a short
response.

> A valid ATR_RES always carries the full 17-byte fixed header (the
> general bytes, if present, start at offset 17), so reject any response
> shorter than sizeof(*rsp) before parsing it. pn533_rf_complete()-style
> handlers in this file already length-check resp->len before use (e.g.
> the ATR_REQ_GB_OFFSET check in pn533_init_target_complete()).

[Severity: Low]
Is this description of the other handlers accurate? pn533_rf_complete()
only checks IS_ERR(resp), then queues poll_work and frees the skb. It
never looks at resp->len or resp->data:

drivers/nfc/pn533/pn533.c:pn533_rf_complete() {
	...
	if (IS_ERR(resp)) {
		...
		return rc;
	}

	queue_delayed_work(dev->wq, &dev->poll_work,
			   msecs_to_jiffies(PN533_POLL_INTERVAL));

	dev_kfree_skb(resp);
	...
}

pn533_start_poll_complete() also reads the response without checking its
length:

	nbtg = resp->data[0];
	tg = resp->data[1];
	tgdata = &resp->data[2];
	tgdata_len = resp->len - 2;  /* nbtg + tg */

pn533_autopoll_complete() also walks the response without bounds checks.
Of the handlers mentioned, only pn533_init_target_complete() checks the
length.

Could this sentence be reworded so it doesn't suggest the rest of the
file already has these checks?

> Found by code review; not reproduced.
>
> Signed-off-by: Hyunjung Ko <hj351016@gmail.com>

[Severity: Low]
The commit message describes an over-read whose bytes reach userspace
through nfc_targets_found(). Should the patch carry Fixes: tags so the
fix reaches stable kernels? The unchecked parsing in
pn533_in_dep_link_up_complete() and pn533_poll_dep_complete() appears to
come from these commits:

Fixes: 361f3cb7f9cf ("NFC: DEP link hook implementation for pn533")
Fixes: 673088fb42d0 ("NFC: pn533: Send ATR_REQ directly for active device detection")

> diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
> index f5a6a7c20d5af..63b6032692184 100644
> --- a/drivers/nfc/pn533/pn533.c
> +++ b/drivers/nfc/pn533/pn533.c
> @@ -1310,6 +1310,11 @@ static int pn533_poll_dep_complete(struct pn533 *dev, void *arg,
>  
>  	rsp = (struct pn533_cmd_jump_dep_response *)resp->data;
>  
> +	if (resp->len < sizeof(*rsp)) {
> +		rc = -EINVAL;
> +		goto error;
> +	}
> +
>  	rc = rsp->status & PN533_CMD_RET_MASK;
>  	if (rc != PN533_CMD_RET_SUCCESS) {
>  		/* Not target found, turn radio off */

[Severity: High]
Can this check stop NFC polling whenever no DEP peer is in the field?

As far as I can tell, a PN53x returns the ATR_RES fields (Tg, NFCID3t,
DIDt, BSt, BRt, TO, PPt, Gt) only after a successful InJumpForDEP. If no
ATR_RES arrives, for example on a timeout, the chip replies with
D5 57 <Status> and nothing more. After pn533_send_async_complete() strips
TFI and CC, resp->len is 1.

The old code handled this case by reading only rsp->status. With the new
check, 1 < 17 jumps to error and returns -EINVAL, so this branch never
runs:

	if (rc != PN533_CMD_RET_SUCCESS) {
		/* Not target found, turn radio off */
		queue_work(dev->wq, &dev->rf_work);

pn533_wq_rf() sends RFConfiguration. Its completion, pn533_rf_complete(),
is the only place that re-arms poll_work after a DEP attempt.
pn533_wq_cmd_complete() ignores the -EINVAL:

	rc = pn533_send_async_complete(dev);
	if (rc != -EINPROGRESS)
		queue_work(dev->wq, &dev->cmd_work);

So nothing restarts the poll loop. poll_mod_count and nfc_dev->polling
still report that polling is active.

Since commit 673088fb42d0 ("NFC: pn533: Send ATR_REQ directly for active
device detection"), pn533_send_poll_frame() sends a DEP poll between
passive cycles whenever NFC_DEP is requested:

	if ((dev->poll_protocols & NFC_PROTO_NFC_DEP_MASK) && dev->poll_dep)  {
		dev->poll_dep = 0;
		return pn533_poll_dep(dev->nfc_dev);
	}

So any poll that includes NFC_DEP and finds nothing on its first passive
cycle would stall after the first DEP attempt. It stays stalled until
userspace stops and restarts polling, and then it stalls again the same
way.

pn533_in_dep_link_up_complete() gets the same status-only reply and now
returns -EINVAL without logging anything. The "Bringing DEP link up
failed (error 0x%x)" message with the chip's status code is never
printed.

The commit message says "A valid ATR_RES always carries the full 17-byte
fixed header", but that seems to hold only for successful replies.

Would it work to check resp->len >= 1 before reading the status, keep the
existing failure handling, and require resp->len >= sizeof(*rsp) only on
the success path, before nfcid3t is copied and target_gt_len is computed?

> @@ -1928,6 +1933,11 @@ static int pn533_in_dep_link_up_complete(struct pn533 *dev, void *arg,
>  
>  	rsp = (struct pn533_cmd_jump_dep_response *)resp->data;
>  
> +	if (resp->len < sizeof(*rsp)) {
> +		rc = -EINVAL;
> +		goto error;
> +	}
> +
>  	rc = rsp->status & PN533_CMD_RET_MASK;
>  	if (rc != PN533_CMD_RET_SUCCESS) {
>  		nfc_err(dev->dev,

[Severity: Low]
pn533_activate_target_nfcdep() parses the InATR ATR_RES (struct
pn533_cmd_activate_response, which has a 16-byte fixed header) the same
way, and this patch leaves it unchanged:

drivers/nfc/pn533/pn533.c:pn533_activate_target_nfcdep() {
	...
	rsp = (struct pn533_cmd_activate_response *)resp->data;
	rc = rsp->status & PN533_CMD_RET_MASK;
	...
	/* ATR_RES general bytes are located at offset 16 */
	gt_len = resp->len - 16;
	rc = nfc_set_remote_general_bytes(dev->nfc_dev, rsp->gt, gt_len);
	...
}

rsp->status is read without checking that resp->len is at least 1. If the
reply carries only TFI and CC (resp->len == 0), the trimmed checksum byte
is read as the status.

The gt_len underflow is rejected later in nfc_llcp_set_remote_gb(). That
is the same mitigation the commit message describes for target_gt_len.

Should this third ATR_RES parser get the same status length check as the
other two?

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