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

[Stefano Stabellini <sstabellini@kernel.org>]

On Sat, 14 May 2022, Julien Grall wrote:
> 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>

Thank you that is very reasonable. I committed the series with
STR_MAX_LENGTH set to 128. I'll send a separate patch to improve the
snprintf checks.