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

[Luca Fancellu <luca.fancellu@xxxxxxx>]

> On 17 Jun 2021, at 18:38, Julien Grall <julien@xxxxxxx> wrote:
> 
> From: Julien GralL <jgrall@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>

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

> 
> ----
> 
> This is currently based on top of:
> 
> https://lore.kernel.org/xen-devel/20210616144324.31652-1-julien@xxxxxxx
> 
> 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
> 
>