Commit b647d2195 introduced a use-after-free situation when the caller
is trying to delete a snapshot and its children: if the callback
function deletes the parent, it is no longer safe to query the parent
to learn which children also need to be deleted (where we previously
saved deleting the parent for last).  To fix the problem, while still
maintaining support for topological visits of callback functions, we
have to stash off any information needed for later traversal prior to
using a callback function (virDomainMomentForEachChild already does
this, it is only virDomainMomentActOnDescendant that was running into
problems).

Sadly, the testsuite did not cover the problem at the time. Worse,
even though I later added commit 280a2b41e to catch problems like
this, and even though that test is indeed sufficient to detect the
problem when run under valgrind or suitable MALLOC_PERTURB_ settings,
I'm guilty of not running the test in such an environment.  Thus,
v5.2.0 has a regression that could have been prevented had we used the
testsuite to its full power. On the bright side, deleting snapshots
requires ACL domain:snapshot, which is arguably as powerful as
domain:write, so I don't think this use-after-free forms a security
hole.

At some point, it would be nice to convert virDomainMomentObj into a
virObject, at which point, the solution is even simpler: add
virObjectRef/Unref around the callback. But as that will require
auditing even more places in the code, I went with the simplest patch
for the regression fix.

Fixes: b647d2195
Reported-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 src/conf/virdomainmomentobjlist.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/conf/virdomainmomentobjlist.c b/src/conf/virdomainmomentobjlist.c
index 65e82f632c..66eb66017b 100644
--- a/src/conf/virdomainmomentobjlist.c
+++ b/src/conf/virdomainmomentobjlist.c
@@ -80,9 +80,11 @@ virDomainMomentActOnDescendant(void *payload,
 {
     virDomainMomentObjPtr obj = payload;
     struct moment_act_on_descendant *curr = data;
+    virDomainMomentObj tmp = *obj;

+    /* Careful: curr->iter can delete obj, hence the need for tmp */
     (curr->iter)(payload, name, curr->data);
-    curr->number += 1 + virDomainMomentForEachDescendant(obj,
+    curr->number += 1 + virDomainMomentForEachDescendant(&tmp,
                                                          curr->iter,
                                                          curr->data);
     return 0;
-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

  Eric Blake wrote:

> Commit b647d2195 introduced a use-after-free situation when the caller
> is trying to delete a snapshot and its children: if the callback
> function deletes the parent, it is no longer safe to query the parent
> to learn which children also need to be deleted (where we previously
> saved deleting the parent for last).  To fix the problem, while still
> maintaining support for topological visits of callback functions, we
> have to stash off any information needed for later traversal prior to
> using a callback function (virDomainMomentForEachChild already does
> this, it is only virDomainMomentActOnDescendant that was running into
> problems).
> 
> Sadly, the testsuite did not cover the problem at the time. Worse,
> even though I later added commit 280a2b41e to catch problems like
> this, and even though that test is indeed sufficient to detect the
> problem when run under valgrind or suitable MALLOC_PERTURB_ settings,
> I'm guilty of not running the test in such an environment.  Thus,
> v5.2.0 has a regression that could have been prevented had we used the
> testsuite to its full power. On the bright side, deleting snapshots
> requires ACL domain:snapshot, which is arguably as powerful as
> domain:write, so I don't think this use-after-free forms a security
> hole.
> 
> At some point, it would be nice to convert virDomainMomentObj into a
> virObject, at which point, the solution is even simpler: add
> virObjectRef/Unref around the callback. But as that will require
> auditing even more places in the code, I went with the simplest patch
> for the regression fix.
> 
> Fixes: b647d2195
> Reported-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  src/conf/virdomainmomentobjlist.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/src/conf/virdomainmomentobjlist.c b/src/conf/virdomainmomentobjlist.c
> index 65e82f632c..66eb66017b 100644
> --- a/src/conf/virdomainmomentobjlist.c
> +++ b/src/conf/virdomainmomentobjlist.c
> @@ -80,9 +80,11 @@ virDomainMomentActOnDescendant(void *payload,
>  {
>      virDomainMomentObjPtr obj = payload;
>      struct moment_act_on_descendant *curr = data;
> +    virDomainMomentObj tmp = *obj;
> 
> +    /* Careful: curr->iter can delete obj, hence the need for tmp */
>      (curr->iter)(payload, name, curr->data);
> -    curr->number += 1 + virDomainMomentForEachDescendant(obj,
> +    curr->number += 1 + virDomainMomentForEachDescendant(&tmp,
>                                                           curr->iter,
>                                                           curr->data);
>      return 0;

This fixes the problem for me, thanks.
The change itself looks reasonable to me, but I'm not familiar with the code
enough to go with Reviewed-by I guess.

Roman Bogorodskiy
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 4/8/19 1:35 PM, Roman Bogorodskiy wrote:
>   Eric Blake wrote:
> 
>> Commit b647d2195 introduced a use-after-free situation when the caller
>> is trying to delete a snapshot and its children: if the callback

>>
>> Fixes: b647d2195
>> Reported-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>  src/conf/virdomainmomentobjlist.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/conf/virdomainmomentobjlist.c b/src/conf/virdomainmomentobjlist.c
>> index 65e82f632c..66eb66017b 100644
>> --- a/src/conf/virdomainmomentobjlist.c
>> +++ b/src/conf/virdomainmomentobjlist.c
>> @@ -80,9 +80,11 @@ virDomainMomentActOnDescendant(void *payload,
>>  {
>>      virDomainMomentObjPtr obj = payload;
>>      struct moment_act_on_descendant *curr = data;
>> +    virDomainMomentObj tmp = *obj;
>>
>> +    /* Careful: curr->iter can delete obj, hence the need for tmp */
>>      (curr->iter)(payload, name, curr->data);
>> -    curr->number += 1 + virDomainMomentForEachDescendant(obj,
>> +    curr->number += 1 + virDomainMomentForEachDescendant(&tmp,
>>                                                           curr->iter,
>>                                                           curr->data);
>>      return 0;
> 
> This fixes the problem for me, thanks.
> The change itself looks reasonable to me, but I'm not familiar with the code
> enough to go with Reviewed-by I guess.

Well, Tested-by is better than nothing; so I've gone ahead and pushed
it. Thanks for checking that it works :)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list