Message ID | 20240304032203.3480001-1-lizetao1@huawei.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-90008-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7301:fa17:b0:10a:f01:a869 with SMTP id ju23csp1195979dyc; Sun, 3 Mar 2024 19:22:57 -0800 (PST) X-Forwarded-Encrypted: i=3; AJvYcCUncjbW9o8Jge1FqviyWqmgSCY/auvonuDj6Ft5DBSCMjM0UcBbzToCdeZ990njTMO6rC+w+7WuOf2KQpqPTzAPykPaSQ== X-Google-Smtp-Source: AGHT+IFsWKjTWtnmqR5wwcZiBf/mwlq/0FsFGvYUqsKDjonu5nveeYDgWBPvDd6gvZa2dbRGiUxq X-Received: by 2002:a17:906:7d6:b0:a43:88c0:7729 with SMTP id m22-20020a17090607d600b00a4388c07729mr5386005ejc.56.1709522576854; Sun, 03 Mar 2024 19:22:56 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1709522576; cv=pass; d=google.com; s=arc-20160816; b=w8/iQmFMIjvoBJSQ+XETUIE3wtAiWU9Q/Co82UiPL46o1FrE/OvgQaIlyZPH0G5hMj uvmwQK8V0ZW1fEpATysNP/g1HFGxQXHFVdQk1NQRicVs0ffAt/EspqRzzld6PZ9MBPlQ dnqBlUvkZ5bdT+2POQ8spP07brdn94ZpslavYlLmK4eBKiyN4VXN3EE9tzdCmh+pmdjV CVTga+FL5VKOF6M79Om8MB10xmGnYcRFn1l6k+2OqlHbMTWJKNL0l5+jzYtU4rRf1S/o 8TNoE9/f3n8UhoVMP07rV/QE8cgstMyOfCq2QzTrRINO6kmYLhEWQcqeaobCmPt5fTmu l1+A== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:message-id:date:subject:cc:to :from; bh=jxmt+Kc2v6qV/L6QxjdTZYCYf9lVHm+1NQ1vS7oFQKw=; fh=q9t7aa7z/wDgd9X0fMDtFVCiZWFUxMKnsUqB2uZsFt8=; b=QMczBWPTDBGYiWXVlqvUovFauwv9BPFRmYHbyCtk0K/dI9LKll9KHJYQtJINi4Ri+9 HGMJxeVqCnNpWyFUWytQxUJFqs15x8QU+KaWNK6XzLSv75bmvt3PQTU0QkaZg6bIV4Tr JNzMoXG1kgLvw3seZ+ZiKBj8TDt5bya50CWC50PcYDRFHsFZpxJuqV0YkD0IKiyK44ql UbZsGe+8pS7+32Ke8W4bY7VAULj3S7DwxYqjfZwWZOfaumRIw/me1vlgaiBHUeqtdoSm vOvnfMnn3AS1pX9qfyd1YIyOawq/IQNL57EbpRCquD8jgrmkPOPmR+DM7Npt8gMWkgfN Czbw==; dara=google.com ARC-Authentication-Results: i=2; mx.google.com; arc=pass (i=1 spf=pass spfdomain=huawei.com dmarc=pass fromdomain=huawei.com); spf=pass (google.com: domain of linux-kernel+bounces-90008-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) smtp.mailfrom="linux-kernel+bounces-90008-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=huawei.com Received: from am.mirrors.kernel.org (am.mirrors.kernel.org. [2604:1380:4601:e00::3]) by mx.google.com with ESMTPS id c13-20020a170906528d00b00a3e3965250dsi3579393ejm.883.2024.03.03.19.22.56 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 03 Mar 2024 19:22:56 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-90008-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3; Authentication-Results: mx.google.com; arc=pass (i=1 spf=pass spfdomain=huawei.com dmarc=pass fromdomain=huawei.com); spf=pass (google.com: domain of linux-kernel+bounces-90008-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) smtp.mailfrom="linux-kernel+bounces-90008-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=huawei.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by am.mirrors.kernel.org (Postfix) with ESMTPS id 7A0E51F2140D for <ouuuleilei@gmail.com>; Mon, 4 Mar 2024 03:22:56 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 157D753A6; Mon, 4 Mar 2024 03:22:45 +0000 (UTC) Received: from szxga05-in.huawei.com (szxga05-in.huawei.com [45.249.212.191]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 8F583211C; Mon, 4 Mar 2024 03:22:41 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=45.249.212.191 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709522563; cv=none; b=f2hna1515vWoGVh0XBoNN43VsliJ0rEhPPW/biBEaVMvpQ7uavCZbOA4mf7/97hGy1kVzXP8Q+Ru4SLf8oTmq8VHndfQNysTvt5ZifYz8FwfLP2OUKQwkPcPgvt7srXzjjDInFF4ip/AaCrExticNpfLaW+AgPQYfy/t9nno9n4= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709522563; c=relaxed/simple; bh=veLqZ5GaMtlPnNbR+KmO08huNmMlZ5vta4bUNMiDihc=; h=From:To:CC:Subject:Date:Message-ID:MIME-Version:Content-Type; b=I4hfPovIpD792U5GIo8GYGfMVhOiIT5vmWTMDTTUSL8Df69gHCnMETIBPS2RXT86yZoGEnWmA1uTh07he6UkD1LBL4w/89MIl3wGXGlLpatD6oJT9+CxaYtYwnOoayaMVt5chOQtnzgEK8nmmdQXQa1Ky+RNbiXv0hh0baeQ7Ps= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=huawei.com; spf=pass smtp.mailfrom=huawei.com; arc=none smtp.client-ip=45.249.212.191 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=huawei.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=huawei.com Received: from mail.maildlp.com (unknown [172.19.88.214]) by szxga05-in.huawei.com (SkyGuard) with ESMTP id 4Tp3sZ4GfTz1FLcr; Mon, 4 Mar 2024 11:22:26 +0800 (CST) Received: from kwepemd500004.china.huawei.com (unknown [7.221.188.173]) by mail.maildlp.com (Postfix) with ESMTPS id 34A571A016B; Mon, 4 Mar 2024 11:22:32 +0800 (CST) Received: from kwepemd500012.china.huawei.com (7.221.188.25) by kwepemd500004.china.huawei.com (7.221.188.173) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1258.28; Mon, 4 Mar 2024 11:22:31 +0800 Received: from huawei.com (10.90.53.73) by kwepemd500012.china.huawei.com (7.221.188.25) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1258.28; Mon, 4 Mar 2024 11:22:31 +0800 From: Li Zetao <lizetao1@huawei.com> To: <kent.overstreet@linux.dev>, <bfoster@redhat.com> CC: <lizetao1@huawei.com>, <linux-bcachefs@vger.kernel.org>, <linux-kernel@vger.kernel.org> Subject: [PATCH] bcachefs: Fix null-ptr-deref in bch2_fs_alloc() Date: Mon, 4 Mar 2024 11:22:03 +0800 Message-ID: <20240304032203.3480001-1-lizetao1@huawei.com> X-Mailer: git-send-email 2.34.1 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-ClientProxiedBy: dggems701-chm.china.huawei.com (10.3.19.178) To kwepemd500012.china.huawei.com (7.221.188.25) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1792564345557492848 X-GMAIL-MSGID: 1792564345557492848 |
Series |
bcachefs: Fix null-ptr-deref in bch2_fs_alloc()
|
|
Commit Message
Li Zetao
March 4, 2024, 3:22 a.m. UTC
There is a null-ptr-deref issue reported by kasan:
KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
Call Trace:
<TASK>
bch2_fs_alloc+0x1092/0x2170 [bcachefs]
bch2_fs_open+0x683/0xe10 [bcachefs]
...
When initializing the name of bch_fs, it needs to dynamically alloc memory
to meet the length of the name. However, when name allocation failed, it
will cause a null-ptr-deref access exception in subsequent string copy.
Fix this issue by checking if name allocation is successful.
Fixes: 401ec4db6308 ("bcachefs: Printbuf rework")
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
fs/bcachefs/super.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
Comments
On Mon 04 Mar 2024 at 11:22, Li Zetao <lizetao1@huawei.com> wrote: > There is a null-ptr-deref issue reported by kasan: > > KASAN: null-ptr-deref in range > [0x0000000000000000-0x0000000000000007] > Call Trace: > <TASK> > bch2_fs_alloc+0x1092/0x2170 [bcachefs] > bch2_fs_open+0x683/0xe10 [bcachefs] > ... > > When initializing the name of bch_fs, it needs to dynamically > alloc memory > to meet the length of the name. However, when name allocation > failed, it > will cause a null-ptr-deref access exception in subsequent > string copy. > bch2_printbuf_make_room() does return -ENOMEM but bch2_prt_printf() doesn't check the return code. And there are too many callers of bch2_prt_printf() don't check allocation_failure. > Fix this issue by checking if name allocation is successful. > > Fixes: 401ec4db6308 ("bcachefs: Printbuf rework") > Signed-off-by: Li Zetao <lizetao1@huawei.com> > --- > fs/bcachefs/super.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/fs/bcachefs/super.c b/fs/bcachefs/super.c > index 6b23e11825e6..24fa41bbe7e3 100644 > --- a/fs/bcachefs/super.c > +++ b/fs/bcachefs/super.c > @@ -818,13 +818,13 @@ static struct bch_fs *bch2_fs_alloc(struct > bch_sb *sb, struct bch_opts opts) > goto err; > > pr_uuid(&name, c->sb.user_uuid.b); > - strscpy(c->name, name.buf, sizeof(c->name)); > - printbuf_exit(&name); > - > ret = name.allocation_failure ? -BCH_ERR_ENOMEM_fs_name_alloc > : 0; > if (ret) > goto err; > IIRC, krealloc() doesn't free old pointer if new-size allocation failed. There is no printbuf_exit called in label err then memory leak happens. -- Su > > + strscpy(c->name, name.buf, sizeof(c->name)); > + printbuf_exit(&name); > + > /* Compat: */ > if (le16_to_cpu(sb->version) <= > bcachefs_metadata_version_inode_v2 && > !BCH_SB_JOURNAL_FLUSH_DELAY(sb))
On 2024/3/4 13:12, Su Yue wrote: > > On Mon 04 Mar 2024 at 11:22, Li Zetao <lizetao1@huawei.com> wrote: > >> There is a null-ptr-deref issue reported by kasan: >> >> KASAN: null-ptr-deref in range >> [0x0000000000000000-0x0000000000000007] >> Call Trace: >> <TASK> >> bch2_fs_alloc+0x1092/0x2170 [bcachefs] >> bch2_fs_open+0x683/0xe10 [bcachefs] >> ... >> >> When initializing the name of bch_fs, it needs to dynamically alloc >> memory >> to meet the length of the name. However, when name allocation failed, it >> will cause a null-ptr-deref access exception in subsequent string copy. >> > bch2_printbuf_make_room() does return -ENOMEM but > bch2_prt_printf() doesn't check the return code. And there are too many > callers of bch2_prt_printf() don't check allocation_failure. Indeed, too many callers do not check whether name allocation is successful, which may cause hidden dangers. Maybe it is neccssary to use somethings like __GFP_NOFAIL flag here? > >> Fix this issue by checking if name allocation is successful. >> >> Fixes: 401ec4db6308 ("bcachefs: Printbuf rework") >> Signed-off-by: Li Zetao <lizetao1@huawei.com> >> --- >> fs/bcachefs/super.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/fs/bcachefs/super.c b/fs/bcachefs/super.c >> index 6b23e11825e6..24fa41bbe7e3 100644 >> --- a/fs/bcachefs/super.c >> +++ b/fs/bcachefs/super.c >> @@ -818,13 +818,13 @@ static struct bch_fs *bch2_fs_alloc(struct >> bch_sb *sb, struct bch_opts opts) >> goto err; >> >> pr_uuid(&name, c->sb.user_uuid.b); >> - strscpy(c->name, name.buf, sizeof(c->name)); >> - printbuf_exit(&name); >> - >> ret = name.allocation_failure ? -BCH_ERR_ENOMEM_fs_name_alloc : 0; >> if (ret) >> goto err; >> > IIRC, krealloc() doesn't free old pointer if new-size allocation failed. > There is no printbuf_exit called in label err then memory leak happens. > Here krealloc() is a bit complicated: 1.if name allocation failure happens on the first time, the old pointer will be NULL, which cause a null-ptr-deref issue. 2.if name allocation failure don't happens on the first time, the old pointer will be available and need to free. So the correct modification should be something like this: pr_uuid(&name, c->sb.user_uuid.b); if (unlikely(!name.buf)) { ret = -BCH_ERR_ENOMEM_fs_name_alloc; goto err; } strscpy(c->name, name.buf, sizeof(c->name)); printbuf_exit(&name); ret = name.allocation_failure ? -BCH_ERR_ENOMEM_fs_name_alloc : 0; if (ret) goto err; > -- > Su >> >> + strscpy(c->name, name.buf, sizeof(c->name)); >> + printbuf_exit(&name); >> + >> /* Compat: */ >> if (le16_to_cpu(sb->version) <= >> bcachefs_metadata_version_inode_v2 && >> !BCH_SB_JOURNAL_FLUSH_DELAY(sb)) Best regards, -- Li Zetao
On 2024-03-04 09:47, Li Zetao wrote: > On 2024/3/4 13:12, Su Yue wrote: >> >> On Mon 04 Mar 2024 at 11:22, Li Zetao <lizetao1@huawei.com> wrote: >> >>> There is a null-ptr-deref issue reported by kasan: >>> >>> KASAN: null-ptr-deref in range >>> [0x0000000000000000-0x0000000000000007] >>> Call Trace: >>> <TASK> >>> bch2_fs_alloc+0x1092/0x2170 [bcachefs] >>> bch2_fs_open+0x683/0xe10 [bcachefs] >>> ... >>> >>> When initializing the name of bch_fs, it needs to dynamically alloc >>> memory >>> to meet the length of the name. However, when name allocation failed, >>> it >>> will cause a null-ptr-deref access exception in subsequent string >>> copy. >>> >> bch2_printbuf_make_room() does return -ENOMEM but >> bch2_prt_printf() doesn't check the return code. And there are too >> many >> callers of bch2_prt_printf() don't check allocation_failure. > Indeed, too many callers do not check whether name allocation is > successful, which may cause hidden dangers. Maybe it is neccssary to > use somethings like __GFP_NOFAIL flag here? No need of this as printbuf is not critical for using __GFP_NOFAIL. __GFP_NOFAIL should be used carefully. It's just my nags. IOW, a fix as your fix is fine to me unless someone sends 100+ patches to fix places like this. >> >>> Fix this issue by checking if name allocation is successful. >>> >>> Fixes: 401ec4db6308 ("bcachefs: Printbuf rework") >>> Signed-off-by: Li Zetao <lizetao1@huawei.com> >>> --- >>> fs/bcachefs/super.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/fs/bcachefs/super.c b/fs/bcachefs/super.c >>> index 6b23e11825e6..24fa41bbe7e3 100644 >>> --- a/fs/bcachefs/super.c >>> +++ b/fs/bcachefs/super.c >>> @@ -818,13 +818,13 @@ static struct bch_fs *bch2_fs_alloc(struct >>> bch_sb *sb, struct bch_opts opts) >>> goto err; >>> >>> pr_uuid(&name, c->sb.user_uuid.b); >>> - strscpy(c->name, name.buf, sizeof(c->name)); >>> - printbuf_exit(&name); >>> - >>> ret = name.allocation_failure ? -BCH_ERR_ENOMEM_fs_name_alloc : >>> 0; >>> if (ret) >>> goto err; >>> >> IIRC, krealloc() doesn't free old pointer if new-size allocation >> failed. >> There is no printbuf_exit called in label err then memory leak >> happens. >> > Here krealloc() is a bit complicated: > 1.if name allocation failure happens on the first time, the old pointer > will be NULL, which cause a null-ptr-deref issue. But kfree(NULL) is safe, right? mm/slub.c: /** * kfree - free previously allocated memory * @object: pointer returned by kmalloc() or kmem_cache_alloc() * * If @object is NULL, no operation is performed. */ void kfree(const void *object)
diff --git a/fs/bcachefs/super.c b/fs/bcachefs/super.c index 6b23e11825e6..24fa41bbe7e3 100644 --- a/fs/bcachefs/super.c +++ b/fs/bcachefs/super.c @@ -818,13 +818,13 @@ static struct bch_fs *bch2_fs_alloc(struct bch_sb *sb, struct bch_opts opts) goto err; pr_uuid(&name, c->sb.user_uuid.b); - strscpy(c->name, name.buf, sizeof(c->name)); - printbuf_exit(&name); - ret = name.allocation_failure ? -BCH_ERR_ENOMEM_fs_name_alloc : 0; if (ret) goto err; + strscpy(c->name, name.buf, sizeof(c->name)); + printbuf_exit(&name); + /* Compat: */ if (le16_to_cpu(sb->version) <= bcachefs_metadata_version_inode_v2 && !BCH_SB_JOURNAL_FLUSH_DELAY(sb))