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

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

[Bill O'Donnell]

Fix potential use-after-free of list pointer in fstab_commit().
Use a copy of the pointer when calling invidx_commit().

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;
+		invidx_commit(win, d->children[i], list_cpy);
 	}
+	free(list_cpy);
 	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) {
-- 
2.46.0

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

[Eric Sandeen]

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.


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

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

[Bill O'Donnell]

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

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

[Bill O'Donnell]

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

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

[Eric Sandeen]

On 8/29/24 3:34 PM, Bill O'Donnell wrote:
> On Thu, Aug 29, 2024 at 02:47:24PM -0500, Bill O'Donnell wrote:

...

>>>> +	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.

I don't think that's the right approach.

invidx_commit() has this while loop, which is where coverity thinks the passed-in "list"
might get freed, before the caller uses it again:

                /* Clean up the mess we just created */
                /* find node for dst_fileidx */
                dst_n = find_invidx_node(list, dst_fileidx);
                tmp_parent = ((data_t *)(dst_n->data))->parent;
                while(dst_n != NULL) {
                    node_t *tmp_n1;

                    dst_d = dst_n->data;

                    /* close affected invidx file and stobj files */
                    for(i = 0; i < dst_d->nbr_children; i++) {
                        close_stobj_file(((data_t *)(dst_d->children[i]->data))->file_idx, BOOL_FALSE);
                    }

                    /* free_all_children on that node */
                    free_all_children(dst_n);
                    tmp_n1 = find_invidx_node(dst_n->next, dst_fileidx);
                    node_free(list_del(dst_n));
                    dst_n = tmp_n1;
                }

"list" is presumably the head of a list of items, and this is cleaning up / freeing items
within that list. Coverity seems to think that the while loop can end up getting back to
the head and freeing it, which the caller then uses again in a loop.

My guess is that coverity is wrong, but I don't think you're going to be able to prove that
(or fix this) without at least getting a sense of what this code is actually doing, and
how this list is shaped and managed...

-Eric

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

[Bill O'Donnell]

On Thu, Aug 29, 2024 at 04:56:01PM -0500, Eric Sandeen wrote:
> On 8/29/24 3:34 PM, Bill O'Donnell wrote:
> > On Thu, Aug 29, 2024 at 02:47:24PM -0500, Bill O'Donnell wrote:
> 
> ...
> 
> >>>> +	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.
> 
> I don't think that's the right approach.
> 
> invidx_commit() has this while loop, which is where coverity thinks the passed-in "list"
> might get freed, before the caller uses it again:
> 
>                 /* Clean up the mess we just created */
>                 /* find node for dst_fileidx */
>                 dst_n = find_invidx_node(list, dst_fileidx);
>                 tmp_parent = ((data_t *)(dst_n->data))->parent;
>                 while(dst_n != NULL) {
>                     node_t *tmp_n1;
> 
>                     dst_d = dst_n->data;
> 
>                     /* close affected invidx file and stobj files */
>                     for(i = 0; i < dst_d->nbr_children; i++) {
>                         close_stobj_file(((data_t *)(dst_d->children[i]->data))->file_idx, BOOL_FALSE);
>                     }
> 
>                     /* free_all_children on that node */
>                     free_all_children(dst_n);
>                     tmp_n1 = find_invidx_node(dst_n->next, dst_fileidx);
>                     node_free(list_del(dst_n));
>                     dst_n = tmp_n1;
>                 }
> 
> "list" is presumably the head of a list of items, and this is cleaning up / freeing items
> within that list. Coverity seems to think that the while loop can end up getting back to
> the head and freeing it, which the caller then uses again in a loop.
> 
> My guess is that coverity is wrong, but I don't think you're going to be able to prove that
> (or fix this) without at least getting a sense of what this code is actually doing, and
> how this list is shaped and managed...

That's my take on it as well. I'm leaning towards a false positive. I'll have another look.
Thanks for reviewing.
-Bill

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

[Bill O'Donnell]

On Thu, Aug 29, 2024 at 12:59:25PM -0500, 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().
> 
> Coverity CID 1498039.
> 
> Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>

NACK. Deeming this finding as a false positive.

> ---
>  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;
> +		invidx_commit(win, d->children[i], list_cpy);
>  	}
> +	free(list_cpy);
>  	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) {
> -- 
> 2.46.0
> 

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

[Eric Sandeen]

On 8/30/24 9:56 AM, Mark Tinguely wrote:
> I spent some time in xfsdump/xfsrestore but am not an inventory expert…I don’t see a use after free problem but I am scratching my head with the node->data->children memory.
> 
> The callers all do a free_all_children() which marks the node->data->children pointer to be NULL and then calls node_free() which does a free to node->data->children.
> 
> IMO, the inventory management code is not worth a ton of time.
> 

Thanks Mark. Agree on that, but certain security orgs think that every
"finding" of a certain severity needs triage, so we're a little bit
between a rock and a hard place here. :)

-Eric

> 
> Mark
> 
>  
> 
> *From: *Bill O'Donnell <bodonnel@redhat.com>
> *Date: *Thursday, August 29, 2024 at 5:49 PM
> *To: *Eric Sandeen <sandeen@sandeen.net>
> *Cc: *linux-xfs@vger.kernel.org <linux-xfs@vger.kernel.org>, cem@kernel.org <cem@kernel.org>, djwong@kernel.org <djwong@kernel.org>
> *Subject: *[External] : Re: [PATCH] xfsdump: prevent use-after-free in fstab_commit()
> 
> On Thu, Aug 29, 2024 at 04:56:01PM -0500, Eric Sandeen wrote:
>> On 8/29/24 3:34 PM, Bill O'Donnell wrote:
>> > On Thu, Aug 29, 2024 at 02:47:24PM -0500, Bill O'Donnell wrote:
>> 
>> ...
>> 
>> >>>> +        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.
>> 
>> I don't think that's the right approach.
>> 
>> invidx_commit() has this while loop, which is where coverity thinks the passed-in "list"
>> might get freed, before the caller uses it again:
>> 
>>                 /* Clean up the mess we just created */
>>                 /* find node for dst_fileidx */
>>                 dst_n = find_invidx_node(list, dst_fileidx);
>>                 tmp_parent = ((data_t *)(dst_n->data))->parent;
>>                 while(dst_n != NULL) {
>>                     node_t *tmp_n1;
>> 
>>                     dst_d = dst_n->data;
>> 
>>                     /* close affected invidx file and stobj files */
>>                     for(i = 0; i < dst_d->nbr_children; i++) {
>>                         close_stobj_file(((data_t *)(dst_d->children[i]->data))->file_idx, BOOL_FALSE);
>>                     }
>> 
>>                     /* free_all_children on that node */
>>                     free_all_children(dst_n);
>>                     tmp_n1 = find_invidx_node(dst_n->next, dst_fileidx);
>>                     node_free(list_del(dst_n));
>>                     dst_n = tmp_n1;
>>                 }
>> 
>> "list" is presumably the head of a list of items, and this is cleaning up / freeing items
>> within that list. Coverity seems to think that the while loop can end up getting back to
>> the head and freeing it, which the caller then uses again in a loop.
>> 
>> My guess is that coverity is wrong, but I don't think you're going to be able to prove that
>> (or fix this) without at least getting a sense of what this code is actually doing, and
>> how this list is shaped and managed...
> 
> That's my take on it as well. I'm leaning towards a false positive. I'll have another look.
> Thanks for reviewing.
> -Bill
>