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

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

[hyunjungg]

From: Hyunjung Ko <hj351016@gmail.com>

pn533_poll_dep_complete() and pn533_in_dep_link_up_complete() cast
resp->data to a struct pn533_cmd_jump_dep_response and read its fixed
17-byte header (status, tg, nfcid3t[10], didt, bst, brt, to, ppt)
without first checking that resp->len covers that header.

On a short or truncated response the handlers read bytes beyond the
received frame:

  - rsp->status is dereferenced with no minimum-length check;
  - for a success status, memcpy(nfc_target.nfcid1, rsp->nfcid3t, 10)
    copies 10 bytes from offset 2, so for resp->len < 12 it reads past
    the valid response data. Those bytes are then handed to userspace
    via nfc_targets_found() (netlink), which can disclose stale buffer
    contents.

target_gt_len = resp->len - 17 additionally underflows (it is u8) when
resp->len < 17. That path is mitigated downstream --
nfc_set_remote_general_bytes() -> nfc_llcp_set_remote_gb() rejects the
resulting out-of-range length -- so the subtraction does not by itself
over-read rsp->gt[]; the fixed-field reads above are unguarded
regardless.

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()).

Found by code review; not reproduced.

Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
---
 drivers/nfc/pn533/pn533.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
--- a/drivers/nfc/pn533/pn533.c
+++ b/drivers/nfc/pn533/pn533.c
@@ -1309,7 +1309,12 @@ static int pn533_poll_dep_complete(struct pn533 *dev, void *arg,
 	memset(&nfc_target, 0, sizeof(struct nfc_target));

 	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 */
@@ -1929,7 +1929,12 @@ 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,

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

[netdev-bot+sashiko]

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

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

[khj]

 Thanks -- the [High] is correct and important. v1 put the length
check before the status read, so a no-peer/timeout reply (resp->len ==
1) returned -EINVAL and skipped the "turn radio off, re-arm polling"
path, stalling the DEP poll loop. v2 moves the check onto the success
path (after the status check) in all three ATR_RES handlers, so the
short failure reply keeps its existing handling and only a short
success reply (which would over-read the fixed header) is rejected.

 v2 also adds the Fixes: tags, guards the third parser
(pn533_activate_target_nfcdep), and drops the inaccurate sentence that
claimed the other handlers already length-check resp->len (only
pn533_init_target_complete() does).

 pw-bot: cr

2026년 11월 9일 (월) 오후 7:53, <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

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

[hyunjungg]

From: Hyunjung Ko <hj351016@gmail.com>

pn533_poll_dep_complete() and pn533_in_dep_link_up_complete() cast
resp->data to a struct pn533_cmd_jump_dep_response and read its fixed
17-byte header (status, tg, nfcid3t[10], didt, bst, brt, to, ppt)
without first checking that resp->len covers that header.

On a short or truncated response the handlers read bytes beyond the
received frame:

  - rsp->status is dereferenced with no minimum-length check;
  - for a success status, memcpy(nfc_target.nfcid1, rsp->nfcid3t, 10)
    copies 10 bytes from offset 2, so for resp->len < 12 it reads past
    the valid response data. Those bytes are then handed to userspace
    via nfc_targets_found() (netlink), which can disclose stale buffer
    contents.

target_gt_len = resp->len - 17 additionally underflows (it is u8) when
resp->len < 17. That path is mitigated downstream --
nfc_set_remote_general_bytes() -> nfc_llcp_set_remote_gb() rejects the
resulting out-of-range length -- so the subtraction does not by itself
over-read rsp->gt[]; the fixed-field reads above are unguarded
regardless.

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()).

Found by code review; not reproduced.

Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
---
 drivers/nfc/pn533/pn533.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
--- a/drivers/nfc/pn533/pn533.c
+++ b/drivers/nfc/pn533/pn533.c
@@ -1309,7 +1309,12 @@ static int pn533_poll_dep_complete(struct pn533 *dev, void *arg,
 	memset(&nfc_target, 0, sizeof(struct nfc_target));

 	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 */
@@ -1929,7 +1929,12 @@ 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,

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 1 · 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 new length check in pn533_poll_dep_complete() and
  pn533_in_dep_link_up_complete() (drivers/nfc/pn533/pn533.c) runs…
- [Medium] pn533_activate_target_nfcdep() in drivers/nfc/pn533/pn533.c
  parses the ATR_RES returned by PN533_CMD_IN_ATR in the same way as the…
- [Low] The commit message says 'pn533_rf_complete()-style handlers in
  this file already length-check resp->len before use (e.g. the…
- [Low] The patch says it fixes an out-of-bounds read whose bytes reach
  userspace through nfc_targets_found() (netlink), but it has no Fixes:…

--

nfc: pn533: validate response length before parsing DEP ATR_RES

This patch adds a resp->len < sizeof(*rsp) check to
pn533_poll_dep_complete() and pn533_in_dep_link_up_complete(). The check
runs before the InJumpForDEP response is parsed as a struct
pn533_cmd_jump_dep_response. The goal is to stop reads of the 17-byte
fixed header from going past the end of a short frame.

> 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 accurate? pn533_rf_complete() only checks
IS_ERR(resp), queues poll_work and frees the skb. It never reads
resp->data and has no resp->len check.

pn533_init_target_complete() does have a real check:

    if (resp->len < ATR_REQ_GB_OFFSET + 1)
        return -EINVAL;

Other completion handlers in this file read resp->data without any
length check. One example is pn533_start_poll_complete():

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

pn533_tm_get_data_complete() and pn533_data_exchange_complete() do the
same.

Could this sentence be reworded or dropped, so it doesn't suggest the rest
of the file already validates lengths?

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

[Severity: Low]
The commit message says stale buffer contents can reach userspace through
nfc_targets_found(). Should the patch carry Fixes: tags and Cc: stable?

git blame shows the unchecked parsing came from these commits:

  pn533_poll_dep_complete(): 673088fb42d0 ("NFC: pn533: Send ATR_REQ
  directly for active device detection")

  pn533_in_dep_link_up_complete(): 361f3cb7f9cf ("NFC: DEP link hook
  implementation for pn533"), with the current rsp cast from
  13003649b1eb

> 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]
Does this check also reject failure responses that worked before? The old
failure path read only rsp->status (resp->data[0]), so it handled a
failure frame of any length.

pn533_send_async_complete() strips the frame header and tail:

    skb_pull(resp, dev->ops->rx_header_len);
    skb_trim(resp, resp->len - dev->ops->rx_tail_len);

After that, a status-only InJumpForDEP error reply such as D5 57 <status>
has resp->len == 1. It now takes the goto error path and returns -EINVAL,
skipping the "Not target found" branch:

    queue_work(dev->wq, &dev->rf_work);

pn533_wq_rf()->pn533_rf_complete() is the only place that re-queues
poll_work after a DEP poll attempt. pn533_wq_cmd_complete() ignores the
callback's return value except for -EINPROGRESS. Doesn't the poll loop
then stop silently?

In that state poll_mod_count stays non-zero and nfc_dev->polling stays
true. No targets are reported, and a new NFC_CMD_START_POLL fails with
-EBUSY in nfc_start_poll() until userspace stops polling. The one
exception is when the current modulation is listen mode
(cur_mod->len == 0). In that case listen_timer eventually moves polling
to the next modulation.

Once pn533_start_poll_complete() has set poll_dep, this is the normal
"no peer in range" path, not an edge case.

pn533_in_dep_link_up_complete() below has the same ordering. A short
failure reply there now returns -EINVAL before reaching this:

    nfc_err(dev->dev,
        "Bringing DEP link up failed (error 0x%x)\n", rc);

so the chip's error code is no longer logged.

The commit message says "A valid ATR_RES always carries the full 17-byte
fixed header", but an error reply is not an ATR_RES. The message also
doesn't mention any change to the failure path.

Would it work to require resp->len >= 1 before reading status in both
handlers? The resp->len >= sizeof(*rsp) check could then apply only after
status is PN533_CMD_RET_SUCCESS.

> @@ -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: Medium]
Should pn533_activate_target_nfcdep() get the same fix? This patch leaves
it unchanged, but it parses the ATR_RES from PN533_CMD_IN_ATR in the same
way:

    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;

rsp->status is read with no resp->len check. With resp->len == 0, that
read hits the trimmed DCS byte. The u16 gt_len also wraps when
resp->len < 16.

nfc_set_remote_general_bytes() narrows the wrapped value to u8, and
nfc_llcp_set_remote_gb() then rejects it. So the gt[] over-read is
mitigated, as with the two patched handlers. The unguarded status read
and the wrap are still there.

The same approach would cover this handler: a resp->len >= 1 check
before reading status, then a sizeof(*rsp) check after the success test.

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