[Patch net] netrom: fix a potential NULL pointer dereference

[Patch net] netrom: fix a potential NULL pointer dereference

[Cong Wang]

It is possible that the skb gets removed between skb_peek() and
skb_dequeue(). So we should just check the return value of
skb_dequeue().  Otherwise, skb_clone() may get a NULL pointer.

Technically, this should be protected by sock lock, but netrom
doesn't use it correctly. It is harder to fix sock lock than just
fixing this.

Reported-by: syzbot+9fe8e3f6c64aa5e5d82c@syzkaller.appspotmail.com
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/netrom/nr_out.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/netrom/nr_out.c b/net/netrom/nr_out.c
index 44929657f5b7..9491a0c02bce 100644
--- a/net/netrom/nr_out.c
+++ b/net/netrom/nr_out.c
@@ -131,9 +131,6 @@ void nr_kick(struct sock *sk)
 	if (nr->condition & NR_COND_PEER_RX_BUSY)
 		return;
 
-	if (!skb_peek(&sk->sk_write_queue))
-		return;
-
 	start = (skb_peek(&nr->ack_queue) == NULL) ? nr->va : nr->vs;
 	end   = (nr->va + nr->window) % NR_MODULUS;
 
@@ -151,6 +148,8 @@ void nr_kick(struct sock *sk)
 	 * Dequeue the frame and copy it.
 	 */
 	skb = skb_dequeue(&sk->sk_write_queue);
+	if (!skb)
+		return;
 
 	do {
 		if ((skbn = skb_clone(skb, GFP_ATOMIC)) == NULL) {
-- 
2.21.0

Re: [Patch net] netrom: fix a potential NULL pointer dereference

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Sat, 30 Nov 2019 12:05:40 -0800

> It is possible that the skb gets removed between skb_peek() and
> skb_dequeue(). So we should just check the return value of
> skb_dequeue().  Otherwise, skb_clone() may get a NULL pointer.
> 
> Technically, this should be protected by sock lock, but netrom
> doesn't use it correctly. It is harder to fix sock lock than just
> fixing this.
> 
> Reported-by: syzbot+9fe8e3f6c64aa5e5d82c@syzkaller.appspotmail.com
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

This is really bogus because it also means that all of the other
state such as the ack_queue, nr->va, nr->vs, nr->window can also
change meanwhile.

And these determine whether a dequeue should be done at all, and
I'm sure some range violations are possible as a result as well.

This code is really terminally broken and just adding this check
will leave many other other obvious bugs here that syzbot will
trigger eventually.

Sorry I'm not applying this, it's a hack that leaves obvious remaining
problems in the code.

Re: [Patch net] netrom: fix a potential NULL pointer dereference

[Cong Wang]

On Sat, Nov 30, 2019 at 12:31 PM David Miller <davem@davemloft.net> wrote:
>
> From: Cong Wang <xiyou.wangcong@gmail.com>
> Date: Sat, 30 Nov 2019 12:05:40 -0800
>
> > It is possible that the skb gets removed between skb_peek() and
> > skb_dequeue(). So we should just check the return value of
> > skb_dequeue().  Otherwise, skb_clone() may get a NULL pointer.
> >
> > Technically, this should be protected by sock lock, but netrom
> > doesn't use it correctly. It is harder to fix sock lock than just
> > fixing this.
> >
> > Reported-by: syzbot+9fe8e3f6c64aa5e5d82c@syzkaller.appspotmail.com
> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>
> This is really bogus because it also means that all of the other
> state such as the ack_queue, nr->va, nr->vs, nr->window can also
> change meanwhile.
>
> And these determine whether a dequeue should be done at all, and
> I'm sure some range violations are possible as a result as well.
>
> This code is really terminally broken and just adding this check
> will leave many other other obvious bugs here that syzbot will
> trigger eventually.

Yes, this is what I meant by mentioning sock lock above, it is
more broken than this NULL-deref as I said. It is not worth time
to audit sock lock for netrom, so I decided to just fix this single
crash.

Thanks.