Received-SPF: pass (google.com: domain of linux-kernel+bounces-90342-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <f472dffa-d888-1566-e1e2-313f4eb842c2@huawei.com>
Date: Mon, 4 Mar 2024 17:47:46 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.0
Subject: Re: [PATCH] bcachefs: Fix null-ptr-deref in bch2_fs_alloc()
Content-Language: en-US
To: Su Yue <l@damenly.org>
CC: <kent.overstreet@linux.dev>, <bfoster@redhat.com>,
	<linux-bcachefs@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20240304032203.3480001-1-lizetao1@huawei.com>
 <frx6edjh.fsf@damenly.org>
From: Li Zetao <lizetao1@huawei.com>
In-Reply-To: <frx6edjh.fsf@damenly.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit


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