| Message ID | 20191130200540.2461-1-xiyou.wangcong@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] netrom: fix a potential NULL pointer dereference | expand |
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.
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.
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) {
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(-)