[PATCH] bcachefs: Fix null-ptr-deref in bch2_fs_alloc()

[PATCH] bcachefs: Fix null-ptr-deref in bch2_fs_alloc()

[Li Zetao]

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

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

Re: [PATCH] bcachefs: Fix null-ptr-deref in bch2_fs_alloc()

[Su Yue]

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

Re: [PATCH] bcachefs: Fix null-ptr-deref in bch2_fs_alloc()

[Li Zetao]

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

Re: [PATCH] bcachefs: Fix null-ptr-deref in bch2_fs_alloc()

[l]

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