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: [email protected]
Closes: https://syzkaller.appspot.com/bug?extid=d7b4dc6cd50410152534 [1]
Signed-off-by: Ryosuke Yasuoka <[email protected]>
---
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)) {
kfree_skb(skb);
break;
}
--
2.44.0
On Sun, Apr 7, 2024 at 10:05 AM Ryosuke Yasuoka <[email protected]> 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: [email protected]
> Closes: https://syzkaller.appspot.com/bug?extid=d7b4dc6cd50410152534 [1]
> Signed-off-by: Ryosuke Yasuoka <[email protected]>
> ---
> 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 <[email protected]> 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: [email protected]
> > Closes: https://syzkaller.appspot.com/bug?extid=d7b4dc6cd50410152534 [1]
> > Signed-off-by: Ryosuke Yasuoka <[email protected]>
> > ---
> > 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