syzbot reported the following uninit-value access issue [1]
nci_rx_work() parses received packet from ndev->rx_q. It should be
validated header size, payload size and total packet size before
processing the packet. If an invalid packet is detected, it should be
silently discarded.
Fixes: d24b03535e5e ("nfc: nci: Fix uninit-value in nci_dev_up and nci_ntf_packet")
Reported-and-tested-by: [email protected]
Closes: https://syzkaller.appspot.com/bug?extid=d7b4dc6cd50410152534 [1]
Signed-off-by: Ryosuke Yasuoka <[email protected]>
---
v5
- As Jakub pointed out, add BUILD_BUG_ON() and make the patch simpler.
All validating packet size has been done in nci_valid_size() before
switch statement.
- Also, v4 patch changed break to continue when the invalid packet is
detected. Since it is unrelated to this patch, leave it in this patch.
This fix was proposed in another patch.
v4
- v3 patch uses goto statement and it makes codes complicated. So this
patch simply calls kfree_skb inside loop and remove goto statement.
- [2] inserted kcov_remote_stop() to fix kcov check. However, as we
discuss about my v3 patch [3], it should not exit the for statement
and should continue processing subsequent packets. This patch removes
them and simply insert continue statement.
[2] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=19e35f24750d
v3
https://lore.kernel.org/netdev/[email protected]/T/
- As Simon pointed out, the valid packets will reach invalid_pkt_free
and kfree_skb(skb) after being handled correctly in switch statement.
It can lead to double free issues, which is not intended. So this patch
uses "continue" instead of "break" in switch statement.
- In the current implementation, once zero payload size is detected, the
for statement exits. It should continue processing subsequent packets.
So this patch just frees skb in invalid_pkt_free when the invalid
packets are detected. [3]
v2
https://lore.kernel.org/lkml/[email protected]/T/
- The v1 patch only checked whether skb->len is zero. This patch also
checks header size, payload size and total packet size.
v1
https://lore.kernel.org/linux-kernel/CANn89iJrQevxPFLCj2P=U+XSisYD0jqrUQpa=zWMXTjj5+RriA@mail.gmail.com/T/
net/nfc/nci/core.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c
index b133dc55304c..7a9897fbf4f4 100644
--- a/net/nfc/nci/core.c
+++ b/net/nfc/nci/core.c
@@ -1463,6 +1463,19 @@ int nci_core_ntf_packet(struct nci_dev *ndev, __u16 opcode,
ndev->ops->n_core_ops);
}
+static bool nci_valid_size(struct sk_buff *skb)
+{
+ BUILD_BUG_ON(NCI_CTRL_HDR_SIZE != NCI_DATA_HDR_SIZE);
+ unsigned int hdr_size = NCI_CTRL_HDR_SIZE;
+
+ if (skb->len < hdr_size ||
+ !nci_plen(skb->data) ||
+ skb->len < hdr_size + nci_plen(skb->data)) {
+ return false;
+ }
+ return true;
+}
+
/* ---- NCI TX Data worker thread ---- */
static void nci_tx_work(struct work_struct *work)
@@ -1516,7 +1529,7 @@ static void nci_rx_work(struct work_struct *work)
nfc_send_to_raw_sock(ndev->nfc_dev, skb,
RAW_PAYLOAD_NCI, NFC_DIRECTION_RX);
- if (!nci_plen(skb->data)) {
+ if (!nci_valid_size(skb)) {
kfree_skb(skb);
kcov_remote_stop();
break;
--
2.44.0
On 19/05/2024 11:43, Ryosuke Yasuoka wrote:
> syzbot reported the following uninit-value access issue [1]
>
> nci_rx_work() parses received packet from ndev->rx_q. It should be
> validated header size, payload size and total packet size before
> processing the packet. If an invalid packet is detected, it should be
> silently discarded.
>
> Fixes: d24b03535e5e ("nfc: nci: Fix uninit-value in nci_dev_up and nci_ntf_packet")
> Reported-and-tested-by: [email protected]
> Closes: https://syzkaller.appspot.com/bug?extid=d7b4dc6cd50410152534 [1]
> Signed-off-by: Ryosuke Yasuoka <[email protected]>
> ---
> v5
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
Hello:
This patch was applied to netdev/net.git (main)
by David S. Miller <[email protected]>:
On Sun, 19 May 2024 18:43:03 +0900 you wrote:
> syzbot reported the following uninit-value access issue [1]
>
> nci_rx_work() parses received packet from ndev->rx_q. It should be
> validated header size, payload size and total packet size before
> processing the packet. If an invalid packet is detected, it should be
> silently discarded.
>
> [...]
Here is the summary with links:
- [net,v5] nfc: nci: Fix uninit-value in nci_rx_work
https://git.kernel.org/netdev/net/c/e4a87abf5885
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html