sockmap, vsock: For connectible sockets allow only connected

[PATCH net 2/4] vsock/bpf: Warn on socket without transport

Posted by Michal Luczaj 10 months, 1 week ago

In the spirit of commit 91751e248256 ("vsock: prevent null-ptr-deref in
vsock_*[has_data|has_space]"), armorize the "impossible" cases with a
warning.

Fixes: 634f1a7110b4 ("vsock: support sockmap")
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 net/vmw_vsock/af_vsock.c  | 3 +++
 net/vmw_vsock/vsock_bpf.c | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 53a081d49d28ac1c04e7f8057c8a55e7b73cc131..7e3db87ae4333cf63327ec105ca99253569bb9fe 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -1189,6 +1189,9 @@ static int vsock_read_skb(struct sock *sk, skb_read_actor_t read_actor)
 {
 	struct vsock_sock *vsk = vsock_sk(sk);
 
+	if (WARN_ON_ONCE(!vsk->transport))
+		return -ENODEV;
+
 	return vsk->transport->read_skb(vsk, read_actor);
 }
 
diff --git a/net/vmw_vsock/vsock_bpf.c b/net/vmw_vsock/vsock_bpf.c
index f201d9eca1df2f8143638cf7a4d08671e8368c11..07b96d56f3a577af71021b1b8132743554996c4f 100644
--- a/net/vmw_vsock/vsock_bpf.c
+++ b/net/vmw_vsock/vsock_bpf.c
@@ -87,7 +87,7 @@ static int vsock_bpf_recvmsg(struct sock *sk, struct msghdr *msg,
 	lock_sock(sk);
 	vsk = vsock_sk(sk);
 
-	if (!vsk->transport) {
+	if (WARN_ON_ONCE(!vsk->transport)) {
 		copied = -ENODEV;
 		goto out;
 	}

-- 
2.48.1

Re: [PATCH net 2/4] vsock/bpf: Warn on socket without transport

Posted by Stefano Garzarella 10 months ago

On Thu, Feb 13, 2025 at 12:58:50PM +0100, Michal Luczaj wrote:
>In the spirit of commit 91751e248256 ("vsock: prevent null-ptr-deref in
>vsock_*[has_data|has_space]"), armorize the "impossible" cases with a
>warning.
>
>Fixes: 634f1a7110b4 ("vsock: support sockmap")
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
> net/vmw_vsock/af_vsock.c  | 3 +++
> net/vmw_vsock/vsock_bpf.c | 2 +-
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>index 53a081d49d28ac1c04e7f8057c8a55e7b73cc131..7e3db87ae4333cf63327ec105ca99253569bb9fe 100644
>--- a/net/vmw_vsock/af_vsock.c
>+++ b/net/vmw_vsock/af_vsock.c
>@@ -1189,6 +1189,9 @@ static int vsock_read_skb(struct sock *sk, skb_read_actor_t read_actor)
> {
> 	struct vsock_sock *vsk = vsock_sk(sk);
>
>+	if (WARN_ON_ONCE(!vsk->transport))
>+		return -ENODEV;
>+
> 	return vsk->transport->read_skb(vsk, read_actor);
> }
>
>diff --git a/net/vmw_vsock/vsock_bpf.c b/net/vmw_vsock/vsock_bpf.c
>index f201d9eca1df2f8143638cf7a4d08671e8368c11..07b96d56f3a577af71021b1b8132743554996c4f 100644
>--- a/net/vmw_vsock/vsock_bpf.c
>+++ b/net/vmw_vsock/vsock_bpf.c
>@@ -87,7 +87,7 @@ static int vsock_bpf_recvmsg(struct sock *sk, struct msghdr *msg,
> 	lock_sock(sk);
> 	vsk = vsock_sk(sk);
>
>-	if (!vsk->transport) {
>+	if (WARN_ON_ONCE(!vsk->transport)) {
> 		copied = -ENODEV;
> 		goto out;
> 	}

I'm not a sockmap expert, so I don't understand why here print an
error.

Since there was already a check, I expected it to be a case that can 
happen, but instead calling `rcvmsg()` on a socket not yet connected is 
impossible?

Thanks,
Stefano

Re: [PATCH net 2/4] vsock/bpf: Warn on socket without transport

Posted by Michal Luczaj 10 months ago

On 2/17/25 11:59, Stefano Garzarella wrote:
> On Thu, Feb 13, 2025 at 12:58:50PM +0100, Michal Luczaj wrote:
>> In the spirit of commit 91751e248256 ("vsock: prevent null-ptr-deref in
>> vsock_*[has_data|has_space]"), armorize the "impossible" cases with a
>> warning.
>>
>> Fixes: 634f1a7110b4 ("vsock: support sockmap")
>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>> ---
>> net/vmw_vsock/af_vsock.c  | 3 +++
>> net/vmw_vsock/vsock_bpf.c | 2 +-
>> 2 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>> index 53a081d49d28ac1c04e7f8057c8a55e7b73cc131..7e3db87ae4333cf63327ec105ca99253569bb9fe 100644
>> --- a/net/vmw_vsock/af_vsock.c
>> +++ b/net/vmw_vsock/af_vsock.c
>> @@ -1189,6 +1189,9 @@ static int vsock_read_skb(struct sock *sk, skb_read_actor_t read_actor)
>> {
>> 	struct vsock_sock *vsk = vsock_sk(sk);
>>
>> +	if (WARN_ON_ONCE(!vsk->transport))
>> +		return -ENODEV;
>> +
>> 	return vsk->transport->read_skb(vsk, read_actor);
>> }
>>
>> diff --git a/net/vmw_vsock/vsock_bpf.c b/net/vmw_vsock/vsock_bpf.c
>> index f201d9eca1df2f8143638cf7a4d08671e8368c11..07b96d56f3a577af71021b1b8132743554996c4f 100644
>> --- a/net/vmw_vsock/vsock_bpf.c
>> +++ b/net/vmw_vsock/vsock_bpf.c
>> @@ -87,7 +87,7 @@ static int vsock_bpf_recvmsg(struct sock *sk, struct msghdr *msg,
>> 	lock_sock(sk);
>> 	vsk = vsock_sk(sk);
>>
>> -	if (!vsk->transport) {
>> +	if (WARN_ON_ONCE(!vsk->transport)) {
>> 		copied = -ENODEV;
>> 		goto out;
>> 	}
> 
> I'm not a sockmap expert, so I don't understand why here print an
> error.
> 
> Since there was already a check, I expected it to be a case that can 
> happen, but instead calling `rcvmsg()` on a socket not yet connected is 
> impossible?

That's right, calling vsock_bpf_recvmsg() on a not-yet-connected
connectible socket is impossible since PATCH 1/4 of this series.

That is because to reach vsock_bpf_recvmsg(), you must have sock's proto
replaced in vsock_bpf_update_proto(). For that you need to run
sock_map_init_proto(), which you can't because the patched
sock_map_sk_state_allowed() will stop you.

Re: [PATCH net 2/4] vsock/bpf: Warn on socket without transport

Posted by Stefano Garzarella 10 months ago

On Mon, Feb 17, 2025 at 08:45:06PM +0100, Michal Luczaj wrote:
>On 2/17/25 11:59, Stefano Garzarella wrote:
>> On Thu, Feb 13, 2025 at 12:58:50PM +0100, Michal Luczaj wrote:
>>> In the spirit of commit 91751e248256 ("vsock: prevent null-ptr-deref in
>>> vsock_*[has_data|has_space]"), armorize the "impossible" cases with a
>>> warning.
>>>
>>> Fixes: 634f1a7110b4 ("vsock: support sockmap")
>>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>>> ---
>>> net/vmw_vsock/af_vsock.c  | 3 +++
>>> net/vmw_vsock/vsock_bpf.c | 2 +-
>>> 2 files changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>>> index 53a081d49d28ac1c04e7f8057c8a55e7b73cc131..7e3db87ae4333cf63327ec105ca99253569bb9fe 100644
>>> --- a/net/vmw_vsock/af_vsock.c
>>> +++ b/net/vmw_vsock/af_vsock.c
>>> @@ -1189,6 +1189,9 @@ static int vsock_read_skb(struct sock *sk, skb_read_actor_t read_actor)
>>> {
>>> 	struct vsock_sock *vsk = vsock_sk(sk);
>>>
>>> +	if (WARN_ON_ONCE(!vsk->transport))
>>> +		return -ENODEV;
>>> +
>>> 	return vsk->transport->read_skb(vsk, read_actor);
>>> }
>>>
>>> diff --git a/net/vmw_vsock/vsock_bpf.c b/net/vmw_vsock/vsock_bpf.c
>>> index f201d9eca1df2f8143638cf7a4d08671e8368c11..07b96d56f3a577af71021b1b8132743554996c4f 100644
>>> --- a/net/vmw_vsock/vsock_bpf.c
>>> +++ b/net/vmw_vsock/vsock_bpf.c
>>> @@ -87,7 +87,7 @@ static int vsock_bpf_recvmsg(struct sock *sk, struct msghdr *msg,
>>> 	lock_sock(sk);
>>> 	vsk = vsock_sk(sk);
>>>
>>> -	if (!vsk->transport) {
>>> +	if (WARN_ON_ONCE(!vsk->transport)) {
>>> 		copied = -ENODEV;
>>> 		goto out;
>>> 	}
>>
>> I'm not a sockmap expert, so I don't understand why here print an
>> error.
>>
>> Since there was already a check, I expected it to be a case that can
>> happen, but instead calling `rcvmsg()` on a socket not yet connected is
>> impossible?
>
>That's right, calling vsock_bpf_recvmsg() on a not-yet-connected
>connectible socket is impossible since PATCH 1/4 of this series.

Okay, I get it now.

If you need to send a v2, can you write it in the commit description 
that the behavior changed in the previous patch and so in 
vsock_bpf_recvmsg() now we no longer expect to be called with a null 
transport.

In any case, LGTM:

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

>
>That is because to reach vsock_bpf_recvmsg(), you must have sock's proto
>replaced in vsock_bpf_update_proto(). For that you need to run
>sock_map_init_proto(), which you can't because the patched
>sock_map_sk_state_allowed() will stop you.
>