Re: [PATCH net] net: sock: prevent integer overflow in sock_reserve_memory()

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
--- Patch [1]: [PATCH net] net: sock: prevent integer overflow in sock_reserve_memory() ---
commit 16cb32b81c740c27028a52ef3c27a62623c808ad
Author: Xiang Mei (Microsoft) <xmei5@asu.edu>

net: sock: prevent integer overflow in sock_reserve_memory()

This commit attempts to prevent an integer overflow in sk_forward_alloc by
computing the new value in u64 and rejecting allocations that exceed
INT_MAX.

> diff --git a/net/core/sock.c b/net/core/sock.c
> index 8a59bfaa8096e..2ba95bead1aed 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -1042,6 +1042,9 @@ static int sock_reserve_memory(struct sock *sk, int bytes)
>  
>  	pages = sk_mem_pages(bytes);
>  
> +	if ((u64)sk->sk_forward_alloc + ((u64)pages << PAGE_SHIFT) > INT_MAX)
> +		return -EINVAL;
> +

[Severity: Medium]
Does casting sk_forward_alloc to u64 sign-extend negative values?

Since sk_forward_alloc is a signed int and can legitimately be negative
(e.g., due to over-reclaim), casting it to u64 when negative produces a
massive value (e.g., -4096 becomes 0xfffffffffffff000).

If a user requests a small reservation such that sk_forward_alloc +
(pages << PAGE_SHIFT) remains negative, the 64-bit sum remains a large
unsigned value greater than INT_MAX.

Does this falsely trigger the > INT_MAX check, rejecting valid reservations
with -EINVAL and causing a functional denial of service for the socket?


[Severity: High]
This is a pre-existing issue, but does checking sk_forward_alloc allow
a state-change (TOCTOU) bypass?

sk_forward_alloc dynamically decreases when packets are queued on the
socket, as memory is temporarily moved to sk_rmem_alloc or
sk_wmem_queued.

Could an attacker reserve memory near INT_MAX, queue packets to reduce
sk_forward_alloc, and then call SO_RESERVE_MEM again? The temporarily
reduced sk_forward_alloc allows the check to pass.

When the attacker reads or drops the packets, sk_mem_uncharge()
unconditionally adds the memory back, overflowing sk_forward_alloc past
INT_MAX into a negative value.

On socket destruction, this negative value bypasses sk_mem_reclaim(),
leaving a non-zero balance that triggers a WARN_ON_ONCE kernel panic in
inet_sock_destruct().

Should this check use the total tracked sk->sk_reserved_mem instead of
the fluctuating sk_forward_alloc?


[Severity: High]
This is a pre-existing issue, but does sk_forward_alloc modification in
this function race with UDP receive paths?

sock_reserve_memory() modifies sk_forward_alloc using the non-atomic
WRITE_ONCE() helper via sk_forward_alloc_add() under lock_sock().

For UDP sockets, receive-side modifications (like udp_rmem_release())
occur in softirq without holding lock_sock() (using
sk_receive_queue.lock).

Can an unprivileged user repeatedly call setsockopt(SO_RESERVE_MEM) while
receiving UDP packets, causing a read-modify-write data race?

Could these lost updates corrupt socket memory accounting, leading to memory
limit bypasses or triggering panics during socket destruction?

>  	/* pre-charge to memcg */
>  	charged = mem_cgroup_sk_charge(sk, pages,
>  				       GFP_KERNEL | __GFP_RETRY_MAYFAIL);
-- 
pw-bot: cr