[PATCH net] inet: frags: bound the wmem charge of the socket completing a reassembly

[PATCH net] inet: frags: bound the wmem charge of the socket completing a reassembly

[Eric Dumazet]

When a socket owned fragment completes a reassembly queue,
inet_frag_reasm_prepare() and inet_frag_reasm_finish() charge the
truesize of all other fragments to its sk_wmem_alloc. These fragments
were orphaned when queued, and might have been sent by someone else,
for instance padded TUN packets.

Commit 3ae20e4697eb ("inet: frags: limit the truesize of a reassembly
queue") bounds this charge to INT_MAX / 2 per queue, but a socket can
complete several queues while the reassembled skbs are still alive,
for instance held in a qdisc. sk_wmem_alloc can then go past INT_MAX,
refcount_add() saturates it with a WARN, and the socket is leaked.

This is reachable from an unprivileged user/network namespace, by
raising ipfrag_high_thresh.

Refuse the reassembly if the charge would push sk_wmem_alloc above
2 * sk_sndbuf, the bound ip_append_data() uses, plus 4MB of slack for
fragments inflated by a refragmentation with a small MTU.
SO_SNDBUFFORCE requires CAP_NET_ADMIN in the init user namespace,
so an unprivileged user can not raise sk_sndbuf above 2 * wmem_max.

The check is done before inet_frag_reasm_prepare() modifies the queue.
All callers already kill the queue when it returns NULL.

Fixes: 18685451fc4e ("inet: inet_defrag: prevent sk release while still in use")
Reported-by: Paolo Abeni <pabeni@redhat.com>
Closes: https://lore.kernel.org/netdev/24feda7d-ae49-45af-9596-2cc5f149f28e@redhat.com/
Cc: Florian Westphal <fw@strlen.de>
Signed-off-by: Eric Dumazet <edumazet@kernel.org>
---
 net/ipv4/inet_fragment.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
index 0b8d22ffafca69b2bccd98eca3d4bda93ebb816a..c11d7e976dfa534b773f99f33f47fcc562ed569b 100644
--- a/net/ipv4/inet_fragment.c
+++ b/net/ipv4/inet_fragment.c
@@ -17,6 +17,7 @@
 #include <linux/rtnetlink.h>
 #include <linux/slab.h>
 #include <linux/rhashtable.h>
+#include <linux/sizes.h>
 
 #include <net/sock.h>
 #include <net/inet_frag.h>
@@ -519,6 +520,25 @@ int inet_frag_queue_insert(struct inet_frag_queue *q, struct sk_buff *skb,
 }
 EXPORT_SYMBOL(inet_frag_queue_insert);
 
+/* When a socket owned fragment completes a queue, the truesize of all
+ * other fragments is charged to its sk_wmem_alloc, although they were
+ * orphaned when queued and possibly sent by someone else.
+ * Bound this like ip_append_data() bounds the memory a socket can
+ * allocate (2 * sk_sndbuf), plus slack for fragments inflated by a
+ * refragmentation using a small MTU. Otherwise, several reassembled skbs
+ * alive at the same time can saturate sk_wmem_alloc, and leak the socket.
+ */
+#define INET_FRAG_WMEM_SLACK	SZ_4M
+
+static bool inet_frag_wmem_ok(const struct inet_frag_queue *q,
+			      const struct sock *sk)
+{
+	u64 limit = 2ULL * READ_ONCE(sk->sk_sndbuf) + INET_FRAG_WMEM_SLACK;
+	u64 wmem = (u64)refcount_read(&sk->sk_wmem_alloc) + q->truesize;
+
+	return wmem <= min_t(u64, limit, INET_FRAG_MAX_TRUESIZE);
+}
+
 void *inet_frag_reasm_prepare(struct inet_frag_queue *q, struct sk_buff *skb,
 			      struct sk_buff *parent)
 {
@@ -530,6 +550,8 @@ void *inet_frag_reasm_prepare(struct inet_frag_queue *q, struct sk_buff *skb,
 	int delta;
 
 	if (sk && is_skb_wmem(skb)) {
+		if (!inet_frag_wmem_ok(q, sk))
+			return NULL;
 		/* TX: skb->sk might have been passed as argument to
 		 * dst->output and must remain valid until tx completes.
 		 *
-- 
2.53.0

Re: [PATCH net] inet: frags: bound the wmem charge of the socket completing a reassembly

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.