[PATCH net-next] net: fix token index wraparound in SO_DEVMEM_DONTNEED

[PATCH net-next] net: fix token index wraparound in SO_DEVMEM_DONTNEED

[Xia Yan]

SO_DEVMEM_DONTNEED computes each index as token_start + j, in 32-bit
unsigned arithmetic. A start of 0xfffffffe and a count of 4 produce
indices 0xfffffffe, 0xffffffff, 0 and 1. Once the sum wraps, the loop
can erase an existing low-numbered token from the same socket.

Skip an index when the sum wraps below token_start. The check runs
after the MAX_DONTNEED_FRAGS test; skipped indices still consume the
1024-item budget. Tokens allocated with xa_limit_31b are unaffected.

Found during a code review of the devmem token release path.

Built net/core/sock.o for i386 and x86_64 with W=1.

A temporary UML module tested copies of both loops against a real
XArray. All five cases passed. The wrapping case deleted one entry
with the original loop and none with the guard.

Fixes: 678f6e28b5f6 ("net: add SO_DEVMEM_DONTNEED setsockopt to release RX frags")
Signed-off-by: Xia Yan <yanxia@cmss.chinamobile.com>
---
 net/core/sock.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/core/sock.c b/net/core/sock.c
index e2401d7..90f59b9 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1115,6 +1115,8 @@ sock_devmem_dontneed(struct sock *sk, sockptr_t optval, unsigned int optlen)
 		for (j = 0; j < tokens[i].token_count; j++) {
 			if (++num_frags > MAX_DONTNEED_FRAGS)
 				goto frag_limit_reached;
+			if (tokens[i].token_start + j < tokens[i].token_start)
+				continue;
 
 			netmem_ref netmem = (__force netmem_ref)__xa_erase(
 				&sk->sk_user_frags, tokens[i].token_start + j);

Re: [PATCH net-next] net: fix token index wraparound in SO_DEVMEM_DONTNEED

[Stanislav Fomichev]

On 11/10, Xia Yan wrote:
> SO_DEVMEM_DONTNEED computes each index as token_start + j, in 32-bit
> unsigned arithmetic. A start of 0xfffffffe and a count of 4 produce
> indices 0xfffffffe, 0xffffffff, 0 and 1. Once the sum wraps, the loop
> can erase an existing low-numbered token from the same socket.

Sorry, I fail to understand what's the issue. This is (buggy) userspace
misusing the api and potentially removing different tokens, right?
So what are we fixing? Can you explain more?