[PATCH net] net: skbuff: uncharge trimmed zerocopy frags on error

[PATCH net] net: skbuff: uncharge trimmed zerocopy frags on error

[Willem de Bruijn]

From: Chris Mason <mason@kernel.org>

__zerocopy_sg_from_iter() accounts for the frags that it adds even when
it returns an error: it adds the growth of skb->truesize to
sk_wmem_queued and, unless the skb is pure zerocopy, charges it to the
socket.

On error, skb_zerocopy_iter_stream() cannot free the skb, as it may hold
data from an earlier send. It trims the new frags, but leaves
skb->truesize and their accounting in place.

Repeated failed sends to the same skb accumulate this stale accounting.
This can inflate TCP memory accounting past tcp_mem, and can wrap
skb->truesize, leading to a use-after-free of the socket.

Undo the accounting after the trim. If the trim fails, the frags and
their accounting remain. The trim only fails if the skb is cloned,
which is not expected for the unsent skb at the TCP write queue tail.

The issue was found by an LLM.

Fixes: 52267790ef52 ("sock: add MSG_ZEROCOPY")
Reported-by: Kyle Zeng <kylebot@openai.com>
Signed-off-by: Chris Mason <mason@kernel.org>
[willemb: revised commit message]
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
Note for stable backports: before v5.16 (9b65b17db723, "net: avoid double
accounting for pure zerocopy skbs"), __zerocopy_sg_from_iter() calls
sk_mem_charge() unconditionally, so drop the skb_zcopy_pure() test and
always call sk_mem_uncharge().
---
 net/core/skbuff.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 8da53d5b76fc..60aa4e1609bf 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1908,6 +1908,7 @@ int skb_zerocopy_iter_stream(struct sock *sk, struct sk_buff *skb,
 			     struct ubuf_info *uarg,
 			     struct net_devmem_dmabuf_binding *binding)
 {
+	unsigned int orig_truesize = skb->truesize;
 	int err, orig_len = skb->len;
 
 	if (uarg->ops->link_skb) {
@@ -1928,12 +1929,18 @@ int skb_zerocopy_iter_stream(struct sock *sk, struct sk_buff *skb,
 	err = __zerocopy_sg_from_iter(msg, sk, skb, &msg->msg_iter, len,
 				      binding);
 	if (err == -EFAULT || (err == -EMSGSIZE && skb->len == orig_len)) {
+		u32 delta_truesize = skb->truesize - orig_truesize;
 		struct sock *save_sk = skb->sk;
 
 		/* Streams do not free skb on error. Reset to prev state. */
 		iov_iter_revert(&msg->msg_iter, skb->len - orig_len);
 		skb->sk = sk;
-		___pskb_trim(skb, orig_len);
+		if (!___pskb_trim(skb, orig_len)) {
+			skb->truesize -= delta_truesize;
+			sk_wmem_queued_add(sk, -delta_truesize);
+			if (!skb_zcopy_pure(skb))
+				sk_mem_uncharge(sk, delta_truesize);
+		}
 		skb->sk = save_sk;
 		return err;
 	}
-- 
2.56.0.455.g6c5a503b38-goog

Re: [PATCH net] net: skbuff: uncharge trimmed zerocopy frags on error

[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.

Re: [PATCH net] net: skbuff: uncharge trimmed zerocopy frags on error

[Willem de Bruijn]

netdev-bot+sinfo@ wrote:
> 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.

I thought "The issue was found by an LLM." in the commit message would be
sufficient.

This was found through analysis. A repro can be built, but its output
only pollutes the commit message imho.

> 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.