Received-SPF: pass (google.com: domain of linux-kernel+bounces-90076-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
References: <20240304032203.3480001-1-lizetao1@huawei.com>
User-agent: mu4e 1.7.5; emacs 28.2
From: Su Yue <l@damenly.org>
To: Li Zetao <lizetao1@huawei.com>
Cc: kent.overstreet@linux.dev, bfoster@redhat.com,
 linux-bcachefs@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] bcachefs: Fix null-ptr-deref in bch2_fs_alloc()
Date: Mon, 04 Mar 2024 13:12:07 +0800
In-reply-to: <20240304032203.3480001-1-lizetao1@huawei.com>
Message-ID: <frx6edjh.fsf@damenly.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; format=flowed


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))