linux-kernel - Re: [PATCH] efivarfs: Suppress false-positive kmemleak warning for sfi

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: <ec0a66f40bc826bd016f338568e01908b86be35a.camel@HansenPartnership.com>
Date: Wed, 16 Jul 2025 09:26:03 -0400
From: James Bottomley <James.Bottomley@...senPartnership.com>
To: Breno Leitao <leitao@...ian.org>
Cc: Ard Biesheuvel <ardb@...nel.org>, Jeremy Kerr <jk@...abs.org>, 
 linux-efi@...r.kernel.org, linux-kernel@...r.kernel.org,
 kernel-team@...a.com
Subject: Re: [PATCH] efivarfs: Suppress false-positive kmemleak warning for
 sfi

On Wed, 2025-07-16 at 06:16 -0700, Breno Leitao wrote:
> On Wed, Jul 16, 2025 at 09:09:00AM -0400, James Bottomley wrote:
> > On Wed, 2025-07-16 at 08:31 -0400, James Bottomley wrote:
> > [...]
> > > If the theory is correct, the leak is genuine and we need to
> > > implement .free in efivarfs_context_ops to fix it.
> > 
> > Rather than trying to trace this, which will be hard, it might be
> > easier just to try the fix below (not even compile tested) and see
> > if
> > it works.  Note there's no danger of a double free because when fc-
> > > s_fs_info is copied to sb->s_fs_info, the field is nulled in fc.
> > 
> > Regards,
> > 
> > James
> > 
> > ---
> > 
> > diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
> > index c900d98bf494..90a619d027fd 100644
> > --- a/fs/efivarfs/super.c
> > +++ b/fs/efivarfs/super.c
> > @@ -390,10 +390,16 @@ static int efivarfs_reconfigure(struct
> > fs_context *fc)
> >  	return 0;
> >  }
> >  
> > +static void efivarfs_fs_context_free(struct fs_context *fc)
> > +{
> > +	kfree(fc->s_fs_info);
> > +}
> > +
> >  static const struct fs_context_operations efivarfs_context_ops = {
> >  	.get_tree	= efivarfs_get_tree,
> >  	.parse_param	= efivarfs_parse_param,
> >  	.reconfigure	= efivarfs_reconfigure,
> > +	.free		= efivarfs_fs_context_free,
> >  };
> >  
> >  static int efivarfs_check_missing(efi_char16_t *name16, efi_guid_t
> > vendor,
> 
> Hello James,
> 
> I was testing something very similar based on your previous email. I
> can
> confirm that the following patch make kmemleak happy. 
> 
> Regarding the fixes, I think this was introduced in commit
> 5329aa5101f73c ("efivarfs: Add uid/gid mount options")
> 
> commit 035521e8a5029ea814337d680e0552ccab1f97e2
> Author: Breno Leitao <leitao@...ian.org>
> Date:   Wed Jul 16 06:08:57 2025 -0700
> 
>     efivarfs: Fix memory leak of efivarfs_fs_info in fs_context error
> paths
>     
>     When processing mount options, efivarfs allocates
> efivarfs_fs_info (sfi)
>     early in fs_context initialization. However, sfi is associated
> with the
>     superblock and typically freed when the superblock is destroyed.
> If the
>     fs_context is released (final put) before fill_super is
> called—such as
>     on error paths or during reconfiguration—the sfi structure would
> leak,
>     as ownership never transfers to the superblock.
>     
>     Implement the .free callback in efivarfs_context_ops to ensure
> any
>     allocated sfi is properly freed if the fs_context is torn down
> before
>     fill_super, preventing this memory leak.
>     
>     Suggested-by: James Bottomley
> <James.Bottomley@...senPartnership.com>
>     Signed-off-by: Breno Leitao <leitao@...ian.org>
> 
> diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
> index c900d98bf4945..07a3b9293396b 100644
> --- a/fs/efivarfs/super.c
> +++ b/fs/efivarfs/super.c
> @@ -390,10 +390,22 @@ static int efivarfs_reconfigure(struct
> fs_context *fc)
>  	return 0;
>  }
>  
> +static void efivarfs_free(struct fs_context *fc)
> +{
> +	struct efivarfs_fs_info *sfi;
> +
> +	sfi = fc->s_fs_info;
> +	if (!sfi)
> +		return;

Here, you'll excite the coccinelle checkers looking for if(x) free(x)
because free() already also has a test for NULL.

Other than that elision, it looks fine to me.

Regards,

James