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()
Date: Mon, 04 Mar 2024 10:58:53 +0000 [thread overview]
Message-ID: <c7a48c2f58f409b3446de7655ce59a42@damenly.org> (raw)
In-Reply-To: <f472dffa-d888-1566-e1e2-313f4eb842c2@huawei.com>
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
prev parent reply other threads:[~2024-03-04 11:04 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-04 3:22 [PATCH] bcachefs: Fix null-ptr-deref in bch2_fs_alloc() Li Zetao
2024-03-04 5:12 ` Su Yue
2024-03-04 9:47 ` Li Zetao
2024-03-04 10:58 ` l [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c7a48c2f58f409b3446de7655ce59a42@damenly.org \
--to=l@damenly.org \
--cc=bfoster@redhat.com \
--cc=kent.overstreet@linux.dev \
--cc=linux-bcachefs@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=lizetao1@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).