netdev - [PATCH bpf-next] xsk: fix xsk_build_skb() error: 'skb' dereferencing possible ERR

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date: Thu, 10 Aug 2023 22:22:23 +0530
From: Tirthendu Sarkar <tirthendu.sarkar@...el.com>
To: bpf@...r.kernel.org
Cc: netdev@...r.kernel.org,
	bjorn@...nel.org,
	magnus.karlsson@...el.com,
	maciej.fijalkowski@...el.com,
	jonathan.lemon@...il.com,
	davem@...emloft.net,
	kuba@...nel.org,
	pabeni@...hat.com,
	ast@...nel.org,
	daniel@...earbox.net,
	martin.lau@...ux.dev,
	dan.carpenter@...aro.org
Subject: [PATCH bpf-next] xsk: fix xsk_build_skb() error: 'skb' dereferencing possible ERR_PTR()

xsk_build_skb_zerocopy() may return an error other than -EAGAIN and this
is received as skb and used later in xsk_set_destructor_arg() and
xsk_drop_skb() which must operate on a valid skb.

Add new parameter to xsk_build_skb_zerocopy() to explicitly return error
and invoke xsk_set_destructor_arg() and xsk_drop_skb() only for a valid
skb.

Signed-off-by: Tirthendu Sarkar <tirthendu.sarkar@...el.com>
Reported-by: kernel test robot <lkp@...el.com>
Reported-by: Dan Carpenter <dan.carpenter@...aro.org>
Closes: https://lore.kernel.org/r/202307210434.OjgqFcbB-lkp@intel.com/
Fixes: cf24f5a5feea ("xsk: add support for AF_XDP multi-buffer on Tx path")
---
 net/xdp/xsk.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index 47796a5a79b3..ff467ade2da6 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -572,22 +572,23 @@ static void xsk_drop_skb(struct sk_buff *skb)
 }
 
 static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,
-					      struct xdp_desc *desc)
+					      struct xdp_desc *desc,
+					      int *err)
 {
 	struct xsk_buff_pool *pool = xs->pool;
 	u32 hr, len, ts, offset, copy, copied;
 	struct sk_buff *skb = xs->skb;
 	struct page *page;
 	void *buffer;
-	int err, i;
 	u64 addr;
+	int i;
 
 	if (!skb) {
 		hr = max(NET_SKB_PAD, L1_CACHE_ALIGN(xs->dev->needed_headroom));
 
-		skb = sock_alloc_send_skb(&xs->sk, hr, 1, &err);
+		skb = sock_alloc_send_skb(&xs->sk, hr, 1, err);
 		if (unlikely(!skb))
-			return ERR_PTR(err);
+			goto ret_err;
 
 		skb_reserve(skb, hr);
 	}
@@ -601,8 +602,10 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,
 	addr = buffer - pool->addrs;
 
 	for (copied = 0, i = skb_shinfo(skb)->nr_frags; copied < len; i++) {
-		if (unlikely(i >= MAX_SKB_FRAGS))
-			return ERR_PTR(-EFAULT);
+		if (unlikely(i >= MAX_SKB_FRAGS)) {
+			*err = -EFAULT;
+			goto ret_err;
+		}
 
 		page = pool->umem->pgs[addr >> PAGE_SHIFT];
 		get_page(page);
@@ -620,7 +623,8 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,
 	skb->truesize += ts;
 
 	refcount_add(ts, &xs->sk.sk_wmem_alloc);
-
+	*err = 0;
+ret_err:
 	return skb;
 }
 
@@ -632,11 +636,9 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
 	int err;
 
 	if (dev->priv_flags & IFF_TX_SKB_NO_LINEAR) {
-		skb = xsk_build_skb_zerocopy(xs, desc);
-		if (IS_ERR(skb)) {
-			err = PTR_ERR(skb);
+		skb = xsk_build_skb_zerocopy(xs, desc, &err);
+		if (err)
 			goto free_err;
-		}
 	} else {
 		u32 hr, tr, len;
 		void *buffer;
@@ -692,7 +694,7 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
 free_err:
 	if (err == -EAGAIN) {
 		xsk_cq_cancel_locked(xs, 1);
-	} else {
+	} else if (skb) {
 		xsk_set_destructor_arg(skb);
 		xsk_drop_skb(skb);
 		xskq_cons_release(xs->tx);
-- 
2.34.1