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

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

On Thu, Aug 29, 2024 at 02:47:24PM -0500, Bill O'Donnell wrote:
> 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.

i.e., Removing what Coverity tags as the culprit (node_free(list_del(dst_n)) from
invidx_commit(), and adding free(list) following the for-loop iteration in fstab_commit() may be
a better solution.

> 
> 
> > 
> > 
> > >  	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) {
> > 
>