linux-ext4 - Re: [PATCH] ext4: added missing kfree

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] [day] [month] [year] [list]

Message-ID: <CAPXz4EO=94LOxmp6Orbv5P+HMHua-1tV76eh40VStH=bgiDWnA@mail.gmail.com>
Date: Sat, 3 May 2025 17:48:50 -0700
From: Nicolas Bretz <bretznic@...il.com>
To: "Theodore Ts'o" <tytso@....edu>
Cc: "Darrick J. Wong" <djwong@...nel.org>, linux-ext4@...r.kernel.org, jack@...e.cz, 
	adilger.kernel@...ger.ca
Subject: Re: [PATCH] ext4: added missing kfree

On Sat, May 3, 2025 at 9:45 AM Theodore Ts'o <tytso@....edu> wrote:
>
> On Fri, May 02, 2025 at 03:26:22PM -0700, Nicolas Bretz wrote:
> > On Fri, May 2, 2025 at 1:37 PM Darrick J. Wong <djwong@...nel.org> wrote:
> > >
> > > On Fri, May 02, 2025 at 10:40:12AM -0700, Nicolas Bretz wrote:
> > > > Added one missing kfree to fsmap.c
> > > >
> > > > Signed-off-by: Nicolas Bretz <bretznic@...il.com>
> > > > ---
> > > >  fs/ext4/fsmap.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/fs/ext4/fsmap.c b/fs/ext4/fsmap.c
> > > > index b232c2767534..d41210abea0c 100644
> > > > --- a/fs/ext4/fsmap.c
> > > > +++ b/fs/ext4/fsmap.c
> > > > @@ -304,6 +304,7 @@ static inline int ext4_getfsmap_fill(struct list_head *meta_list,
> > > >       fsm->fmr_length = len;
> > > >       list_add_tail(&fsm->fmr_list, meta_list);
> > > >
> > > > +     kfree(fsm);
> > >
> > > OI: UAF, NAK.
> > >
> > > --D
> >
> > I apologize, it definitely wasn't my intention. I guess not really
> > putting my best foot forward...
> > I don't yet fully get the UAF in this instance, but I'm studying it.
>
> UAF == "Use After Free"
>
> What this function does is to allocate a data struture, and then add
> it to a linked list.  This is what list_add_tail() does.
>
> By adding the kfree(), this will result in the callers of
> ext4_getfsap_fill() trying to dereference a pointer to memory that has
> been freed.  So your patch very cleary introduces a bug, and makes it
> clear you (a) don't understand what lst_add_tail() does, and (b)
> dont't understand what the ext4_getfsap_fill()function does, and (c)
> almost certainkly didn't understand how to test a particular code path
> and/or didn't bother to adequately test the code that you were trying
> to modify.
>
> For future reference, the commit description for this patch is also
> not adequate.  Don't replicate in English what the change in the C
> code is.  Explain *why* the change was made; what bug were you trying
> to fix?  Or what performance optimization were you going for?  And
> it's often a good idea to explain how you tested your change.
>
> Cheers,
>
>                                         - Ted

I thought I understood what list_add_tail() does, and also
__list_add(), but obviously I was very wrong.

All your points are well taken. It was outright sloppy of me and I
apologize again for the trouble I caused.
Nic