Re: [PATCH] libxl: Fix handling XenStore errors in device creation

[Jürgen Groß <jgross@xxxxxxxx>]

On 27.04.24 04:17, Demi Marie Obenour wrote:
> If xenstored runs out of memory it is possible for it to fail operations
> that should succeed.  libxl wasn't robust against this, and could fail
> to ensure that the TTY path of a non-initial console was created and
> read-only for guests.  This doesn't qualify for an XSA because guests
> should not be able to run xenstored out of memory, but it still needs to
> be fixed.
>
> Add the missing error checks to ensure that all errors are properly
> handled and that at no point can a guest make the TTY path of its
> frontend directory writable.
>
> Signed-off-by: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>
Apart from one nit below:

Reviewed-by: Juergen Gross <jgross@xxxxxxxx>

> ---
>   tools/libs/light/libxl_console.c | 10 ++---
>   tools/libs/light/libxl_device.c  | 72 ++++++++++++++++++++------------
>   tools/libs/light/libxl_xshelp.c  | 13 ++++--
>   3 files changed, 59 insertions(+), 36 deletions(-)
>
> diff --git a/tools/libs/light/libxl_console.c b/tools/libs/light/libxl_console.c
> index 
> cd7412a3272a2faf4b9dab0ef4dd077e55472546..adf82aa844a4f4989111bfc8a94af18ad8e114f1
>  100644
> --- a/tools/libs/light/libxl_console.c
> +++ b/tools/libs/light/libxl_console.c
> @@ -351,11 +351,10 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t 
> domid,
>           flexarray_append(front, "protocol");
>           flexarray_append(front, LIBXL_XENCONSOLE_PROTOCOL);
>       }
> -    libxl__device_generic_add(gc, XBT_NULL, device,
> -                              libxl__xs_kvs_of_flexarray(gc, back),
> -                              libxl__xs_kvs_of_flexarray(gc, front),
> -                              libxl__xs_kvs_of_flexarray(gc, ro_front));
> -    rc = 0;
> +    rc = libxl__device_generic_add(gc, XBT_NULL, device,
> +                                   libxl__xs_kvs_of_flexarray(gc, back),
> +                                   libxl__xs_kvs_of_flexarray(gc, front),
> +                                   libxl__xs_kvs_of_flexarray(gc, ro_front));
>   out:
>       return rc;
>   }
> @@ -665,6 +664,7 @@ int libxl_device_channel_getinfo(libxl_ctx *ctx, uint32_t 
> domid,
>                 */
>                if (!val) val = "/NO-SUCH-PATH";
>                channelinfo->u.pty.path = strdup(val);
> +             if (channelinfo->u.pty.path == NULL) abort();
Even with the bad example 2 lines up, please put the "abort();" into a
line of its own.


Juergen