| Message ID | 20220809094915.150391-1-liujian56@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 583585e48d965338e73e1eb383768d16e0922d73 |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,v2] skmsg: Fix wrong last sg check in sk_msg_recvmsg() | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next-PR | success | PR summary |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Single patches do not need cover letters |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 2 this patch: 2 |
| netdev/cc_maintainers | success | CCed 10 of 10 maintainers |
| netdev/build_clang | success | Errors and warnings before: 5 this patch: 5 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 2 this patch: 2 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 16 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
| bpf/vmtest-bpf-next-VM_Test-3 | success | Logs for Kernel LATEST on z15 with gcc |
| bpf/vmtest-bpf-next-VM_Test-4 | success | Logs for llvm-toolchain |
| bpf/vmtest-bpf-next-VM_Test-5 | success | Logs for set-matrix |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for Kernel LATEST on ubuntu-latest with gcc |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for Kernel LATEST on ubuntu-latest with llvm-16 |
Liu Jian wrote: > Fix one kernel NULL pointer dereference as below: > > [ 224.462334] Call Trace: > [ 224.462394] __tcp_bpf_recvmsg+0xd3/0x380 > [ 224.462441] ? sock_has_perm+0x78/0xa0 > [ 224.462463] tcp_bpf_recvmsg+0x12e/0x220 > [ 224.462494] inet_recvmsg+0x5b/0xd0 > [ 224.462534] __sys_recvfrom+0xc8/0x130 > [ 224.462574] ? syscall_trace_enter+0x1df/0x2e0 > [ 224.462606] ? __do_page_fault+0x2de/0x500 > [ 224.462635] __x64_sys_recvfrom+0x24/0x30 > [ 224.462660] do_syscall_64+0x5d/0x1d0 > [ 224.462709] entry_SYSCALL_64_after_hwframe+0x65/0xca > > In commit 9974d37ea75f ("skmsg: Fix invalid last sg check in > sk_msg_recvmsg()"), we change last sg check to sg_is_last(), > but in sockmap redirection case (without stream_parser/stream_verdict/ > skb_verdict), we did not mark the end of the scatterlist. Check the > sk_msg_alloc, sk_msg_page_add, and bpf_msg_push_data functions, they all > do not mark the end of sg. They are expected to use sg.end for end > judgment. So the judgment of '(i != msg_rx->sg.end)' is added back here. > > Fixes: 9974d37ea75f ("skmsg: Fix invalid last sg check in sk_msg_recvmsg()") > Signed-off-by: Liu Jian <liujian56@huawei.com> > --- OK lets take this. Thanks. Acked-by: John Fastabend <john.fastabend@gmail.com>
On Tue, Aug 09, 2022 at 05:49 PM +08, Liu Jian wrote: > Fix one kernel NULL pointer dereference as below: > > [ 224.462334] Call Trace: > [ 224.462394] __tcp_bpf_recvmsg+0xd3/0x380 > [ 224.462441] ? sock_has_perm+0x78/0xa0 > [ 224.462463] tcp_bpf_recvmsg+0x12e/0x220 > [ 224.462494] inet_recvmsg+0x5b/0xd0 > [ 224.462534] __sys_recvfrom+0xc8/0x130 > [ 224.462574] ? syscall_trace_enter+0x1df/0x2e0 > [ 224.462606] ? __do_page_fault+0x2de/0x500 > [ 224.462635] __x64_sys_recvfrom+0x24/0x30 > [ 224.462660] do_syscall_64+0x5d/0x1d0 > [ 224.462709] entry_SYSCALL_64_after_hwframe+0x65/0xca > > In commit 9974d37ea75f ("skmsg: Fix invalid last sg check in > sk_msg_recvmsg()"), we change last sg check to sg_is_last(), > but in sockmap redirection case (without stream_parser/stream_verdict/ > skb_verdict), we did not mark the end of the scatterlist. Check the > sk_msg_alloc, sk_msg_page_add, and bpf_msg_push_data functions, they all > do not mark the end of sg. They are expected to use sg.end for end > judgment. So the judgment of '(i != msg_rx->sg.end)' is added back here. > > Fixes: 9974d37ea75f ("skmsg: Fix invalid last sg check in sk_msg_recvmsg()") > Signed-off-by: Liu Jian <liujian56@huawei.com> > --- > v1->v2: change fix commit info in fixes tag and commit message. > net/core/skmsg.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/core/skmsg.c b/net/core/skmsg.c > index 81627892bdd4..385ae23580a5 100644 > --- a/net/core/skmsg.c > +++ b/net/core/skmsg.c > @@ -462,7 +462,7 @@ int sk_msg_recvmsg(struct sock *sk, struct sk_psock *psock, struct msghdr *msg, > > if (copied == len) > break; > - } while (!sg_is_last(sge)); > + } while ((i != msg_rx->sg.end) && !sg_is_last(sge)); > > if (unlikely(peek)) { > msg_rx = sk_psock_next_msg(psock, msg_rx); > @@ -472,7 +472,7 @@ int sk_msg_recvmsg(struct sock *sk, struct sk_psock *psock, struct msghdr *msg, > } > > msg_rx->sg.start = i; > - if (!sge->length && sg_is_last(sge)) { > + if (!sge->length && (i == msg_rx->sg.end || sg_is_last(sge))) { > msg_rx = sk_psock_dequeue_msg(psock); > kfree_sk_msg(msg_rx); > } Looks to me like I sk_msg_alloc does mark the end of a scatterlist: sk_msg_alloc sg_unmark_end sg_set_page sg_assign_page <- sets SG_END bit Why do we unmark the SG end in sk_msg_page_add and bpf_msg_push_data? John, do you recall? As it might take a moment to figure it out, and we want to fix the regression introduced by 9974d37ea75f ("skmsg: Fix invalid last sg check in sk_msg_recvmsg()"): Acked-by: Jakub Sitnicki <jakub@cloudflare.com>
Hello: This patch was applied to bpf/bpf.git (master) by Daniel Borkmann <daniel@iogearbox.net>: On Tue, 9 Aug 2022 17:49:15 +0800 you wrote: > Fix one kernel NULL pointer dereference as below: > > [ 224.462334] Call Trace: > [ 224.462394] __tcp_bpf_recvmsg+0xd3/0x380 > [ 224.462441] ? sock_has_perm+0x78/0xa0 > [ 224.462463] tcp_bpf_recvmsg+0x12e/0x220 > [ 224.462494] inet_recvmsg+0x5b/0xd0 > [ 224.462534] __sys_recvfrom+0xc8/0x130 > [ 224.462574] ? syscall_trace_enter+0x1df/0x2e0 > [ 224.462606] ? __do_page_fault+0x2de/0x500 > [ 224.462635] __x64_sys_recvfrom+0x24/0x30 > [ 224.462660] do_syscall_64+0x5d/0x1d0 > [ 224.462709] entry_SYSCALL_64_after_hwframe+0x65/0xca > > [...] Here is the summary with links: - [bpf-next,v2] skmsg: Fix wrong last sg check in sk_msg_recvmsg() https://git.kernel.org/bpf/bpf/c/583585e48d96 You are awesome, thank you!
diff --git a/net/core/skmsg.c b/net/core/skmsg.c index 81627892bdd4..385ae23580a5 100644 --- a/net/core/skmsg.c +++ b/net/core/skmsg.c @@ -462,7 +462,7 @@ int sk_msg_recvmsg(struct sock *sk, struct sk_psock *psock, struct msghdr *msg, if (copied == len) break; - } while (!sg_is_last(sge)); + } while ((i != msg_rx->sg.end) && !sg_is_last(sge)); if (unlikely(peek)) { msg_rx = sk_psock_next_msg(psock, msg_rx); @@ -472,7 +472,7 @@ int sk_msg_recvmsg(struct sock *sk, struct sk_psock *psock, struct msghdr *msg, } msg_rx->sg.start = i; - if (!sge->length && sg_is_last(sge)) { + if (!sge->length && (i == msg_rx->sg.end || sg_is_last(sge))) { msg_rx = sk_psock_dequeue_msg(psock); kfree_sk_msg(msg_rx); }
Fix one kernel NULL pointer dereference as below: [ 224.462334] Call Trace: [ 224.462394] __tcp_bpf_recvmsg+0xd3/0x380 [ 224.462441] ? sock_has_perm+0x78/0xa0 [ 224.462463] tcp_bpf_recvmsg+0x12e/0x220 [ 224.462494] inet_recvmsg+0x5b/0xd0 [ 224.462534] __sys_recvfrom+0xc8/0x130 [ 224.462574] ? syscall_trace_enter+0x1df/0x2e0 [ 224.462606] ? __do_page_fault+0x2de/0x500 [ 224.462635] __x64_sys_recvfrom+0x24/0x30 [ 224.462660] do_syscall_64+0x5d/0x1d0 [ 224.462709] entry_SYSCALL_64_after_hwframe+0x65/0xca In commit 9974d37ea75f ("skmsg: Fix invalid last sg check in sk_msg_recvmsg()"), we change last sg check to sg_is_last(), but in sockmap redirection case (without stream_parser/stream_verdict/ skb_verdict), we did not mark the end of the scatterlist. Check the sk_msg_alloc, sk_msg_page_add, and bpf_msg_push_data functions, they all do not mark the end of sg. They are expected to use sg.end for end judgment. So the judgment of '(i != msg_rx->sg.end)' is added back here. Fixes: 9974d37ea75f ("skmsg: Fix invalid last sg check in sk_msg_recvmsg()") Signed-off-by: Liu Jian <liujian56@huawei.com> --- v1->v2: change fix commit info in fixes tag and commit message. net/core/skmsg.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)