[libvirt] [PATCH] snapshot: Fix use-after-free during snapshot delete

[Roman Bogorodskiy]

  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 at gmail.com>
> Signed-off-by: Eric Blake <eblake at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 455 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190408/6666e2df/attachment-0001.sig>