Re: [PATCH] ext4: added missing kfree

["Theodore Ts'o" <tytso@mit.edu>]

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@kernel.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@gmail.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