Re: [PATCH v7 6/7] tools: add example application to initialize dom0less PV drivers

[Julien Grall <julien@xen.org>]

Hi Stefano,

On 13/05/2022 22:07, Stefano Stabellini wrote:
> diff --git a/tools/helpers/init-dom0less.c b/tools/helpers/init-dom0less.c
> new file mode 100644
> index 0000000000..3e7ad54da7
> --- /dev/null
> +++ b/tools/helpers/init-dom0less.c
> @@ -0,0 +1,345 @@
> +#include <stdbool.h>
> +#include <syslog.h>
> +#include <stdio.h>
> +#include <err.h>
> +#include <stdlib.h>
> +#include <sys/mman.h>
> +#include <sys/time.h>
> +#include <xenstore.h>
> +#include <xenctrl.h>
> +#include <xenguest.h>
> +#include <libxl.h>
> +#include <xenevtchn.h>
> +#include <xenforeignmemory.h>
> +#include <xen/io/xs_wire.h>
> +
> +#include "init-dom-json.h"
> +
> +#define XENSTORE_PFN_OFFSET 1
> +#define STR_MAX_LENGTH 64

Sorry, I should have spotted this earlier. Looking at the nodes below, 
the node control/platform-feature-multiprocessor-suspend would result to 
63 characters without even the domid:

42sh> echo -n 
'/local/domain//control/platform-feature-multiprocessor-suspend' | wc -c
62

So I think it would be wiser to bump the value to 128 here.

> +static bool do_xs_write_dom(struct xs_handle *xsh, xs_transaction_t t,
> +                            domid_t domid, char *path, char *val)
> +{
> +    char full_path[STR_MAX_LENGTH];
> +    struct xs_permissions perms[2];
> +
> +    perms[0].id = domid;
> +    perms[0].perms = XS_PERM_NONE;
> +    perms[1].id = 0;
> +    perms[1].perms = XS_PERM_READ;
> +
> +    if (snprintf(full_path, STR_MAX_LENGTH,
> +                 "/local/domain/%u/%s", domid, path) < 0)

The issue I mentionned above would not have been spotted because you 
only check the value is negative. From glibc version 2.1,
snprintf() returns the number of character (excluding the NUL bytes) it 
would have written if the buffer is big enough.

So to avoid writing a truncated node, you will want to check the return 
value is > 0 && < (STR_MAX_LENGTH - 1).

Looking at the code below, there are a few wrong use of snprintf(). To 
avoid another round (we are at v7 already), I would be OK if they are 
dealt after so long we bump the size of the buffer.

The rest of the code looks ok:

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall