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