Received-SPF: pass (google.com: domain of linux-kernel+bounces-90475-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
Date: Mon, 04 Mar 2024 10:58:53 +0000
From: 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()
In-Reply-To: <f472dffa-d888-1566-e1e2-313f4eb842c2@huawei.com>
References: <20240304032203.3480001-1-lizetao1@huawei.com>
 <frx6edjh.fsf@damenly.org> <f472dffa-d888-1566-e1e2-313f4eb842c2@huawei.com>
Message-ID: <c7a48c2f58f409b3446de7655ce59a42@damenly.org>
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit

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)


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