| Message ID | 20240829175925.59281-1-bodonnel@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | xfsdump: prevent use-after-free in fstab_commit() | expand |
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) {
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) { >
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) { > > >
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
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
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 >
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 >
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) {
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(-)