[PATCH bpf-next] skmsg: Fix wrong last sg check in sk_msg_recvmsg()

[PATCH bpf-next] skmsg: Fix wrong last sg check in sk_msg_recvmsg()

[Liu Jian]

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 7303524e04af ("skmsg: Lose offset info in sk_psock_skb_ingress"),
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: 7303524e04af ("skmsg: Lose offset info in sk_psock_skb_ingress")
Signed-off-by: Liu Jian <liujian56@huawei.com>
---
 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);
 		}
-- 
2.17.1

RE: [PATCH bpf-next] skmsg: Fix wrong last sg check in sk_msg_recvmsg()

[John Fastabend]

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 7303524e04af ("skmsg: Lose offset info in sk_psock_skb_ingress"),
> 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: 7303524e04af ("skmsg: Lose offset info in sk_psock_skb_ingress")
> Signed-off-by: Liu Jian <liujian56@huawei.com>
> ---

This is the wrong fixes tag though right? We should have,

9974d37ea75f0 ("skmsg: Fix invalid last sg check in sk_msg_recvmsg()")

Fix looks OK though although its not great we have two ways
to find the last frag now. I'm going to look at getting some better
testing in place and then see if we can get to just one check.

Assuming I'm right on the fixes tag please update that.

>  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);
>  		}
> -- 
> 2.17.1
> 

RE: [PATCH bpf-next] skmsg: Fix wrong last sg check in sk_msg_recvmsg()

[liujian (CE)]

> -----Original Message-----
> From: John Fastabend [mailto:john.fastabend@gmail.com]
> Sent: Tuesday, August 9, 2022 2:58 PM
> To: liujian (CE) <liujian56@huawei.com>; john.fastabend@gmail.com;
> jakub@cloudflare.com; davem@davemloft.net; edumazet@google.com;
> kuba@kernel.org; pabeni@redhat.com; daniel@iogearbox.net;
> andrii@kernel.org; netdev@vger.kernel.org; bpf@vger.kernel.org
> Cc: liujian (CE) <liujian56@huawei.com>
> Subject: RE: [PATCH bpf-next] skmsg: Fix wrong last sg check in
> sk_msg_recvmsg()
> 
> 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 7303524e04af ("skmsg: Lose offset info in
> > sk_psock_skb_ingress"), 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: 7303524e04af ("skmsg: Lose offset info in
> > sk_psock_skb_ingress")
> > Signed-off-by: Liu Jian <liujian56@huawei.com>
> > ---
> 
> This is the wrong fixes tag though right? We should have,

I am sorry for this, and will send v2 to update the fix tag.

> 
> 9974d37ea75f0 ("skmsg: Fix invalid last sg check in sk_msg_recvmsg()")
> 
> Fix looks OK though although its not great we have two ways to find the last
> frag now. I'm going to look at getting some better testing in place and then
> see if we can get to just one check.
> 
> Assuming I'm right on the fixes tag please update that.
> 
> >  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);
> >  		}
> > --
> > 2.17.1
> >
>