| Message ID | 20240519094304.518279-1-ryasuoka@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e4a87abf588536d1cdfb128595e6e680af5cf3ed |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v5] nfc: nci: Fix uninit-value in nci_rx_work | expand |
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: syzbot+d7b4dc6cd50410152534@syzkaller.appspotmail.com > Closes: https://syzkaller.appspot.com/bug?extid=d7b4dc6cd50410152534 [1] > Signed-off-by: Ryosuke Yasuoka <ryasuoka@redhat.com> > --- > v5 Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
Hello: This patch was applied to netdev/net.git (main) by David S. Miller <davem@davemloft.net>: 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!
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;
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: syzbot+d7b4dc6cd50410152534@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=d7b4dc6cd50410152534 [1] Signed-off-by: Ryosuke Yasuoka <ryasuoka@redhat.com> --- 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/20240502082323.250739-1-ryasuoka@redhat.com/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/20240428134525.GW516117@kernel.org/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(-)