Re: [PATCH] xfsdump: prevent use-after-free in fstab_commit()

[Bill O'Donnell <bodonnel@redhat.com>]

On Thu, Aug 29, 2024 at 02:35:27PM -0500, Eric Sandeen wrote:
> On 8/29/24 12:59 PM, Bill O'Donnell wrote:
> > Fix potential use-after-free of list pointer in fstab_commit().
> > Use a copy of the pointer when calling invidx_commit().
> 
> I'm not sure how (or even if) the use after free happens -xfsdump is so hard
> to read - but ...
> 
> > Coverity CID 1498039.
> > 
> > Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>
> > ---
> >  invutil/fstab.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/invutil/fstab.c b/invutil/fstab.c
> > index 88d849e..fe2b1f9 100644
> > --- a/invutil/fstab.c
> > +++ b/invutil/fstab.c
> > @@ -66,6 +66,7 @@ fstab_commit(WINDOW *win, node_t *current, node_t *list)
> >      data_t *d;
> >      invt_fstab_t *fstabentry;
> >      int fstabentry_idx;
> > +    node_t *list_cpy = list;
> >  
> >      n = current;
> >      if(n == NULL || n->data == NULL)
> > @@ -77,8 +78,10 @@ fstab_commit(WINDOW *win, node_t *current, node_t *list)
> >  
> >      if(d->deleted == BOOL_TRUE && d->imported == BOOL_FALSE) {
> >  	for(i = 0; i < d->nbr_children; i++) {
> > -	    invidx_commit(win, d->children[i], list);
> > +		list_cpy = list;
> 
> this copies the memory address stored in "list" into your new pointer var "list_cpy"
> 
> > +		invidx_commit(win, d->children[i], list_cpy);
> 
> If invidx_commit() frees the 2nd argument, it frees the memory address pointed
> to by both list and list_cpy.
> 
> Storing the same memory address in 2 pointer variables does not prevent that memory
> from being freed.
> 
> >  	}
> > +	free(list_cpy);
> 
> and then this would double-free that same memory address.

I see that now. This code is indeed difficult to grok.

Perhaps (if this a legitimate finding of use after free), instead of having the memory
freed in invidx_commit(), it should instead be freed once in fstab_commit() after the iterations
of the for-loops in that function. I'll have a look at that possibility.

Thanks-
Bill


> 
> 
> >  	mark_all_children_commited(current);
> >  
> >  	fstabentry_idx = (int)(((long)fstabentry - (long)fstab_file[fidx].mapaddr - sizeof(invt_counter_t)) / sizeof(invt_fstab_t));
> > @@ -101,8 +104,10 @@ fstab_commit(WINDOW *win, node_t *current, node_t *list)
> >  	invt_fstab_t *dest;
> >  
> >  	for(i = 0; i < d->nbr_children; i++) {
> > -	    invidx_commit(win, d->children[i], list);
> > +		list_cpy = list;	
> > +		invidx_commit(win, d->children[i], list_cpy);
> >  	}
> > +	free(list_cpy);
> >  	mark_all_children_commited(current);
> >  
> >  	if(find_matching_fstab(0, fstabentry) >= 0) {
>