Re: [PATCH] jffs: initialize inocache to NULL when initializing a jffs2_inode_info

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Double free of `f->target` on early VFS failure paths due to uninitialized slab memory.
--

commit 9fd2012cfec1b8d15df193fea97deabda99aa5e5
Author: Lucas Jeffrey <luquijeffrey@gmail.com>

jffs: initialize inocache to NULL when initializing a jffs2_inode_info

When a jffs2_inode_info is allocated via new_inode(), due to memory reuse it
can retain stale values from its previous use.

If the new jffs2_inode_info object allocated retains an old reference to a
freed inocache, the function jffs2_new_inode may crash if either
jffs2_init_acl_pre or jffs2_do_new_inode returns an error value because
when calling iput() with the inode it will eventually attempt to free again
the inocache.

Reported-by: syzbot+3a8099322b09d8d073d1@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=3a8099322b09d8d073d1
Signed-off-by: Lucas Jeffrey <luquijeffrey@gmail.com>

> diff --git a/fs/jffs2/os-linux.h b/fs/jffs2/os-linux.h
> index 86ab014a349c5..40f17218a276b 100644
> --- a/fs/jffs2/os-linux.h
> +++ b/fs/jffs2/os-linux.h
> @@ -52,6 +52,7 @@ static inline void jffs2_init_inode_info(struct jffs2_inode_info *f)
>  {
>  	f->highest_version = 0;
>  	f->fragtree = RB_ROOT;
> +	f->inocache = NULL;
>  	f->metadata = NULL;
>  	f->dents = NULL;
>  	f->target = NULL;

[Severity: Critical]
This isn't a bug introduced by this patch, but doesn't f->target suffer from
a similar issue due to uninitialized slab memory on early VFS failure paths?

When the VFS allocates a new inode via jffs2_alloc_inode() in
fs/jffs2/super.c, the object is returned from the SLAB cache without
__GFP_ZERO. It retains stale values from its previous lifecycle, including
a dangling pointer in f->target, because jffs2_free_inode() frees f->target
but does not nullify it.

JFFS2 defers initializing f->target = NULL to jffs2_init_inode_info(), which
is called after VFS initialization completes. If the VFS fails early (e.g.,
inode_init_always() fails due to an LSM memory allocation, or iget_locked()
in fs/inode.c encounters a hash collision race and drops the newly allocated
inode), the VFS destroys the inode via jffs2_free_inode() before JFFS2 ever
initializes it.

This causes a double free when kfree(f->target) executes again on the
dangling pointer:

fs/jffs2/super.c:jffs2_free_inode() {
	...
	kfree(f->target);
	kmem_cache_free(jffs2_inode_cachep, f);
}

Userspace can reliably trigger the iget_locked() hash collision race by
performing concurrent lookups (e.g., stat() or open()) on the same
non-resident inode:

fs/inode.c:iget_locked() {
	...
	if (unlikely(test_bit(__I_NEW, &inode->i_state))) {
		/* Uhhuh, somebody else created the same inode under us... */
		spin_unlock(&inode_hash_lock);
		destroy_inode(inode);
	...
}

Would it be safer to zero these fields in jffs2_alloc_inode() or ensure
jffs2_free_inode() nullifies them before returning the object to the slab
cache?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909023955.1642244-1-luquijeffrey@gmail.com?part=1

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/