| Message ID | ZhJTu7qmOtTs9u2c@zeus (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] nfc: nci: Fix uninit-value in nci_rx_work | expand |
On Sun, Apr 7, 2024 at 10:05 AM Ryosuke Yasuoka <ryasuoka@redhat.com> wrote: > > syzbot reported the following uninit-value access issue [1] > > nci_rx_work() parses received packet from ndev->rx_q. It should be > checked skb->len is non-zero to verify if it is valid before processing > the packet. If skb->len is zero but skb->data is not, such packet is > invalid and 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> > --- > net/nfc/nci/core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c > index 0d26c8ec9993..b7a020484131 100644 > --- a/net/nfc/nci/core.c > +++ b/net/nfc/nci/core.c > @@ -1516,7 +1516,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 (!skb->len || !nci_plen(skb->data)) { #define nci_plen(hdr) (__u8)((hdr)[2]) So your patch will not help if skb->len is 1 or 2.
Thank you for your review, Eric. On Mon, Apr 08, 2024 at 11:55:54AM +0200, Eric Dumazet wrote: > On Sun, Apr 7, 2024 at 10:05 AM Ryosuke Yasuoka <ryasuoka@redhat.com> wrote: > > > > syzbot reported the following uninit-value access issue [1] > > > > nci_rx_work() parses received packet from ndev->rx_q. It should be > > checked skb->len is non-zero to verify if it is valid before processing > > the packet. If skb->len is zero but skb->data is not, such packet is > > invalid and 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> > > --- > > net/nfc/nci/core.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c > > index 0d26c8ec9993..b7a020484131 100644 > > --- a/net/nfc/nci/core.c > > +++ b/net/nfc/nci/core.c > > @@ -1516,7 +1516,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 (!skb->len || !nci_plen(skb->data)) { > > #define nci_plen(hdr) (__u8)((hdr)[2]) > > So your patch will not help if skb->len is 1 or 2. Exactly. I've reviewed my patch and here's a draft. I'd like to hear your and all member's opinion. Note this code currently redundant. Let me explain why I've written it this way. I'll refine it before sending the v2 patch. diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c index b7a020484131..89bdb959080c 100644 --- a/net/nfc/nci/core.c +++ b/net/nfc/nci/core.c @@ -1516,7 +1516,22 @@ 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 (!skb->len || !nci_plen(skb->data)) { + // FIXME: hardcoded + if (skb->len < 3) { + /* packet is too small */ + kfree_skb(skb); + break; + } + + if (!nci_plen(skb->data)) { + /* payload should not be zero */ + kfree_skb(skb); + break; + } + + // FIXME: hardcoded + if (skb->len < 3 + nci_plen(skb->data)) { + /* packet length is not match with actual packet size */ kfree_skb(skb); break; } Before refering to message type in nci_mt(skb->data), we need to check a couple of things below. 1. check if skb->len is larger than 3 Octets, which is header size of NCI packet. Since the payload size is in 3rd octet of header, We should check in advance. 2. check if nci packet payload is non zero as my previous fix (03456156). 3. check if skb->len is match with actual NCI packet size Could you check if these conditions are reasonable? If yes, then I need to remove hardcorded "3", which represents the header size of NCI packet. Now we can use NCI_CTRL_HDR_SIZE and NCI_DATA_HDR_SIZE instead of the hardcorded value. However, we have no way to know if this NCI packet is data or ctrl packet before refering to message type in nci_mt(skb->data). In the current specification, since both of header sizes are the same, we can check like this. - if (skb->len < 3) { + if (skb->len < NCI_CTRL_HDR_SIZE || + skb->len < NCI_DATA_HDR_SIZE) { But it has potential bug that packet will drop unexpectedly if either ctrl or data header size become larger. Do you have any good idea to implement this condition? Regards, Ryosuke
diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c index 0d26c8ec9993..b7a020484131 100644 --- a/net/nfc/nci/core.c +++ b/net/nfc/nci/core.c @@ -1516,7 +1516,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 (!skb->len || !nci_plen(skb->data)) { kfree_skb(skb); break; }
syzbot reported the following uninit-value access issue [1] nci_rx_work() parses received packet from ndev->rx_q. It should be checked skb->len is non-zero to verify if it is valid before processing the packet. If skb->len is zero but skb->data is not, such packet is invalid and 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> --- net/nfc/nci/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)