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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 10/05/2024 9:05 am, Jürgen Groß wrote:
> 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@invisiblethingslab.com>
>
> Apart from one nit below:
>
> Reviewed-by: Juergen Gross <jgross@suse.com>
>
>> ---
>>   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.

I've fixed this on commit.

~Andrew