[PATCH v2 net] vsock: fix buffer size clamping order

[Norbert Szetei]

Hi,

v2: fix indentation/whitespace issues in the previous patch.

-- >8 --
From f5d160167c862c7f2ad6e6a1d4181d01997b683a Mon Sep 17 00:00:00 2001
From: Norbert Szetei <norbert@xxxxxxxxxxxx>
Date: Mon, 6 Apr 2026 19:52:52 +0200
Subject: [PATCH] vsock: fix buffer size clamping order

In vsock_update_buffer_size(), the buffer size was being clamped to the
maximum first, and then to the minimum. If a user sets a minimum buffer
size larger than the maximum, the minimum check overrides the maximum
check, inverting the constraint.

This breaks the intended socket memory boundaries by allowing the
vsk->buffer_size to grow beyond the configured vsk->buffer_max_size.

Fix this by checking the minimum first, and then the maximum. This
ensures the buffer size never exceeds the buffer_max_size.

Signed-off-by: Norbert Szetei <norbert@xxxxxxxxxxxx>
---
 net/vmw_vsock/af_vsock.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index d912ed2f012a..08f4dfb9782c 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -1951,12 +1951,12 @@ static void vsock_update_buffer_size(struct vsock_sock *vsk,
 				     const struct vsock_transport *transport,
 				     u64 val)
 {
-	if (val > vsk->buffer_max_size)
-		val = vsk->buffer_max_size;
-
 	if (val < vsk->buffer_min_size)
 		val = vsk->buffer_min_size;
 
+	if (val > vsk->buffer_max_size)
+		val = vsk->buffer_max_size;
+
 	if (val != vsk->buffer_size &&
 	    transport && transport->notify_buffer_size)
 		transport->notify_buffer_size(vsk, &val);
-- 
2.53.0


> On Apr 6, 2026, at 20:41, Norbert Szetei <norbert@xxxxxxxxxxxx> wrote:
> 
> Hi Stefano,
> 
> I like option 1 the most, as it is the most straightforward way to fix
> the issue. I am including the patch below. This fixes the clamping
> mismatch, but as you pointed out, it won't solve the root problem
> regarding the issue to arbitrarily set a maximum buffer value.
> 
> Since VSOCK uses a unified buffer size rather than separating read and
> write buffers like the core stack, introducing a vsock-specific sysctl
> (e.g., net.vmw_vsock.buffer_max_size) seems the cleanest approach to me.
> I was also considering net.core.wmem_max/rmem_max, but mapping to those
> feels less natural.
> 
> If you agree with the vsock-specific sysctl, or have a different
> suggestion, let me know and I will send a follow-up patch for that.
> Thanks.
> 
> Best, Norbert
> 
> -- >8 --
> From f5d160167c862c7f2ad6e6a1d4181d01997b683a Mon Sep 17 00:00:00 2001
> From: Norbert Szetei <norbert@xxxxxxxxxxxx>
> Date: Mon, 6 Apr 2026 19:52:52 +0200
> Subject: [PATCH] vsock: fix buffer size clamping order
> 
> In vsock_update_buffer_size(), the buffer size was being clamped to the
> maximum first, and then to the minimum. If a user sets a minimum buffer
> size larger than the maximum, the minimum check overrides the maximum
> check, inverting the constraint.
> 
> This breaks the intended socket memory boundaries by allowing the
> vsk->buffer_size to grow beyond the configured vsk->buffer_max_size.
> 
> Fix this by checking the minimum first, and then the maximum. This
> ensures the buffer size never exceeds the buffer_max_size.
> 
> Signed-off-by: Norbert Szetei <norbert@xxxxxxxxxxxx>
> ---
> net/vmw_vsock/af_vsock.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index d912ed2f012a..08f4dfb9782c 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -1951,12 +1951,12 @@ static void vsock_update_buffer_size(struct vsock_sock *vsk,
>     const struct vsock_transport *transport,
>     u64 val)
> {
> - if (val > vsk->buffer_max_size)
> - val = vsk->buffer_max_size;
> -
>  if (val < vsk->buffer_min_size)
>  val = vsk->buffer_min_size;
> + if (val > vsk->buffer_max_size)
> + val = vsk->buffer_max_size;
> +
>  if (val != vsk->buffer_size &&
>    transport && transport->notify_buffer_size)
>  transport->notify_buffer_size(vsk, &val);
> -- 
> 2.53.0
> 
>> On Mar 31, 2026, at 14:37, Stefano Garzarella <sgarzare@xxxxxxxxxx> wrote:
>> 
>> On Tue, Mar 24, 2026 at 06:28:12PM +0100, Norbert Szetei wrote:
>>> Hello,
>>> 
>>> we have discovered a bug in AF_VSOCK where an unprivileged user can bypass socket
>>> memory constraints. This leads to refcount_t saturation and OOM. While
>>> refcount_t prevents a true UAF by saturating, the resulting state triggers
>>> kernel warnings and kernel panic, depending on the setup.
>>> 
>>> In vsock_connectible_setsockopt(), the SO_VM_SOCKETS_BUFFER_MIN_SIZE and
>>> SO_VM_SOCKETS_BUFFER_MAX_SIZE options are used to update the buffer's minimum
>>> and maximum values independently.
>>> 
>>> The vsock_update_buffer_size() function clamps the buffer size to the maximum
>>> first, then the minimum:
>>> 
>>> // https://github.com/torvalds/linux/blob/c369299895a591d96745d6492d4888259b004a9e/net/vmw_vsock/af_vsock.c#L1950
>>> if (val > vsk->buffer_max_size)
>>> val = vsk->buffer_max_size;
>>> 
>>> if (val < vsk->buffer_min_size)
>>> val = vsk->buffer_min_size;
>>> 
>>> vsk->buffer_size = val;
>>> 
>>> By setting buffer_min_size to a large value, the second clamp overrides the
>>> first, forcing vsk->buffer_size to exceed the intended maximum. The transport
>>> layer then uses this value, allowing unbounded SKB allocation that saturates the
>>> 32-bit sk_wmem_alloc refcount.
>>> 
>>> The fix should ensure that SO_VM_SOCKETS_BUFFER_MIN_SIZE cannot be used to set a
>>> value higher than the current buffer_max_size. Conversely,
>>> SO_VM_SOCKETS_BUFFER_MAX_SIZE should not be allowed to be set lower than the
>>> current buffer_min_size.
>> 
>> Okay, but that wouldn't change much. As long as the user sets the maximum to match the minimum you set in the POC, it behaves exactly the same way, right?
>> 
>> Maybe we should add a sysctl to set a global upper bound, but this is another problem, I agree that we should improve the kernel behavior around min/max.  I see 3 options:
>> 
>> 1. Just invert the checks, fist check for min, then for max.
>> 
>> 2. Simply adjust the min and max values so that they make sense. For example, if the minimum value being set is greater than the maximum, the kernel could adjust the maximum to the same value. However, this would not change the behavior of your POC.
>> 
>> 3. Force the minimum to be less than or equal to the maximum. This, however, would require a certain order when setting the minimum and maximum, especially relative to the default. For example, if you increase the minimum beyond the default maximum, you must adjust the maximum first; conversely, if you want to set the maximum below the default minimum, you must adjust the minimum first.
>> 
>> I'm more into 1 or 2. 3 IMO is too much.
>> 
>> Do you want to send a patch?
>> 
>> Thanks,
>> Stefano
> 
>