linux-kernel - Re: [PATCH] adfs: fix memory leak in sb->s_fs

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]

Message-ID: <20251214020212.GJ1712166@ZenIV>
Date: Sun, 14 Dec 2025 02:02:12 +0000
From: Al Viro <viro@...iv.linux.org.uk>
To: Ahmet Eray Karadag <eraykrdg1@...il.com>
Cc: akpm@...ux-foundation.org, linux-kernel@...r.kernel.org,
	linux-fsdevel@...r.kernel.org, skhan@...uxfoundation.org,
	david.hunter.linux@...il.com,
	syzbot+1c70732df5fd4f0e4fbb@...kaller.appspotmail.com
Subject: Re: [PATCH] adfs: fix memory leak in sb->s_fs_info

On Sun, Dec 14, 2025 at 01:32:49AM +0000, Al Viro wrote:

> Question: if that thing is leaking all the time, why hadn't that been caught
> earlier?
> 
> Question: does it really leak all the time?  How would one check that?
> 
> Question: if it does not leak in each and every case, presumably the damn thing
> does get freed at some point; where would that be?
> 
> Question: would we, by any chance, run into a double-free with that "fix"?
> 
> 
> Please, do yourself a favour and find answers to the questions above.
> They are fairly trivial and it is the kind of exercise one has to do every
> time when dealing with something of that sort.

<spoiler alert>

.

A trivial experiment would be to mount a valid image, unmount it and see
if anything has leaked.  Finding a valid image is not hard - the second
hit when googling for "ADFS image acorn" is a github-hosted project
and right in there there's a directory called "ADFS Test Images", with
expected contents.  Whether it's legitimate or not, mounting it in a kernel
that runs under unpriveleged qemu ought to be safe enough.

And no, it doesn't leak on mount + umount

Looking for places where it could be freed in normal operation is also
not terribly hard - looking for kfree() in fs/adfs/super.c catches three
hits:

1) in adfs_put_super() we see
	struct adfs_sb_info *asb = ADFS_SB(sb);
	adfs_free_map(sb);
	kfree_rcu(asb, rcu);

2) in the end of adfs_fill_super() there's
error:
        sb->s_fs_info = NULL;
        kfree(asb);
        return ret;

3) in adfs_free_fc() we have
        struct adfs_context *asb = fc->s_fs_info;
        kfree(asb);

#2 and #3 are obviously irrelevant for the case of normal mount + umount -
adfs_fill_super() has already run at mount time (and did not hit error:)
and so did adfs_free_fc().

So we have #1 to look into.  adfs_put_super() is never called directly and
it's only reached as a member of struct super_operations adfs_sops -
something called 'put_super'.  Where would that method be called?

grep and you shall find it:
void generic_shutdown_super(struct super_block *sb)
{
        const struct super_operations *sop = sb->s_op;
 
        if (sb->s_root) {
		...
                if (sop->put_super)
                        sop->put_super(sb);

So it is called by generic_shutdown_super() in case if ->s_root had not
been NULL.  Looking for callers of generic_shutdown_super() immediately
catches
void kill_block_super(struct super_block *sb)
{
        struct block_device *bdev = sb->s_bdev;
 
        generic_shutdown_super(sb);
        if (bdev) {
                sync_blockdev(bdev);
                bdev_fput(sb->s_bdev_file);
        }
}
so it is called by adfs ->kill_sb() - both the current mainline and with
that patch.

IOW, there's our double-free.  For extra fun, it's not just kfree() + kfree(),
it's kfree_rcu() + kfree().