lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 4 Mar 2024 17:47:46 +0800
From: Li Zetao <lizetao1@...wei.com>
To: Su Yue <l@...enly.org>
CC: <kent.overstreet@...ux.dev>, <bfoster@...hat.com>,
	<linux-bcachefs@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] bcachefs: Fix null-ptr-deref in bch2_fs_alloc()



On 2024/3/4 13:12, Su Yue wrote:
> 
> On Mon 04 Mar 2024 at 11:22, Li Zetao <lizetao1@...wei.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@...wei.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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ