| Message ID | 157435350971.16582.7099707189039358561.stgit@john-Precision-5820-Tower |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] bpf: skmsg, fix potential psock NULL pointer dereference | expand |
On Thu, Nov 21, 2019 at 8:28 AM John Fastabend <john.fastabend@gmail.com> wrote: > > Report from Dan Carpenter, > > net/core/skmsg.c:792 sk_psock_write_space() > error: we previously assumed 'psock' could be null (see line 790) > > net/core/skmsg.c > 789 psock = sk_psock(sk); > 790 if (likely(psock && sk_psock_test_state(psock, SK_PSOCK_TX_ENABLED))) > Check for NULL > 791 schedule_work(&psock->work); > 792 write_space = psock->saved_write_space; > ^^^^^^^^^^^^^^^^^^^^^^^^ > 793 rcu_read_unlock(); > 794 write_space(sk); > > Ensure psock dereference on line 792 only occurs if psock is not null. > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Fixes: 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface") > Signed-off-by: John Fastabend <john.fastabend@gmail.com> lgtm. John, do you feel strongly about it going to net tree asap? Can it go to net-next ? The merge window is right around the corner.
Alexei Starovoitov wrote: > On Thu, Nov 21, 2019 at 8:28 AM John Fastabend <john.fastabend@gmail.com> wrote: > > > > Report from Dan Carpenter, > > > > net/core/skmsg.c:792 sk_psock_write_space() > > error: we previously assumed 'psock' could be null (see line 790) > > > > net/core/skmsg.c > > 789 psock = sk_psock(sk); > > 790 if (likely(psock && sk_psock_test_state(psock, SK_PSOCK_TX_ENABLED))) > > Check for NULL > > 791 schedule_work(&psock->work); > > 792 write_space = psock->saved_write_space; > > ^^^^^^^^^^^^^^^^^^^^^^^^ > > 793 rcu_read_unlock(); > > 794 write_space(sk); > > > > Ensure psock dereference on line 792 only occurs if psock is not null. > > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > Fixes: 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface") > > Signed-off-by: John Fastabend <john.fastabend@gmail.com> > > lgtm. > John, do you feel strongly about it going to net tree asap? > Can it go to net-next ? The merge window is right around the corner. Agree we can send it to net-next, its been in the kernel for multiple versions anyways.
From: John Fastabend <john.fastabend@gmail.com> Date: Thu, 21 Nov 2019 12:27:23 -0800 > Alexei Starovoitov wrote: >> On Thu, Nov 21, 2019 at 8:28 AM John Fastabend <john.fastabend@gmail.com> wrote: >> > >> > Report from Dan Carpenter, >> > >> > net/core/skmsg.c:792 sk_psock_write_space() >> > error: we previously assumed 'psock' could be null (see line 790) >> > >> > net/core/skmsg.c >> > 789 psock = sk_psock(sk); >> > 790 if (likely(psock && sk_psock_test_state(psock, SK_PSOCK_TX_ENABLED))) >> > Check for NULL >> > 791 schedule_work(&psock->work); >> > 792 write_space = psock->saved_write_space; >> > ^^^^^^^^^^^^^^^^^^^^^^^^ >> > 793 rcu_read_unlock(); >> > 794 write_space(sk); >> > >> > Ensure psock dereference on line 792 only occurs if psock is not null. >> > >> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> >> > Fixes: 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface") >> > Signed-off-by: John Fastabend <john.fastabend@gmail.com> >> >> lgtm. >> John, do you feel strongly about it going to net tree asap? >> Can it go to net-next ? The merge window is right around the corner. > > Agree we can send it to net-next, its been in the kernel for multiple > versions anyways. Applied to net-next, thanks.
diff --git a/net/core/skmsg.c b/net/core/skmsg.c index ad31e4e..a469d21 100644 --- a/net/core/skmsg.c +++ b/net/core/skmsg.c @@ -793,15 +793,18 @@ static void sk_psock_strp_data_ready(struct sock *sk) static void sk_psock_write_space(struct sock *sk) { struct sk_psock *psock; - void (*write_space)(struct sock *sk); + void (*write_space)(struct sock *sk) = NULL; rcu_read_lock(); psock = sk_psock(sk); - if (likely(psock && sk_psock_test_state(psock, SK_PSOCK_TX_ENABLED))) - schedule_work(&psock->work); - write_space = psock->saved_write_space; + if (likely(psock)) { + if (sk_psock_test_state(psock, SK_PSOCK_TX_ENABLED)) + schedule_work(&psock->work); + write_space = psock->saved_write_space; + } rcu_read_unlock(); - write_space(sk); + if (write_space) + write_space(sk); } int sk_psock_init_strp(struct sock *sk, struct sk_psock *psock)
Report from Dan Carpenter, net/core/skmsg.c:792 sk_psock_write_space() error: we previously assumed 'psock' could be null (see line 790) net/core/skmsg.c 789 psock = sk_psock(sk); 790 if (likely(psock && sk_psock_test_state(psock, SK_PSOCK_TX_ENABLED))) Check for NULL 791 schedule_work(&psock->work); 792 write_space = psock->saved_write_space; ^^^^^^^^^^^^^^^^^^^^^^^^ 793 rcu_read_unlock(); 794 write_space(sk); Ensure psock dereference on line 792 only occurs if psock is not null. Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Fixes: 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface") Signed-off-by: John Fastabend <john.fastabend@gmail.com> --- net/core/skmsg.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)