| Message ID | 20250108180617.154053-3-sgarzare@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | vsock: some fixes due to transport de-assignment | expand |
On Wed, Jan 08, 2025 at 07:06:17PM +0100, Stefano Garzarella wrote: > Some of the core functions can only be called if the transport > has been assigned. > > As Michal reported, a socket might have the transport at NULL, > for example after a failed connect(), causing the following trace: > > BUG: kernel NULL pointer dereference, address: 00000000000000a0 > #PF: supervisor read access in kernel mode > #PF: error_code(0x0000) - not-present page > PGD 12faf8067 P4D 12faf8067 PUD 113670067 PMD 0 > Oops: Oops: 0000 [#1] PREEMPT SMP NOPTI > CPU: 15 UID: 0 PID: 1198 Comm: a.out Not tainted 6.13.0-rc2+ > RIP: 0010:vsock_connectible_has_data+0x1f/0x40 > Call Trace: > vsock_bpf_recvmsg+0xca/0x5e0 > sock_recvmsg+0xb9/0xc0 > __sys_recvfrom+0xb3/0x130 > __x64_sys_recvfrom+0x20/0x30 > do_syscall_64+0x93/0x180 > entry_SYSCALL_64_after_hwframe+0x76/0x7e > > So we need to check the `vsk->transport` in vsock_bpf_recvmsg(), > especially for connected sockets (stream/seqpacket) as we already > do in __vsock_connectible_recvmsg(). > > Fixes: 634f1a7110b4 ("vsock: support sockmap") > Reported-by: Michal Luczaj <mhal@rbox.co> > Closes: https://lore.kernel.org/netdev/5ca20d4c-1017-49c2-9516-f6f75fd331e9@rbox.co/ > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > --- > net/vmw_vsock/vsock_bpf.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/net/vmw_vsock/vsock_bpf.c b/net/vmw_vsock/vsock_bpf.c > index 4aa6e74ec295..f201d9eca1df 100644 > --- a/net/vmw_vsock/vsock_bpf.c > +++ b/net/vmw_vsock/vsock_bpf.c > @@ -77,6 +77,7 @@ static int vsock_bpf_recvmsg(struct sock *sk, struct msghdr *msg, > size_t len, int flags, int *addr_len) > { > struct sk_psock *psock; > + struct vsock_sock *vsk; > int copied; > > psock = sk_psock_get(sk); > @@ -84,6 +85,13 @@ static int vsock_bpf_recvmsg(struct sock *sk, struct msghdr *msg, > return __vsock_recvmsg(sk, msg, len, flags); > > lock_sock(sk); > + vsk = vsock_sk(sk); > + > + if (!vsk->transport) { > + copied = -ENODEV; > + goto out; > + } > + > if (vsock_has_data(sk, psock) && sk_psock_queue_empty(psock)) { > release_sock(sk); > sk_psock_put(sk, psock); > @@ -108,6 +116,7 @@ static int vsock_bpf_recvmsg(struct sock *sk, struct msghdr *msg, > copied = sk_msg_recvmsg(sk, psock, msg, len, flags); > } > > +out: > release_sock(sk); > sk_psock_put(sk, psock); > > -- > 2.47.1 > Looks good to me. Reviewed-by: Hyunwoo Kim <v4bel@theori.io> Regards, Hyunwoo Kim
On Wed, Jan 08, 2025 at 07:06:17PM +0100, Stefano Garzarella wrote: > Some of the core functions can only be called if the transport > has been assigned. > > As Michal reported, a socket might have the transport at NULL, > for example after a failed connect(), causing the following trace: > > BUG: kernel NULL pointer dereference, address: 00000000000000a0 > #PF: supervisor read access in kernel mode > #PF: error_code(0x0000) - not-present page > PGD 12faf8067 P4D 12faf8067 PUD 113670067 PMD 0 > Oops: Oops: 0000 [#1] PREEMPT SMP NOPTI > CPU: 15 UID: 0 PID: 1198 Comm: a.out Not tainted 6.13.0-rc2+ > RIP: 0010:vsock_connectible_has_data+0x1f/0x40 > Call Trace: > vsock_bpf_recvmsg+0xca/0x5e0 > sock_recvmsg+0xb9/0xc0 > __sys_recvfrom+0xb3/0x130 > __x64_sys_recvfrom+0x20/0x30 > do_syscall_64+0x93/0x180 > entry_SYSCALL_64_after_hwframe+0x76/0x7e > > So we need to check the `vsk->transport` in vsock_bpf_recvmsg(), > especially for connected sockets (stream/seqpacket) as we already > do in __vsock_connectible_recvmsg(). > > Fixes: 634f1a7110b4 ("vsock: support sockmap") > Reported-by: Michal Luczaj <mhal@rbox.co> > Closes: https://lore.kernel.org/netdev/5ca20d4c-1017-49c2-9516-f6f75fd331e9@rbox.co/ > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> Acked-by: Michael S. Tsirkin <mst@redhat.com> > --- > net/vmw_vsock/vsock_bpf.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/net/vmw_vsock/vsock_bpf.c b/net/vmw_vsock/vsock_bpf.c > index 4aa6e74ec295..f201d9eca1df 100644 > --- a/net/vmw_vsock/vsock_bpf.c > +++ b/net/vmw_vsock/vsock_bpf.c > @@ -77,6 +77,7 @@ static int vsock_bpf_recvmsg(struct sock *sk, struct msghdr *msg, > size_t len, int flags, int *addr_len) > { > struct sk_psock *psock; > + struct vsock_sock *vsk; > int copied; > > psock = sk_psock_get(sk); > @@ -84,6 +85,13 @@ static int vsock_bpf_recvmsg(struct sock *sk, struct msghdr *msg, > return __vsock_recvmsg(sk, msg, len, flags); > > lock_sock(sk); > + vsk = vsock_sk(sk); > + > + if (!vsk->transport) { > + copied = -ENODEV; > + goto out; > + } > + > if (vsock_has_data(sk, psock) && sk_psock_queue_empty(psock)) { > release_sock(sk); > sk_psock_put(sk, psock); > @@ -108,6 +116,7 @@ static int vsock_bpf_recvmsg(struct sock *sk, struct msghdr *msg, > copied = sk_msg_recvmsg(sk, psock, msg, len, flags); > } > > +out: > release_sock(sk); > sk_psock_put(sk, psock); > > -- > 2.47.1
On Wed, Jan 08, 2025 at 07:06:17PM +0100, Stefano Garzarella wrote: >Some of the core functions can only be called if the transport >has been assigned. > >As Michal reported, a socket might have the transport at NULL, >for example after a failed connect(), causing the following trace: > > BUG: kernel NULL pointer dereference, address: 00000000000000a0 > #PF: supervisor read access in kernel mode > #PF: error_code(0x0000) - not-present page > PGD 12faf8067 P4D 12faf8067 PUD 113670067 PMD 0 > Oops: Oops: 0000 [#1] PREEMPT SMP NOPTI > CPU: 15 UID: 0 PID: 1198 Comm: a.out Not tainted 6.13.0-rc2+ > RIP: 0010:vsock_connectible_has_data+0x1f/0x40 > Call Trace: > vsock_bpf_recvmsg+0xca/0x5e0 > sock_recvmsg+0xb9/0xc0 > __sys_recvfrom+0xb3/0x130 > __x64_sys_recvfrom+0x20/0x30 > do_syscall_64+0x93/0x180 > entry_SYSCALL_64_after_hwframe+0x76/0x7e > >So we need to check the `vsk->transport` in vsock_bpf_recvmsg(), >especially for connected sockets (stream/seqpacket) as we already >do in __vsock_connectible_recvmsg(). > >Fixes: 634f1a7110b4 ("vsock: support sockmap") >Reported-by: Michal Luczaj <mhal@rbox.co> >Closes: https://lore.kernel.org/netdev/5ca20d4c-1017-49c2-9516-f6f75fd331e9@rbox.co/ >Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >--- > net/vmw_vsock/vsock_bpf.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > >diff --git a/net/vmw_vsock/vsock_bpf.c b/net/vmw_vsock/vsock_bpf.c >index 4aa6e74ec295..f201d9eca1df 100644 >--- a/net/vmw_vsock/vsock_bpf.c >+++ b/net/vmw_vsock/vsock_bpf.c >@@ -77,6 +77,7 @@ static int vsock_bpf_recvmsg(struct sock *sk, struct msghdr *msg, > size_t len, int flags, int *addr_len) > { > struct sk_psock *psock; >+ struct vsock_sock *vsk; > int copied; > > psock = sk_psock_get(sk); >@@ -84,6 +85,13 @@ static int vsock_bpf_recvmsg(struct sock *sk, struct msghdr *msg, > return __vsock_recvmsg(sk, msg, len, flags); > > lock_sock(sk); >+ vsk = vsock_sk(sk); >+ >+ if (!vsk->transport) { >+ copied = -ENODEV; >+ goto out; >+ } >+ > if (vsock_has_data(sk, psock) && sk_psock_queue_empty(psock)) { > release_sock(sk); > sk_psock_put(sk, psock); >@@ -108,6 +116,7 @@ static int vsock_bpf_recvmsg(struct sock *sk, struct msghdr *msg, > copied = sk_msg_recvmsg(sk, psock, msg, len, flags); > } > >+out: > release_sock(sk); > sk_psock_put(sk, psock); > >-- >2.47.1 > LGTM! Reviewed-By: Luigi Leonardi <leonardi@redhat.com>
On 1/8/25 19:06, Stefano Garzarella wrote: > Some of the core functions can only be called if the transport > has been assigned. > > As Michal reported, a socket might have the transport at NULL, > for example after a failed connect(), causing the following trace: > > BUG: kernel NULL pointer dereference, address: 00000000000000a0 > #PF: supervisor read access in kernel mode > #PF: error_code(0x0000) - not-present page > PGD 12faf8067 P4D 12faf8067 PUD 113670067 PMD 0 > Oops: Oops: 0000 [#1] PREEMPT SMP NOPTI > CPU: 15 UID: 0 PID: 1198 Comm: a.out Not tainted 6.13.0-rc2+ > RIP: 0010:vsock_connectible_has_data+0x1f/0x40 > Call Trace: > vsock_bpf_recvmsg+0xca/0x5e0 > sock_recvmsg+0xb9/0xc0 > __sys_recvfrom+0xb3/0x130 > __x64_sys_recvfrom+0x20/0x30 > do_syscall_64+0x93/0x180 > entry_SYSCALL_64_after_hwframe+0x76/0x7e > > So we need to check the `vsk->transport` in vsock_bpf_recvmsg(), > especially for connected sockets (stream/seqpacket) as we already > do in __vsock_connectible_recvmsg(). > > Fixes: 634f1a7110b4 ("vsock: support sockmap") > Reported-by: Michal Luczaj <mhal@rbox.co> > Closes: https://lore.kernel.org/netdev/5ca20d4c-1017-49c2-9516-f6f75fd331e9@rbox.co/ > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> Tested-by: Michal Luczaj <mhal@rbox.co> > --- > net/vmw_vsock/vsock_bpf.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/net/vmw_vsock/vsock_bpf.c b/net/vmw_vsock/vsock_bpf.c > index 4aa6e74ec295..f201d9eca1df 100644 > --- a/net/vmw_vsock/vsock_bpf.c > +++ b/net/vmw_vsock/vsock_bpf.c > @@ -77,6 +77,7 @@ static int vsock_bpf_recvmsg(struct sock *sk, struct msghdr *msg, > size_t len, int flags, int *addr_len) > { > struct sk_psock *psock; > + struct vsock_sock *vsk; > int copied; > > psock = sk_psock_get(sk); > @@ -84,6 +85,13 @@ static int vsock_bpf_recvmsg(struct sock *sk, struct msghdr *msg, > return __vsock_recvmsg(sk, msg, len, flags); > > lock_sock(sk); > + vsk = vsock_sk(sk); > + > + if (!vsk->transport) { > + copied = -ENODEV; > + goto out; > + } > + > if (vsock_has_data(sk, psock) && sk_psock_queue_empty(psock)) { > release_sock(sk); > sk_psock_put(sk, psock); > @@ -108,6 +116,7 @@ static int vsock_bpf_recvmsg(struct sock *sk, struct msghdr *msg, > copied = sk_msg_recvmsg(sk, psock, msg, len, flags); > } > > +out: > release_sock(sk); > sk_psock_put(sk, psock); >
diff --git a/net/vmw_vsock/vsock_bpf.c b/net/vmw_vsock/vsock_bpf.c index 4aa6e74ec295..f201d9eca1df 100644 --- a/net/vmw_vsock/vsock_bpf.c +++ b/net/vmw_vsock/vsock_bpf.c @@ -77,6 +77,7 @@ static int vsock_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int flags, int *addr_len) { struct sk_psock *psock; + struct vsock_sock *vsk; int copied; psock = sk_psock_get(sk); @@ -84,6 +85,13 @@ static int vsock_bpf_recvmsg(struct sock *sk, struct msghdr *msg, return __vsock_recvmsg(sk, msg, len, flags); lock_sock(sk); + vsk = vsock_sk(sk); + + if (!vsk->transport) { + copied = -ENODEV; + goto out; + } + if (vsock_has_data(sk, psock) && sk_psock_queue_empty(psock)) { release_sock(sk); sk_psock_put(sk, psock); @@ -108,6 +116,7 @@ static int vsock_bpf_recvmsg(struct sock *sk, struct msghdr *msg, copied = sk_msg_recvmsg(sk, psock, msg, len, flags); } +out: release_sock(sk); sk_psock_put(sk, psock);
Some of the core functions can only be called if the transport has been assigned. As Michal reported, a socket might have the transport at NULL, for example after a failed connect(), causing the following trace: BUG: kernel NULL pointer dereference, address: 00000000000000a0 #PF: supervisor read access in kernel mode #PF: error_code(0x0000) - not-present page PGD 12faf8067 P4D 12faf8067 PUD 113670067 PMD 0 Oops: Oops: 0000 [#1] PREEMPT SMP NOPTI CPU: 15 UID: 0 PID: 1198 Comm: a.out Not tainted 6.13.0-rc2+ RIP: 0010:vsock_connectible_has_data+0x1f/0x40 Call Trace: vsock_bpf_recvmsg+0xca/0x5e0 sock_recvmsg+0xb9/0xc0 __sys_recvfrom+0xb3/0x130 __x64_sys_recvfrom+0x20/0x30 do_syscall_64+0x93/0x180 entry_SYSCALL_64_after_hwframe+0x76/0x7e So we need to check the `vsk->transport` in vsock_bpf_recvmsg(), especially for connected sockets (stream/seqpacket) as we already do in __vsock_connectible_recvmsg(). Fixes: 634f1a7110b4 ("vsock: support sockmap") Reported-by: Michal Luczaj <mhal@rbox.co> Closes: https://lore.kernel.org/netdev/5ca20d4c-1017-49c2-9516-f6f75fd331e9@rbox.co/ Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> --- net/vmw_vsock/vsock_bpf.c | 9 +++++++++ 1 file changed, 9 insertions(+)