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@xxxxxxxxxx>
> *Date: *Thursday, August 29, 2024 at 5:49 PM
> *To: *Eric Sandeen <sandeen@xxxxxxxxxxx>
> *Cc: *linux-xfs@xxxxxxxxxxxxxxx <linux-xfs@xxxxxxxxxxxxxxx>, cem@xxxxxxxxxx <cem@xxxxxxxxxx>, djwong@xxxxxxxxxx <djwong@xxxxxxxxxx>
> *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
>
