| Message ID | 158861290407.14306.5327773422227552482.stgit@john-Precision-5820-Tower |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | sockmap, fix for some error paths with helpers | expand |
On Mon, May 04, 2020 at 10:21:44AM -0700, John Fastabend wrote: > In bpf_tcp_ingress we used apply_bytes to subtract bytes from sg.size > which is used to track total bytes in a message. But this is not > correct because apply_bytes is itself modified in the main loop doing > the mem_charge. > > Then at the end of this we have sg.size incorrectly set and out of > sync with actual sk values. Then we can get a splat if we try to > cork the data later and again try to redirect the msg to ingress. To > fix instead of trying to track msg.size do the easy thing and include > it as part of the sk_msg_xfer logic so that when the msg is moved the > sg.size is always correct. > > To reproduce the below users will need ingress + cork and hit an > error path that will then try to 'free' the skmsg. > > [ 173.699981] BUG: KASAN: null-ptr-deref in sk_msg_free_elem+0xdd/0x120 > [ 173.699987] Read of size 8 at addr 0000000000000008 by task test_sockmap/5317 > > [ 173.700000] CPU: 2 PID: 5317 Comm: test_sockmap Tainted: G I 5.7.0-rc1+ #43 > [ 173.700005] Hardware name: Dell Inc. Precision 5820 Tower/002KVM, BIOS 1.9.2 01/24/2019 > [ 173.700009] Call Trace: > [ 173.700021] dump_stack+0x8e/0xcb > [ 173.700029] ? sk_msg_free_elem+0xdd/0x120 > [ 173.700034] ? sk_msg_free_elem+0xdd/0x120 > [ 173.700042] __kasan_report+0x102/0x15f > [ 173.700052] ? sk_msg_free_elem+0xdd/0x120 > [ 173.700060] kasan_report+0x32/0x50 > [ 173.700070] sk_msg_free_elem+0xdd/0x120 > [ 173.700080] __sk_msg_free+0x87/0x150 > [ 173.700094] tcp_bpf_send_verdict+0x179/0x4f0 > [ 173.700109] tcp_bpf_sendpage+0x3ce/0x5d0 > > Fixes: 604326b41a6fb ("bpf, sockmap: convert to generic sk_msg interface") > Signed-off-by: John Fastabend <john.fastabend@gmail.com> Acked-by: Martin KaFai Lau <kafai@fb.com>
diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h index 8a709f6..ad31c9f 100644 --- a/include/linux/skmsg.h +++ b/include/linux/skmsg.h @@ -187,6 +187,7 @@ static inline void sk_msg_xfer(struct sk_msg *dst, struct sk_msg *src, dst->sg.data[which] = src->sg.data[which]; dst->sg.data[which].length = size; dst->sg.size += size; + src->sg.size -= size; src->sg.data[which].length -= size; src->sg.data[which].offset += size; } diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c index ff96466..629aaa9a 100644 --- a/net/ipv4/tcp_bpf.c +++ b/net/ipv4/tcp_bpf.c @@ -125,7 +125,6 @@ static int bpf_tcp_ingress(struct sock *sk, struct sk_psock *psock, if (!ret) { msg->sg.start = i; - msg->sg.size -= apply_bytes; sk_psock_queue_msg(psock, tmp); sk_psock_data_ready(sk, psock); } else {
In bpf_tcp_ingress we used apply_bytes to subtract bytes from sg.size which is used to track total bytes in a message. But this is not correct because apply_bytes is itself modified in the main loop doing the mem_charge. Then at the end of this we have sg.size incorrectly set and out of sync with actual sk values. Then we can get a splat if we try to cork the data later and again try to redirect the msg to ingress. To fix instead of trying to track msg.size do the easy thing and include it as part of the sk_msg_xfer logic so that when the msg is moved the sg.size is always correct. To reproduce the below users will need ingress + cork and hit an error path that will then try to 'free' the skmsg. [ 173.699981] BUG: KASAN: null-ptr-deref in sk_msg_free_elem+0xdd/0x120 [ 173.699987] Read of size 8 at addr 0000000000000008 by task test_sockmap/5317 [ 173.700000] CPU: 2 PID: 5317 Comm: test_sockmap Tainted: G I 5.7.0-rc1+ #43 [ 173.700005] Hardware name: Dell Inc. Precision 5820 Tower/002KVM, BIOS 1.9.2 01/24/2019 [ 173.700009] Call Trace: [ 173.700021] dump_stack+0x8e/0xcb [ 173.700029] ? sk_msg_free_elem+0xdd/0x120 [ 173.700034] ? sk_msg_free_elem+0xdd/0x120 [ 173.700042] __kasan_report+0x102/0x15f [ 173.700052] ? sk_msg_free_elem+0xdd/0x120 [ 173.700060] kasan_report+0x32/0x50 [ 173.700070] sk_msg_free_elem+0xdd/0x120 [ 173.700080] __sk_msg_free+0x87/0x150 [ 173.700094] tcp_bpf_send_verdict+0x179/0x4f0 [ 173.700109] tcp_bpf_sendpage+0x3ce/0x5d0 Fixes: 604326b41a6fb ("bpf, sockmap: convert to generic sk_msg interface") Signed-off-by: John Fastabend <john.fastabend@gmail.com> --- include/linux/skmsg.h | 1 + net/ipv4/tcp_bpf.c | 1 - 2 files changed, 1 insertion(+), 1 deletion(-)