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@xxxxxxxxxx>님이 작성:
>
> 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@xxxxxxxxx>
>
> [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