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 (and use the "new" pointer so that
compiler hinting works correctly). 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@syzkaller.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: Pavel Begunkov <asml.silence@gmail.com>
Cc: pepsipu <soopthegoop@gmail.com>
Cc: syzbot+fda18eaa8c12534ccb3b@syzkaller.appspotmail.com
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: kasan-dev <kasan-dev@googlegroups.com>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: ast@kernel.org
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Hao Luo <haoluo@google.com>
Cc: Jesper Dangaard Brouer <hawk@kernel.org>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: jolsa@kernel.org
Cc: KP Singh <kpsingh@kernel.org>
Cc: martin.lau@linux.dev
Cc: Stanislav Fomichev <sdf@google.com>
Cc: song@kernel.org
Cc: Yonghong Song <yhs@fb.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Link: https://lore.kernel.org/r/20221208060256.give.994-kees@kernel.org
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

diff --git a/drivers/net/ethernet/broadcom/bnx2.c b/drivers/net/ethernet/broadcom/bnx2.c
index dbe31014478044e0a68d27f4369d20aa316d12de..9f473854b0f446a2f855f840f24b6120086e2206 100644 (file)
--- a/drivers/net/ethernet/broadcom/bnx2.c
+++ b/drivers/net/ethernet/broadcom/bnx2.c
@@ -3045,7 +3045,7 @@ error:
 
        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 ed274f033626d9a0c24159976280b35c5cde9ae8..e5116a86cfbcbf2eae4fd045e94f447edadf28af 100644 (file)
--- 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 4e464a27adafbca281675c0a36ce6911cadcd6f1..4c8492401a101f1d6d43079fc70962210389763c 100644 (file)
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1255,6 +1255,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 6094ef7cffcd26bd6a06439f86785ff8613b305b..c9bfd263dcefd11e5b3db3cd9d3c6ae4a60da85e 100644 (file)
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -1128,7 +1128,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 4bf95e36ed162611e8245c64c6caeecf9f60ef5d..3cbba7099c0ff42076747bde3d41ff3326dc920a 100644 (file)
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -270,12 +270,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));
 
@@ -297,15 +295,71 @@ 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;
+
+       /* Must find the allocation size (and grow it to match). */
+       *size = ksize(data);
+       /* krealloc() will immediately return "data" when
+        * "ksize(data)" is requested: it is the existing upper
+        * bounds. As a result, GFP_ATOMIC will be ignored. Note
+        * that this "new" pointer needs to be passed back to the
+        * caller for use so the __alloc_size hinting will be
+        * tracked correctly.
+        */
+       resized = krealloc(data, *size, GFP_ATOMIC);
+       WARN_ON_ONCE(resized != data);
+       return resized;
+}
+
+/* build_skb() variant which can operate on slab buffers.
+ * Note that this should be used sparingly as slab buffers
+ * cannot be combined efficiently by GRO!
+ */
+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));
+       data = __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;
+
+       /* frag_size == 0 is considered deprecated now. Callers
+        * using slab buffer should use slab_build_skb() instead.
+        */
+       if (WARN_ONCE(size == 0, "Use slab_build_skb() instead"))
+               data = __slab_build_skb(skb, data, &size);
+
+       __finalize_skb_around(skb, data, size);
+}
+
 /**
  * __build_skb - build a network buffer
  * @data: data buffer provided by caller
- * @frag_size: size of data, or 0 if head was kmalloced
+ * @frag_size: size of data (must not be 0)
  *
  * Allocate a new &sk_buff. Caller provides space holding head and
- * skb_shared_info. @data must have been allocated by kmalloc() only if
- * @frag_size is 0, otherwise data should come from the page allocator
- *  or vmalloc()
+ * skb_shared_info. @data must have been allocated from the page
+ * allocator or vmalloc(). (A @frag_size of 0 to indicate a kmalloc()
+ * allocation is deprecated, and callers should use slab_build_skb()
+ * instead.)
  * The return is the new skb buffer.
  * On a failure the return is %NULL, and @data is not freed.
  * Notes :