Re: [PATCH net v2] nfc: digital: check DEP_REQ length before reading DID byte

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 1

- [Medium] The patch closes the 1-byte read past valid skb data in
  digital_tg_recv_dep_req(), but the identical unchecked-read pattern on…
- [Low] Commit message rationale is factually wrong, and the retained
  check it justifies is now dead code.

Pre-existing issues:
- [Medium] DEP wire-format mismatch for the optional DID byte in the
  target transmit path.

--

nfc: digital: check DEP_REQ length before reading DID byte

This reorders the DID check in digital_tg_recv_dep_req() so that the
resp->len test runs before resp->data[size] is dereferenced, avoiding a
one byte read past the valid skb data for a 3-byte DEP_REQ with the DID
bit set in the PFB.

No Link: tags present in the commit header.

A question about this part of the changelog first:

>     Reorder the condition so that the length check (resp->len < size + 1)
>     comes first and short-circuits before resp->data[size] is accessed.
>     The existing fallthrough to the later "size > resp->len" check is
>     preserved for the NAD case.

[Severity: Low]
Is that last sentence accurate?  In digital_tg_recv_dep_req() the NAD
branch never falls through to the later test, it returns immediately and
leaves size untouched:

	if (DIGITAL_NFC_DEP_NAD_BIT_SET(pfb)) {
		rc = -EIO;
		goto exit;
	}

	if (size > resp->len) {
		rc = -EIO;
		goto exit;
	}

Also, after this change, can "size > resp->len" ever be true?  Without the
DID bit size stays at sizeof(struct digital_dep_req_res) and the earlier
"resp->len < size" test already guaranteed resp->len >= size.  With the DID
bit the new guard requires resp->len >= size + 1 before size++.  So the
check that the changelog says is being preserved looks unreachable now.

Could the changelog be reworded to describe what the retained check
actually covers, and could the check itself be dropped or kept with a
comment saying why?

> diff --git a/net/nfc/digital_dep.c b/net/nfc/digital_dep.c
> index 3982fa084737f..ae5407fca12d4 100644
> --- a/net/nfc/digital_dep.c
> +++ b/net/nfc/digital_dep.c
> @@ -1117,12 +1117,12 @@ static void digital_tg_recv_dep_req(struct nfc_digital_dev *ddev, void *arg,
>  	pfb = dep_req->pfb;
>  
>  	if (DIGITAL_NFC_DEP_DID_BIT_SET(pfb)) {
> -		if (ddev->did && (ddev->did == resp->data[3])) {
> -			size++;
> -		} else {
> +		if (resp->len < size + 1 || !ddev->did ||
> +		    ddev->did != resp->data[size]) {
>  			rc = -EIO;
>  			goto exit;
>  		}
> +		size++;
>  	} else if (ddev->did) {
>  		rc = -EIO;
>  		goto exit;

[Severity: Medium]
Does this leave the same read pattern open in the direct caller?  The
first DEP_REQ after ATR_RES arrives in digital_tg_send_atr_res_complete(),
which touches resp->data[0] and then resp->data[2] or resp->data[3] with
no resp->len check at all before dispatching:

net/nfc/digital_dep.c:digital_tg_send_atr_res_complete() {
	...
	offset = 2;
	if (resp->data[0] == DIGITAL_NFC_DEP_NFCA_SOD_SB)
		offset++;

	ddev->atn_count = 0;

	if (resp->data[offset] == DIGITAL_CMD_PSL_REQ)
		digital_tg_recv_psl_req(ddev, arg, resp);
	else
		digital_tg_recv_dep_req(ddev, arg, resp);
}

For a frame shorter than offset + 1 bytes, those reads happen before the
new guard in digital_tg_recv_dep_req() runs, and the stale byte also picks
which handler is called.  That short frames do reach this layer is what
the sibling handler already assumes:

net/nfc/digital_dep.c:digital_tg_recv_atr_req() {
	...
	if (!resp->len) {
		rc = -EIO;
		goto exit;
	}

	if (resp->data[0] == DIGITAL_NFC_DEP_NFCA_SOD_SB) {
	...
}

The initiator side has the same shape in digital_in_recv_dep_res().  Only
"resp->len < size" with size == sizeof(struct digital_dep_req_res) is
validated, then skb_pull(resp, size) leaves resp->len == 0 for a 3-byte
DEP_RES, and the RTOX path reads resp->data[0]:

net/nfc/digital_dep.c:digital_in_recv_dep_res() {
	...
	skb_pull(resp, size);
	...
	case DIGITAL_NFC_DEP_PFB_SUPERVISOR_PDU:
		...
		if (ddev->atn_count || ddev->nack_count) {
			PROTOCOL_ERR("14.12.4.4");
			rc = -EIO;
			goto error;
		}

		rtox = DIGITAL_NFC_DEP_RTOX_VALUE(resp->data[0]);
	...
}

atn_count and nack_count are both cleared earlier in that same function,
so the PROTOCOL_ERR("14.12.4.4") test does not block a 3-byte supervisor
DEP_RES with the timeout bit set, and the value read then feeds
digital_in_send_rtox().

Since this is a stable-tagged fix for reads past the valid skb data on
crafted DEP frames, would it make sense to cover these two sites in the
same series rather than only the DID byte?

[Severity: Medium]
This isn't a bug introduced by this patch, but while looking at the DID
handling here, the transmit side appears to place the DID byte in a
different position than the parser above expects.

digital_tg_recv_dep_req() treats the byte right after DIR/CMD/PFB as the
DID and increments size before skb_pull(), i.e. it expects
[DIR][CMD][PFB][DID][payload].

digital_tg_send_dep_res() pushes the header onto an skb that already
holds the payload and then appends the DID with skb_put_data(), which
extends the tail:

net/nfc/digital_dep.c:digital_tg_send_dep_res() {
	skb_push(skb, sizeof(struct digital_dep_req_res));

	dep_res = (struct digital_dep_req_res *)skb->data;
	...
	if (ddev->did) {
		dep_res->pfb |= DIGITAL_NFC_DEP_PFB_DID_BIT;

		skb_put_data(skb, &ddev->did, sizeof(ddev->did));
	}
	...
}

So a session that negotiated a non-zero DID (ddev->did = atr_req->did in
digital_tg_recv_atr_req()) transmits [DIR][CMD][PFB][payload][DID] for
every non-empty DEP_RES, and digital_send_dep_data_prep() then chains
that buffer.  digital_tg_send_atn() and digital_tg_send_ack() happen to
produce the right layout only because their skb is empty when
skb_put_data() runs.

Is the target really supposed to emit the DID after the payload here, or
should digital_tg_send_dep_res() reserve the DID byte in the skb_push()
and write it directly after the PFB?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/AS8P251MB0001F45C42BBD0D16D7B30ABC8BD2%40AS8P251MB0001.EURP251.PROD.OUTLOOK.COM