Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 8 Dec 2022 18:19:50 +0800
From:   Feng Tang <feng.tang@intel.com>
To:     Vlastimil Babka <vbabka@suse.cz>
CC:     Kees Cook <keescook@chromium.org>,
        Jakub Kicinski <kuba@kernel.org>,
        <syzbot+fda18eaa8c12534ccb3b@syzkaller.appspotmail.com>,
        Eric Dumazet <edumazet@google.com>,
        "David S. Miller" <davem@davemloft.net>,
        Paolo Abeni <pabeni@redhat.com>,
        Pavel Begunkov <asml.silence@gmail.com>,
        pepsipu <soopthegoop@gmail.com>,
        kasan-dev <kasan-dev@googlegroups.com>,
        "Andrii Nakryiko" <andrii@kernel.org>, <ast@kernel.org>,
        bpf <bpf@vger.kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Hao Luo <haoluo@google.com>,
        "Jesper Dangaard Brouer" <hawk@kernel.org>,
        John Fastabend <john.fastabend@gmail.com>, <jolsa@kernel.org>,
        KP Singh <kpsingh@kernel.org>, <martin.lau@linux.dev>,
        Stanislav Fomichev <sdf@google.com>, <song@kernel.org>,
        Yonghong Song <yhs@fb.com>, <netdev@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Rasesh Mody <rmody@marvell.com>,
        Ariel Elior <aelior@marvell.com>,
        Manish Chopra <manishc@marvell.com>,
        Menglong Dong <imagedong@tencent.com>,
        David Ahern <dsahern@kernel.org>,
        Richard Gobert <richardbgobert@gmail.com>,
        "Andrey Konovalov" <andreyknvl@gmail.com>,
        David Rientjes <rientjes@google.com>,
        <GR-Linux-NIC-Dev@marvell.com>, <linux-hardening@vger.kernel.org>
Subject: Re: [PATCH net-next v3] skbuff: Introduce slab_build_skb()
Message-ID: <Y5G6RnoyZC78UO4q@feng-clx>
References: <20221208060256.give.994-kees@kernel.org>
 <6923d6a9-7728-fc71-f963-3617e5361732@suse.cz>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <6923d6a9-7728-fc71-f963-3617e5361732@suse.cz>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?w4f5NMh0OpjLZnN+CNxRmF+wK2OOm7cf1caArr8af5lYbxdJxyopqtBO/agK?=
 =?us-ascii?Q?BxKWcYuMcVhIee0wOcgyZm6KIpJJgekJPOcJLG9ZfVHDdTiIIVWULlR/Cdui?=
 =?us-ascii?Q?br2GTyAjy70yGoy+q5L9xRjO+mbDzgdMevo5scmeTVNSBmVC6teKnbvtY/jH?=
 =?us-ascii?Q?X+e1WUIqPeCotyvHcIl8oUSPsRn04WAST0lRMD/RDiFSBXWohbT0Xp7axWIS?=
 =?us-ascii?Q?hkHGZQWFlWdUDHUstfx45c2PwAYF/dcCEGStgwRv/4+3po1N5EbMH2ASx8Wt?=
 =?us-ascii?Q?vciySkp3lIAgHihooZ9qbfyPHj/nS73wn3skDw4u2ugq4/0V+1zJ9Ah9U4ht?=
 =?us-ascii?Q?3kktVZXTedQfmlFi64e+OJePcNEm3tMmmFTUqbid80+q3f6HbkTGr8DVY4F+?=
 =?us-ascii?Q?9yXdl6E7SBNKx+Fgp5sxTfO4MQ5hyxjLmeKIt+DLZgr9kdxTm125+e7skBc6?=
 =?us-ascii?Q?SbVViJWgO18mNiCFuMQTHFVSdRAQJspL4aIUGphPmCx5Pt5wrcLXarSAql2y?=
 =?us-ascii?Q?EWz6TEcTL+y7W8t+rG+EltbTpeTGPLQ9xzZo2jJgkgJ5KSP0SfoMCuNdGd3Z?=
 =?us-ascii?Q?pmC7620hsZWV3n9NJRjvgFiKqAI8L8dRKRmwfuOVcUx6Uh79e8VUnmiZYI1R?=
 =?us-ascii?Q?WdoDUdWLIuVHZO3tHp8LQqKcWETepTbV0sp1OKUXygAYOcTX3XfP4IuKmdXx?=
 =?us-ascii?Q?g+4MdUR7praMERKrlcWYCPI7Dny/Kp3+ZrHTyol4Q5ZSNTSKFSjManzr3q10?=
 =?us-ascii?Q?IWdXDLEWI2HNY9EB6GLyKDAYZ30qK8A/O9airI6eRpc+D8w6rk33O+tx5ntq?=
 =?us-ascii?Q?YLYqSZFlHjJTgH9GRgVF5ba8nwEfn0Fj1CWNsjHuPiy89cKm9BnbwxqsTc0b?=
 =?us-ascii?Q?LIbNR3bUK/h4nR0UhXFIg5XQ+YIZCp3ZKsbM8tmw0msJjVdRwEx9ROPlBL6m?=
 =?us-ascii?Q?imlPTn15X0lP3E2p/iWVLDjlLb1Rof0aTDqy5/EkFWj0jypZsKk6jGbjAiZa?=
 =?us-ascii?Q?Wpb2lPOEXoRDJiKccjG7+b0V7D/rAkSp/wgHpjjiBx8lBnbt44eGs15RpzWN?=
 =?us-ascii?Q?p0n+Ba5JHteL6KHfpQ+XRtaxRQ/qxMiB6cXW4OK5NwCzwrePHSs3trH+WrOr?=
 =?us-ascii?Q?h1JKdaP2Ee5WPJ469P3nwcuwYB1zuQfXpo837It/85cqoTH0D8ln1oeZRWkC?=
 =?us-ascii?Q?SlbnAyyrGjFFzKDhMv8hQJD0eZBFIF0/5V0QDkFfVas8K8jY8XxLudqQQ4TL?=
 =?us-ascii?Q?pG2kIMBM/8qRg8FytSdMGQWrGYjUd0GEfeYqqu+dj16z7sClYvWjMC8T9bM+?=
 =?us-ascii?Q?aEwyI8TFbmt+X2UqeZbO3tP93+wRxxkrWgsA+4/4YkUbnCvOcxBkt3pGfH18?=
 =?us-ascii?Q?GY7S+ovuvDMM0BvT0QdHjVGDWqGhsFskNP93Ce3jq088Dv9+5puzxvh1JGn4?=
 =?us-ascii?Q?KeBWX/QKH6/AH4isDQPF/XEiu20kzcBX0p1yNmcaPDyizzujRiSHuxoqjSij?=
 =?us-ascii?Q?Fp9zOdPpO1Ruev54d938MWTcW7wpxd5UJSbCASquFKkzpkVKtZ0jNbFs6Hz4?=
 =?us-ascii?Q?OTwUyMp+6u3GXMRnglHZXNTux7dcufz/MAGTH8rJ?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 057287f2-e0d3-4bed-7a2e-08dad9062d67
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6304.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Dec 2022 10:22:58.3633
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: U3p3sCT6jbHx2oS75b9WuLiMglR9DY7Wo++v0iSvpCbc1P37hNtYfQyfTpDEyRxsirLBmXpsoirkzbVZ9h+0Eg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR11MB4630
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-7.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_HI,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Dec 08, 2022 at 09:13:41AM +0100, Vlastimil Babka wrote:
> On 12/8/22 07:02, Kees Cook wrote:
> > 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: Jakub Kicinski <kuba@kernel.org>
> > Cc: Eric Dumazet <edumazet@google.com>
> > Cc: "David S. Miller" <davem@davemloft.net>
> > Cc: Paolo Abeni <pabeni@redhat.com>
> > 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: bpf <bpf@vger.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>
> > Cc: netdev@vger.kernel.org
> > Cc: LKML <linux-kernel@vger.kernel.org>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> > v3:
> > - make sure "resized" is passed back so compiler hints survive
> > - update kerndoc (kuba)
> > v2: https://lore.kernel.org/lkml/20221208000209.gonna.368-kees@kernel.org
> > 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                         | 70 ++++++++++++++++++++---
> >  5 files changed, 66 insertions(+), 11 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..ae5a6f7db37b 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,15 +294,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);
> 
> Hmm, I just realized, this trick will probably break the new kmalloc size
> tracking from Feng Tang (CC'd)? We need to make krealloc() update the stored
> size, right? And even worse if slab_debug redzoning is enabled and after
> commit 946fa0dbf2d8 ("mm/slub: extend redzone check to extra allocated
> kmalloc space than requested") where the lack of update will result in
> redzone check failures.

I think it's still safe, as currently we skip the kmalloc redzone check
by calling skip_orig_size_check() inside __ksize(). But as we have plan
to remove this skip_orig_size_check() after all ksize() usage has been
sanitized, we need to cover this krealloc() case.

Thanks,
Feng