Re: [PATCH] tools/xenstored: Don't crash xenstored when Live-Update is cancelled

[Luca Fancellu <luca.fancellu@arm.com>]

> On 17 Jun 2021, at 18:38, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien GralL <jgrall@amazon.com>
> 
> As Live-Update is asynchronous, it is possible to receive a request to
> cancel it (either on the same connection or from a different one).
> 
> Currently, this will crash xenstored because do_lu_start() assumes
> lu_status will be valid. This is not the case when Live-Update has been
> cancelled. This will result to dereference a NULL pointer and
> crash Xenstored.
> 
> Rework do_lu_start() to check if lu_status is NULL and return an
> error in this case.
> 
> Fixes: af216a99fb ("tools/xenstore: add the basic framework for doing the live update")
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

> 
> ----
> 
> This is currently based on top of:
> 
> https://lore.kernel.org/xen-devel/20210616144324.31652-1-julien@xen.org
> 
> This can be re-ordered if necessary.
> ---
> tools/xenstore/xenstored_control.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/xenstore/xenstored_control.c b/tools/xenstore/xenstored_control.c
> index a045f102a420..37a3d39f20b5 100644
> --- a/tools/xenstore/xenstored_control.c
> +++ b/tools/xenstore/xenstored_control.c
> @@ -696,7 +696,18 @@ static bool do_lu_start(struct delayed_request *req)
> 	time_t now = time(NULL);
> 	const char *ret;
> 	struct buffered_data *saved_in;
> -	struct connection *conn = lu_status->conn;
> +	struct connection *conn = req->data;
> +
> +	/*
> +	 * Cancellation may have been requested asynchronously. In this
> +	 * case, lu_status will be NULL.
> +	 */
> +	if (!lu_status) {
> +		ret = "Cancellation was requested";
> +		conn = req->data;
> +		goto out;
> +	} else
> +		assert(lu_status->conn == conn);
> 
> 	if (!lu_check_lu_allowed()) {
> 		if (now < lu_status->started_at + lu_status->timeout)
> @@ -747,7 +758,7 @@ static const char *lu_start(const void *ctx, struct connection *conn,
> 	lu_status->timeout = to;
> 	lu_status->started_at = time(NULL);
> 
> -	errno = delay_request(conn, conn->in, do_lu_start, NULL, false);
> +	errno = delay_request(conn, conn->in, do_lu_start, conn, false);
> 
> 	return NULL;
> }
> -- 
> 2.17.1
> 
>