| 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
| ||
|
Message-Id: <20221208000209.gonna.368-kees@kernel.org>
Date: Wed, 7 Dec 2022 16:02:13 -0800
From: Kees Cook <keescook@...omium.org>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Kees Cook <keescook@...omium.org>,
syzbot+fda18eaa8c12534ccb3b@...kaller.appspotmail.com,
Eric Dumazet <edumazet@...gle.com>,
"David S. Miller" <davem@...emloft.net>,
Paolo Abeni <pabeni@...hat.com>,
Pavel Begunkov <asml.silence@...il.com>,
pepsipu <soopthegoop@...il.com>,
Vlastimil Babka <vbabka@...e.cz>,
kasan-dev <kasan-dev@...glegroups.com>,
Andrii Nakryiko <andrii@...nel.org>, ast@...nel.org,
bpf <bpf@...r.kernel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Hao Luo <haoluo@...gle.com>,
Jesper Dangaard Brouer <hawk@...nel.org>,
John Fastabend <john.fastabend@...il.com>, jolsa@...nel.org,
KP Singh <kpsingh@...nel.org>, martin.lau@...ux.dev,
Stanislav Fomichev <sdf@...gle.com>, song@...nel.org,
Yonghong Song <yhs@...com>, netdev@...r.kernel.org,
LKML <linux-kernel@...r.kernel.org>,
Rasesh Mody <rmody@...vell.com>,
Ariel Elior <aelior@...vell.com>,
Manish Chopra <manishc@...vell.com>,
Menglong Dong <imagedong@...cent.com>,
David Ahern <dsahern@...nel.org>,
Richard Gobert <richardbgobert@...il.com>,
David Rientjes <rientjes@...gle.com>,
Andrey Konovalov <andreyknvl@...il.com>,
GR-Linux-NIC-Dev@...vell.com, linux-hardening@...r.kernel.org
Subject: [PATCH net-next v2] skbuff: Introduce slab_build_skb()
syzkaller reported:
BUG: KASAN: slab-out-of-bounds in __build_skb_around+0x235/0x340 net/core/skbuff.c:294
Write of size 32 at addr ffff88802aa172c0 by task syz-executor413/5295
For bpf_prog_test_run_skb(), which uses a kmalloc()ed buffer passed to
build_skb().
When build_skb() is passed a frag_size of 0, it means the buffer came
from kmalloc. In these cases, ksize() is used to find its actual size,
but since the allocation may not have been made to that size, actually
perform the krealloc() call so that all the associated buffer size
checking will be correctly notified. Split this logic out into a new
interface, slab_build_skb(), but leave the original 0 checking for now
to catch any stragglers.
Reported-by: syzbot+fda18eaa8c12534ccb3b@...kaller.appspotmail.com
Link: https://groups.google.com/g/syzkaller-bugs/c/UnIKxTtU5-0/m/-wbXinkgAQAJ
Fixes: 38931d8989b5 ("mm: Make ksize() a reporting-only function")
Cc: Jakub Kicinski <kuba@...nel.org>
Cc: Eric Dumazet <edumazet@...gle.com>
Cc: "David S. Miller" <davem@...emloft.net>
Cc: Paolo Abeni <pabeni@...hat.com>
Cc: Pavel Begunkov <asml.silence@...il.com>
Cc: pepsipu <soopthegoop@...il.com>
Cc: syzbot+fda18eaa8c12534ccb3b@...kaller.appspotmail.com
Cc: Vlastimil Babka <vbabka@...e.cz>
Cc: kasan-dev <kasan-dev@...glegroups.com>
Cc: Andrii Nakryiko <andrii@...nel.org>
Cc: ast@...nel.org
Cc: bpf <bpf@...r.kernel.org>
Cc: Daniel Borkmann <daniel@...earbox.net>
Cc: Hao Luo <haoluo@...gle.com>
Cc: Jesper Dangaard Brouer <hawk@...nel.org>
Cc: John Fastabend <john.fastabend@...il.com>
Cc: jolsa@...nel.org
Cc: KP Singh <kpsingh@...nel.org>
Cc: martin.lau@...ux.dev
Cc: Stanislav Fomichev <sdf@...gle.com>
Cc: song@...nel.org
Cc: Yonghong Song <yhs@...com>
Cc: netdev@...r.kernel.org
Cc: LKML <linux-kernel@...r.kernel.org>
Signed-off-by: Kees Cook <keescook@...omium.org>
---
Is this what you had in mind for this kind of change?
v2: introduce separate helper (kuba)
v1: https://lore.kernel.org/netdev/20221206231659.never.929-kees@kernel.org/
---
drivers/net/ethernet/broadcom/bnx2.c | 2 +-
drivers/net/ethernet/qlogic/qed/qed_ll2.c | 2 +-
include/linux/skbuff.h | 1 +
net/bpf/test_run.c | 2 +-
net/core/skbuff.c | 52 +++++++++++++++++++++--
5 files changed, 52 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnx2.c b/drivers/net/ethernet/broadcom/bnx2.c
index fec57f1982c8..b2230a4a2086 100644
--- a/drivers/net/ethernet/broadcom/bnx2.c
+++ b/drivers/net/ethernet/broadcom/bnx2.c
@@ -3045,7 +3045,7 @@ bnx2_rx_skb(struct bnx2 *bp, struct bnx2_rx_ring_info *rxr, u8 *data,
dma_unmap_single(&bp->pdev->dev, dma_addr, bp->rx_buf_use_size,
DMA_FROM_DEVICE);
- skb = build_skb(data, 0);
+ skb = slab_build_skb(data);
if (!skb) {
kfree(data);
goto error;
diff --git a/drivers/net/ethernet/qlogic/qed/qed_ll2.c b/drivers/net/ethernet/qlogic/qed/qed_ll2.c
index ed274f033626..e5116a86cfbc 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_ll2.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_ll2.c
@@ -200,7 +200,7 @@ static void qed_ll2b_complete_rx_packet(void *cxt,
dma_unmap_single(&cdev->pdev->dev, buffer->phys_addr,
cdev->ll2->rx_size, DMA_FROM_DEVICE);
- skb = build_skb(buffer->data, 0);
+ skb = slab_build_skb(buffer->data);
if (!skb) {
DP_INFO(cdev, "Failed to build SKB\n");
kfree(buffer->data);
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 7be5bb4c94b6..0b391b635430 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1253,6 +1253,7 @@ struct sk_buff *build_skb_around(struct sk_buff *skb,
void skb_attempt_defer_free(struct sk_buff *skb);
struct sk_buff *napi_build_skb(void *data, unsigned int frag_size);
+struct sk_buff *slab_build_skb(void *data);
/**
* alloc_skb - allocate a network buffer
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 13d578ce2a09..611b1f4082cf 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -1130,7 +1130,7 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
}
sock_init_data(NULL, sk);
- skb = build_skb(data, 0);
+ skb = slab_build_skb(data);
if (!skb) {
kfree(data);
kfree(ctx);
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 1d9719e72f9d..2bff6af6a777 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -269,12 +269,10 @@ static struct sk_buff *napi_skb_cache_get(void)
return skb;
}
-/* Caller must provide SKB that is memset cleared */
-static void __build_skb_around(struct sk_buff *skb, void *data,
- unsigned int frag_size)
+static inline void __finalize_skb_around(struct sk_buff *skb, void *data,
+ unsigned int size)
{
struct skb_shared_info *shinfo;
- unsigned int size = frag_size ? : ksize(data);
size -= SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
@@ -296,6 +294,52 @@ static void __build_skb_around(struct sk_buff *skb, void *data,
skb_set_kcov_handle(skb, kcov_common_handle());
}
+static inline void __slab_build_skb(struct sk_buff *skb, void *data,
+ unsigned int *size)
+{
+ void *resized;
+
+ *size = ksize(data);
+ /* krealloc() will immediate return "data" when
+ * "ksize(data)" is requested: it is the existing upper
+ * bounds. As a result, GFP_ATOMIC will be ignored.
+ */
+ resized = krealloc(data, *size, GFP_ATOMIC);
+ WARN_ON_ONCE(resized != data);
+}
+
+struct sk_buff *slab_build_skb(void *data)
+{
+ struct sk_buff *skb;
+ unsigned int size;
+
+ skb = kmem_cache_alloc(skbuff_head_cache, GFP_ATOMIC);
+ if (unlikely(!skb))
+ return NULL;
+
+ memset(skb, 0, offsetof(struct sk_buff, tail));
+ __slab_build_skb(skb, data, &size);
+ __finalize_skb_around(skb, data, size);
+
+ return skb;
+}
+EXPORT_SYMBOL(slab_build_skb);
+
+/* Caller must provide SKB that is memset cleared */
+static void __build_skb_around(struct sk_buff *skb, void *data,
+ unsigned int frag_size)
+{
+ unsigned int size = frag_size;
+
+ /* When frag_size == 0, the buffer came from kmalloc, so we
+ * must find its true allocation size (and grow it to match).
+ */
+ if (WARN_ONCE(size == 0, "Use slab_build_skb() instead"))
+ __slab_build_skb(skb, data, &size);
+
+ __finalize_skb_around(skb, data, size);
+}
+
/**
* __build_skb - build a network buffer
* @data: data buffer provided by caller
--
2.34.1
Powered by blists - more mailing lists