| Message ID | 20240509113036.362290-1-ryasuoka@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v4] nfc: nci: Fix uninit-value in nci_rx_work | expand |
On Thu, May 09, 2024 at 08:30:33PM +0900, 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> I suggest giving time for Krzysztof to review this. But from my side this looks good. Reviewed-by: Simon Horman <horms@kernel.org> ...
On 09/05/2024 13:30, 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> > --- > 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 Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On Thu, 9 May 2024 20:30:33 +0900 Ryosuke Yasuoka wrote: > - if (!nci_plen(skb->data)) { > + if (!skb->len) { > kfree_skb(skb); > - kcov_remote_stop(); > - break; > + continue; the change from break to continue looks unrelated > } > - nci_ntf_packet(ndev, skb); > + if (nci_valid_size(skb, NCI_CTRL_HDR_SIZE)) > + if (nci_valid_size(skb, NCI_DATA_HDR_SIZE)) #define NCI_CTRL_HDR_SIZE 3 #define NCI_DATA_HDR_SIZE 3 you can add a BUILD_BUG_ON(NCI_CTRL_HDR_SIZE == NCI_DATA_HDR_SIZE) and save all the code duplication.
Thank you for your review. On Fri, May 10, 2024 at 07:06:13PM -0700, Jakub Kicinski wrote: > On Thu, 9 May 2024 20:30:33 +0900 Ryosuke Yasuoka wrote: > > - if (!nci_plen(skb->data)) { > > + if (!skb->len) { > > kfree_skb(skb); > > - kcov_remote_stop(); > > - break; > > + continue; > > the change from break to continue looks unrelated OK. I'll leave this break in this patch. I'll send another patch about it. > > } > > > - nci_ntf_packet(ndev, skb); > > + if (nci_valid_size(skb, NCI_CTRL_HDR_SIZE)) > > > + if (nci_valid_size(skb, NCI_DATA_HDR_SIZE)) > > > #define NCI_CTRL_HDR_SIZE 3 > #define NCI_DATA_HDR_SIZE 3 > > you can add a BUILD_BUG_ON(NCI_CTRL_HDR_SIZE == NCI_DATA_HDR_SIZE) > and save all the code duplication. > Sorry I don't get it. Do you mean I just insert BUILD_BUG_ON(NCI_CTRL_HDR_SIZE != NCI_DATA_HDR_SIZE) or insert this and clean up the code duplication like this? (It is just a draft. I just share what I mean.) I can avoid to call nci_valid_size() repeatedly inside the switch statement. static void nci_rx_work(struct work_struct *work) { ... if (!skb->len) { kfree_skb(skb); kcov_remote_stop(); break; } BUILD_BUG_ON(NCI_CTRL_HDR_SIZE != NCI_DATA_HDR_SIZE); unsigned int hdr_size = NCI_CTRL_HDR_SIZE; if (!nci_valid_size(skb, hdr_size)) { kfree_skb(skb); continue; } /* Process frame */ switch (nci_mt(skb->data)) { case NCI_MT_RSP_PKT: nci_rsp_packet(ndev, skb); break; case NCI_MT_NTF_PKT: nci_ntf_packet(ndev, skb); break;
On Sat, 11 May 2024 20:02:28 +0900 Ryosuke Yasuoka wrote: > Sorry I don't get it. Do you mean I just insert > BUILD_BUG_ON(NCI_CTRL_HDR_SIZE != NCI_DATA_HDR_SIZE) or insert this and > clean up the code duplication like this? (It is just a draft. I just > share what I mean.) I can avoid to call nci_valid_size() repeatedly > inside the switch statement. > > static void nci_rx_work(struct work_struct *work) > { > ... > if (!skb->len) { > kfree_skb(skb); > kcov_remote_stop(); > break; > } > > BUILD_BUG_ON(NCI_CTRL_HDR_SIZE != NCI_DATA_HDR_SIZE); > unsigned int hdr_size = NCI_CTRL_HDR_SIZE; > > if (!nci_valid_size(skb, hdr_size)) { > kfree_skb(skb); > continue; > } > > /* Process frame */ > switch (nci_mt(skb->data)) { > case NCI_MT_RSP_PKT: > nci_rsp_packet(ndev, skb); > break; > > case NCI_MT_NTF_PKT: > nci_ntf_packet(ndev, skb); > break; Yes, that's what I meant. I'd probably merge the nci_valid_size() check with the !skb->len check, too.
diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c index b133dc55304c..0aaff30cb68f 100644 --- a/net/nfc/nci/core.c +++ b/net/nfc/nci/core.c @@ -1463,6 +1463,16 @@ 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, unsigned int header_size) +{ + if (skb->len < header_size || + !nci_plen(skb->data) || + skb->len < header_size + nci_plen(skb->data)) { + return false; + } + return true; +} + /* ---- NCI TX Data worker thread ---- */ static void nci_tx_work(struct work_struct *work) @@ -1516,24 +1526,32 @@ 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 (!skb->len) { kfree_skb(skb); - kcov_remote_stop(); - break; + continue; } /* Process frame */ switch (nci_mt(skb->data)) { case NCI_MT_RSP_PKT: - nci_rsp_packet(ndev, skb); + if (nci_valid_size(skb, NCI_CTRL_HDR_SIZE)) + nci_rsp_packet(ndev, skb); + else + kfree_skb(skb); break; case NCI_MT_NTF_PKT: - nci_ntf_packet(ndev, skb); + if (nci_valid_size(skb, NCI_CTRL_HDR_SIZE)) + nci_ntf_packet(ndev, skb); + else + kfree_skb(skb); break; case NCI_MT_DATA_PKT: - nci_rx_data_packet(ndev, skb); + if (nci_valid_size(skb, NCI_DATA_HDR_SIZE)) + nci_rx_data_packet(ndev, skb); + else + kfree_skb(skb); break; default:
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> --- 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 | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-)