[PATCH] ext4: added missing kfree

[PATCH] ext4: added missing kfree

[Nicolas Bretz]

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);
 	return 0;
 }
 
-- 
2.43.0

Re: [PATCH] ext4: added missing kfree

[Darrick J. Wong]

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

>  	return 0;
>  }
>  
> -- 
> 2.43.0
> 
> 

Re: [PATCH] ext4: added missing kfree

[Nicolas Bretz]

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.

>
> >       return 0;
> >  }
> >
> > --
> > 2.43.0
> >
> >

Re: [PATCH] ext4: added missing kfree

[Theodore Ts'o]

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

Re: [PATCH] ext4: added missing kfree

[Nicolas Bretz]

On Sat, May 3, 2025 at 9:45 AM Theodore Ts'o <tytso@mit.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@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

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