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@xxxxxxxxx>
> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
