Re: [PATCH] bcachefs: Fix null-ptr-deref in bch2_fs_alloc()
From: l
Date: Mon Mar 04 2024 - 06:04:14 EST
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@xxxxxxxxxx> 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@xxxxxxxxxx>
---
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