* [PATCH 1/2] nfc: pn533: bound device-supplied lengths in the receive path
2026-06-18 2:59 [PATCH 0/2] nfc: pn533: bound device-supplied lengths in the receive path Michael Bommarito
@ 2026-06-18 2:59 ` Michael Bommarito
2026-06-18 2:59 ` [PATCH 2/2] nfc: pn533: add KUnit tests for receive-path bounds Michael Bommarito
1 sibling, 0 replies; 3+ messages in thread
From: Michael Bommarito @ 2026-06-18 2:59 UTC (permalink / raw)
To: Krzysztof Kozlowski, Dan Carpenter
Cc: Jakub Kicinski, Kees Cook, linux-kernel
The PN533 receive path trusts device-supplied length fields and reads
past the received buffer when they exceed it. The rx skb is sized to the
device's USB transfer length, so a malicious PN53x reader triggers
out-of-bounds reads in the standard/ACR122 frame validators, in Type A
target parsing (nfcid_len), and in the autopoll record walk.
Thread the received length into the rx_is_frame_valid callbacks, reject
ACR122 frames with datalen < 2, and add the missing nfcid and per-record
bounds.
This was found with static analysis and confirmed with the KUnit tests
added in the following patch.
Fixes: c46ee38620a2 ("NFC: pn533: add NXP pn533 nfc device driver")
Fixes: 9815c7cf22da ("NFC: pn533: Separate physical layer from the core implementation")
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
drivers/nfc/pn533/pn533.c | 32 ++++++++++++++++++++++++++++++--
drivers/nfc/pn533/pn533.h | 3 ++-
drivers/nfc/pn533/usb.c | 12 ++++++++++--
3 files changed, 42 insertions(+), 5 deletions(-)
diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
index d7bdbc82e2ba2..984a61a83feaf 100644
--- a/drivers/nfc/pn533/pn533.c
+++ b/drivers/nfc/pn533/pn533.c
@@ -274,11 +274,15 @@ static void pn533_std_tx_update_payload_len(void *_frame, int len)
frame->datalen += len;
}
-static bool pn533_std_rx_frame_is_valid(void *_frame, struct pn533 *dev)
+static bool pn533_std_rx_frame_is_valid(void *_frame, size_t frame_len,
+ struct pn533 *dev)
{
u8 checksum;
struct pn533_std_frame *stdf = _frame;
+ if (frame_len < sizeof(struct pn533_std_frame))
+ return false;
+
if (stdf->start_frame != cpu_to_be16(PN533_STD_FRAME_SOF))
return false;
@@ -286,6 +290,10 @@ static bool pn533_std_rx_frame_is_valid(void *_frame, struct pn533 *dev)
/* Standard frame code */
dev->ops->rx_header_len = PN533_STD_FRAME_HEADER_LEN;
+ if (frame_len < sizeof(struct pn533_std_frame) +
+ stdf->datalen + PN533_STD_FRAME_TAIL_LEN)
+ return false;
+
checksum = pn533_std_checksum(stdf->datalen);
if (checksum != stdf->datalen_checksum)
return false;
@@ -299,6 +307,11 @@ static bool pn533_std_rx_frame_is_valid(void *_frame, struct pn533 *dev)
dev->ops->rx_header_len = PN533_EXT_FRAME_HEADER_LEN;
+ if (frame_len < sizeof(struct pn533_ext_frame) ||
+ frame_len < sizeof(struct pn533_ext_frame) +
+ be16_to_cpu(eif->datalen) + PN533_STD_FRAME_TAIL_LEN)
+ return false;
+
checksum = pn533_ext_checksum(be16_to_cpu(eif->datalen));
if (checksum != eif->datalen_checksum)
return false;
@@ -701,6 +714,10 @@ static bool pn533_target_type_a_is_valid(struct pn533_target_type_a *type_a,
if (type_a->nfcid_len > NFC_NFCID1_MAXSIZE)
return false;
+ if (sizeof(struct pn533_target_type_a) + type_a->nfcid_len >
+ (size_t)target_data_len)
+ return false;
+
return true;
}
@@ -1399,6 +1416,7 @@ static int pn533_autopoll_complete(struct pn533 *dev, void *arg,
{
struct pn532_autopoll_resp *apr;
struct nfc_target nfc_tgt;
+ size_t remaining;
u8 nbtg;
int rc;
@@ -1417,12 +1435,21 @@ static int pn533_autopoll_complete(struct pn533 *dev, void *arg,
goto stop_poll;
}
+ if (resp->len < 1)
+ return -EAGAIN;
+
nbtg = resp->data[0];
if ((nbtg > 2) || (nbtg <= 0))
return -EAGAIN;
+ remaining = resp->len - 1;
apr = (struct pn532_autopoll_resp *)&resp->data[1];
while (nbtg--) {
+ if (remaining < sizeof(struct pn532_autopoll_resp) ||
+ apr->ln < 1 ||
+ remaining < (size_t)apr->ln + 2)
+ return -EAGAIN;
+
memset(&nfc_tgt, 0, sizeof(struct nfc_target));
switch (apr->type) {
case PN532_AUTOPOLL_TYPE_ISOA:
@@ -1468,6 +1495,7 @@ static int pn533_autopoll_complete(struct pn533 *dev, void *arg,
}
dev->tgt_available_prots = nfc_tgt.supported_protocols;
+ remaining -= (size_t)apr->ln + 2;
apr = (struct pn532_autopoll_resp *)
(apr->tgdata + (apr->ln - 1));
}
@@ -2189,7 +2217,7 @@ void pn533_recv_frame(struct pn533 *dev, struct sk_buff *skb, int status)
print_hex_dump_debug("PN533 RX: ", DUMP_PREFIX_NONE, 16, 1, skb->data,
dev->ops->rx_frame_size(skb->data), false);
- if (!dev->ops->rx_is_frame_valid(skb->data, dev)) {
+ if (!dev->ops->rx_is_frame_valid(skb->data, skb->len, dev)) {
nfc_err(dev->dev, "Received an invalid frame\n");
dev->cmd->status = -EIO;
} else if (!pn533_rx_frame_is_cmd_response(dev, skb->data)) {
diff --git a/drivers/nfc/pn533/pn533.h b/drivers/nfc/pn533/pn533.h
index 09e35b8693f5a..08804f88b38a3 100644
--- a/drivers/nfc/pn533/pn533.h
+++ b/drivers/nfc/pn533/pn533.h
@@ -201,7 +201,8 @@ struct pn533_frame_ops {
int tx_header_len;
int tx_tail_len;
- bool (*rx_is_frame_valid)(void *frame, struct pn533 *dev);
+ bool (*rx_is_frame_valid)(void *frame, size_t frame_len,
+ struct pn533 *dev);
bool (*rx_frame_is_ack)(void *frame);
int (*rx_frame_size)(void *frame);
int rx_header_len;
diff --git a/drivers/nfc/pn533/usb.c b/drivers/nfc/pn533/usb.c
index efb07f944fce2..1d50600609dd4 100644
--- a/drivers/nfc/pn533/usb.c
+++ b/drivers/nfc/pn533/usb.c
@@ -312,14 +312,22 @@ static void pn533_acr122_tx_update_payload_len(void *_frame, int len)
frame->datalen += len;
}
-static bool pn533_acr122_is_rx_frame_valid(void *_frame, struct pn533 *dev)
+static bool pn533_acr122_is_rx_frame_valid(void *_frame, size_t frame_len,
+ struct pn533 *dev)
{
struct pn533_acr122_rx_frame *frame = _frame;
+ if (frame_len < sizeof(struct pn533_acr122_rx_frame))
+ return false;
+
if (frame->ccid.type != 0x83)
return false;
- if (!frame->ccid.datalen)
+ if (frame->ccid.datalen < 2)
+ return false;
+
+ if (frame_len < sizeof(struct pn533_acr122_rx_frame) +
+ frame->ccid.datalen)
return false;
if (frame->data[frame->ccid.datalen - 2] == 0x63)
--
2.53.0
^ permalink raw reply [flat|nested] 3+ messages in thread* [PATCH 2/2] nfc: pn533: add KUnit tests for receive-path bounds
2026-06-18 2:59 [PATCH 0/2] nfc: pn533: bound device-supplied lengths in the receive path Michael Bommarito
2026-06-18 2:59 ` [PATCH 1/2] " Michael Bommarito
@ 2026-06-18 2:59 ` Michael Bommarito
1 sibling, 0 replies; 3+ messages in thread
From: Michael Bommarito @ 2026-06-18 2:59 UTC (permalink / raw)
To: Krzysztof Kozlowski, Dan Carpenter
Cc: Jakub Kicinski, Kees Cook, linux-kernel
Two regression tests: a header-only standard frame declaring datalen = 8
(KASAN slab-out-of-bounds in pn533_std_rx_frame_is_valid on an unpatched
tree), and a header-only Type A target with nfcid_len = 10
(pn533_target_found_type_a returns -EPROTO once bounded). Benign controls
exercise the valid path.
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
drivers/nfc/pn533/Kconfig | 9 ++++
drivers/nfc/pn533/pn533.c | 93 +++++++++++++++++++++++++++++++++++++++
2 files changed, 102 insertions(+)
diff --git a/drivers/nfc/pn533/Kconfig b/drivers/nfc/pn533/Kconfig
index 31822b432d51c..a0ebbb75419b1 100644
--- a/drivers/nfc/pn533/Kconfig
+++ b/drivers/nfc/pn533/Kconfig
@@ -37,3 +37,12 @@ config NFC_PN532_UART
If you choose to build a module, it'll be called pn532_uart.
Say N if unsure.
+
+config NFC_PN533_KUNIT_TEST
+ bool "KUnit tests for the NFC PN533 driver" if !KUNIT_ALL_TESTS
+ depends on NFC_PN533=y && KUNIT=y
+ default KUNIT_ALL_TESTS
+ help
+ Enable the KUnit regression tests for the NFC PN533 driver,
+ covering bounds checking of device-supplied frame lengths.
+ If unsure, say N.
diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
index 984a61a83feaf..47d2b4b56bc2e 100644
--- a/drivers/nfc/pn533/pn533.c
+++ b/drivers/nfc/pn533/pn533.c
@@ -2883,3 +2883,96 @@ MODULE_AUTHOR("Waldemar Rymarkiewicz <waldemar.rymarkiewicz@tieto.com>");
MODULE_DESCRIPTION("PN533 driver ver " VERSION);
MODULE_VERSION(VERSION);
MODULE_LICENSE("GPL");
+
+#if IS_ENABLED(CONFIG_NFC_PN533_KUNIT_TEST)
+#include <kunit/test.h>
+
+/*
+ * Regression test for the out-of-bounds read in the PN533 standard-frame
+ * validator: pn533_std_rx_frame_is_valid() trusts the device-supplied
+ * datalen field and reads data[0 .. datalen-1] (the data checksum) without
+ * checking that the received buffer actually contains that many bytes. A
+ * malicious USB PN53x device returning a short transfer therefore makes the
+ * driver read past the receive buffer.
+ */
+static void pn533_std_rx_frame_short_oob_test(struct kunit *test)
+{
+ struct pn533 *dev;
+ struct pn533_frame_ops *ops;
+ struct pn533_std_frame *f;
+ const size_t hdr = sizeof(struct pn533_std_frame); /* header, no data[] */
+
+ dev = kunit_kzalloc(test, sizeof(*dev), GFP_KERNEL);
+ ops = kunit_kzalloc(test, sizeof(*ops), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, dev);
+ KUNIT_ASSERT_NOT_NULL(test, ops);
+ dev->ops = ops;
+
+ /* benign control: a correctly sized, well-formed frame validates and
+ * does not read out of bounds (proves the trigger reaches the real
+ * data-checksum path). data is all zero: data checksum = 0, and the
+ * trailing frame-checksum byte data[datalen] is already 0.
+ */
+ {
+ const u8 dl = 3;
+ const size_t full = hdr + dl + PN533_STD_FRAME_TAIL_LEN;
+ struct pn533_std_frame *gf = kunit_kzalloc(test, full, GFP_KERNEL);
+
+ KUNIT_ASSERT_NOT_NULL(test, gf);
+ gf->start_frame = cpu_to_be16(PN533_STD_FRAME_SOF);
+ gf->datalen = dl;
+ gf->datalen_checksum = pn533_std_checksum(dl);
+ KUNIT_EXPECT_TRUE(test,
+ pn533_std_rx_frame_is_valid(gf, full, dev));
+ }
+
+ /* attack: a header-only buffer that declares datalen = 8. The data
+ * checksum loop reads data[0 .. 7] starting one byte past the
+ * allocation -> KASAN slab-out-of-bounds read on an unpatched tree.
+ */
+ f = kunit_kzalloc(test, hdr, GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, f);
+ f->start_frame = cpu_to_be16(PN533_STD_FRAME_SOF);
+ f->datalen = 8;
+ f->datalen_checksum = pn533_std_checksum(8);
+ pn533_std_rx_frame_is_valid(f, hdr, dev);
+}
+
+static void pn533_target_type_a_nfcid_oob_test(struct kunit *test)
+{
+ struct nfc_target nfc_tgt;
+ struct pn533_target_type_a *ta;
+ const size_t hdr = sizeof(struct pn533_target_type_a); /* 4, no nfcid */
+
+ /* header-only target data, but nfcid_len claims the full 10 bytes ->
+ * pn533_target_found_type_a()'s memcpy reads nfcid_data[0..9], past the
+ * allocation -> KASAN slab-out-of-bounds on an unpatched tree.
+ */
+ ta = kunit_kzalloc(test, hdr, GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, ta);
+ ta->sens_res = cpu_to_be16(0x0001);
+ ta->sel_res = 0;
+ ta->nfcid_len = NFC_NFCID1_MAXSIZE; /* 10 */
+
+ /* unpatched: is_valid accepts (nfcid_len only checked vs the
+ * destination) and the memcpy reads nfcid_data[0..9] past the 4-byte
+ * buffer. The fix bounds nfcid_len against the response buffer and
+ * rejects the frame, returning -EPROTO.
+ */
+ KUNIT_EXPECT_EQ(test,
+ pn533_target_found_type_a(&nfc_tgt, (u8 *)ta, hdr),
+ -EPROTO);
+}
+
+static struct kunit_case pn533_test_cases[] = {
+ KUNIT_CASE(pn533_std_rx_frame_short_oob_test),
+ KUNIT_CASE(pn533_target_type_a_nfcid_oob_test),
+ {}
+};
+
+static struct kunit_suite pn533_test_suite = {
+ .name = "pn533",
+ .test_cases = pn533_test_cases,
+};
+kunit_test_suite(pn533_test_suite);
+#endif /* CONFIG_NFC_PN533_KUNIT_TEST */
--
2.53.0
^ permalink raw reply [flat|nested] 3+ messages in thread