Re: [PATCH v2 03/29] tools/xenlogd: connect to frontend

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Fri, Nov 10, 2023 at 1:04 PM Juergen Gross <jgross@xxxxxxxx> wrote:
>
> Add the code for connecting to frontends to xenlogd.
>
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>

> diff --git a/tools/xen-9pfsd/xen-9pfsd.c b/tools/xen-9pfsd/xen-9pfsd.c
> index c365b35fe5..cc5734402d 100644
> --- a/tools/xen-9pfsd/xen-9pfsd.c
> +++ b/tools/xen-9pfsd/xen-9pfsd.c

>
> +static int check_host_path(device *device)
> +{
> +    struct stat statbuf;
> +    char *path, *p;
> +    int ret = 1;
> +
> +    if ( !device->host_path )
> +        return 1;
> +
> +    if ( device->host_path[0] != '/' )
> +        return 1;
> +

>From v1, you stated for alloc_fid_mem(device, fid, path):
> No, "path" is always starting with a "/" if it is not empty.

And then
snprintf(fidp->path, pathlen, "%s%s", device->host_path, path);

While alloc_fid_mem() uses "%s%s"

And p9_create() uses "%s/%s"

p9_walk does:
const char *rel_path = path + strlen(device->host_path)
...
alloc_fid_mem(device, fid, rel_path);

So host_path is expected not to have a tailing '/' to ensure that
rel_path starts with a '/'.  So you want to error out for a trailing
'/' (or overwrite with '\0')?

It seems like alloc_fid_mem() should also check to ensure path is "'/'
if it is not empty".

This is all subtle and security relevant, so it's important to get
this right.  A code comment explaining the expectation of paths for
host_path vs. fids would be good.

Also, maybe using openat() would be a better approach?  Create the
dirfd pointing at the 9pfs export and then use relative paths for the
paths inside.  This would cut down on the manual path manipulations.

> +    path = strdup(device->host_path);
> +    if ( !path )
> +    {
> +        syslog(LOG_CRIT, "memory allocation failure!");
> +        return 1;
> +    }
> +
> +    for ( p = path; p; )
> +    {
> +        p = strchr(p + 1, '/');
> +        if ( p )
> +            *p = 0;
> +        if ( !stat(path, &statbuf) )
> +        {
> +            if ( !(statbuf.st_mode & S_IFDIR) )
> +                break;
> +            if ( !p )
> +            {
> +                ret = 0;
> +                break;
> +            }
> +            *p = '/';
> +            continue;
> +        }
> +        if ( mkdir(path, 0777) )
> +            break;
> +        if ( p )
> +            *p = '/';
> +    }
> +
> +    free(path);
> +    return ret;
> +}
> +

> +
> +static int write_backend_node(device *device, const char *node, const char 
> *val)
> +{
> +    struct path p;
> +    struct xs_permissions perms[2] = {
> +        { .id = 0, .perms = XS_PERM_NONE },

This hard codes dom0.  If xs_permissions supported DOMID_SELF, it
wouldn't need to be looked up.

> +        { .id = device->domid, .perms = XS_PERM_READ }
> +    };
> +
> +    construct_backend_path(device, node, &p);
> +    if ( !xs_write(xs, XBT_NULL, p.path, val, strlen(val)) )
> +    {
> +        syslog(LOG_ERR, "error writing bacḱend node \"%s\" for device %u/%u",
> +               node, device->domid, device->devid);
> +        return 1;
> +    }
> +
> +    if ( !xs_set_permissions(xs, XBT_NULL, p.path, perms, 2) )
> +    {
> +        syslog(LOG_ERR, "error setting permissions for \"%s\"", p.path);
> +        return 1;
> +    }
> +
> +    return 0;
> +}
> +

> +
> +static void connect_device(device *device)
> +{
> +    unsigned int val;
> +    unsigned int ring_idx;
> +    char node[20];
> +    struct ring *ring;
> +    xenevtchn_port_or_error_t evtchn;
> +
> +    val = read_frontend_node_uint(device, "version", 0);
> +    if ( val != 1 )
> +        return connect_err(device, "frontend specifies illegal version");
> +    device->num_rings = read_frontend_node_uint(device, "num-rings", 0);
> +    if ( device->num_rings < 1 || device->num_rings > MAX_RINGS )
> +        return connect_err(device, "frontend specifies illegal ring number");
> +
> +    for ( ring_idx = 0; ring_idx < device->num_rings; ring_idx++ )
> +    {
> +        ring = calloc(1, sizeof(*ring));
> +        if ( !ring )
> +            return connect_err(device, "could not allocate ring memory");
> +        device->ring[ring_idx] = ring;
> +        ring->device = device;
> +        pthread_cond_init(&ring->cond, NULL);
> +        pthread_mutex_init(&ring->mutex, NULL);
> +
> +

extra blank line.

Regards,
Jason