[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v5 3/8] xen: add basic hypervisor filesystem support



On 19.02.2020 09:11, Juergen Gross wrote:
> +static int hypfs_get_path_user(char *buf,
> +                               XEN_GUEST_HANDLE_PARAM(const_char) uaddr,
> +                               unsigned long ulen)
> +{
> +    if ( ulen > XEN_HYPFS_MAX_PATHLEN )
> +        return -EINVAL;
> +
> +    if ( copy_from_guest(buf, uaddr, ulen) )
> +        return -EFAULT;
> +
> +    if ( buf[ulen - 1] )
> +        return -EINVAL;

I (still, but I may not have said so before) wonder whether
memchr(buf, 0, ulen) != buf + ulen - 1 wouldn't be better here.

> +int hypfs_write_leaf(struct hypfs_entry_leaf *leaf,
> +                     XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen)
> +{
> +    char *buf;
> +    int ret;
> +
> +    if ( ulen > leaf->e.size )
> +        return -ENOSPC;

Okay, this makes sure you don't overrun the internal buffer.
What about the opposite mismatch (ulen < leaf->e.size)? The
result, except perhaps for (nul-terminated) strings, is not
going to be very useful, at the very least.

> +    buf = xmalloc_array(char, ulen);
> +    if ( !buf )
> +        return -ENOMEM;
> +
> +    ret = -EFAULT;
> +    if ( copy_from_guest(buf, uaddr, ulen) )
> +        goto out;
> +
> +    ret = -EINVAL;
> +    if ( leaf->e.type == XEN_HYPFS_TYPE_STRING && buf[ulen] )

buf[ulen - 1] I guess? Or, as above, memchr() again?

> +int hypfs_write_bool(struct hypfs_entry_leaf *leaf,
> +                     XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen)
> +{
> +    union {
> +        char buf[8];
> +        uint8_t u8;
> +        uint16_t u16;
> +        uint32_t u32;
> +        uint64_t u64;
> +    } u;
> +
> +    ASSERT(leaf->e.type == XEN_HYPFS_TYPE_UINT && leaf->e.size <= 8);
> +
> +    if ( ulen != leaf->e.size )
> +        return -EDOM;
> +
> +    if ( copy_from_guest(u.buf, uaddr, ulen) )
> +        return -EFAULT;
> +
> +    switch ( leaf->e.size )
> +    {
> +    case 1:
> +        *(uint8_t *)leaf->write_ptr = !!u.u8;
> +        break;
> +    case 2:
> +        *(uint16_t *)leaf->write_ptr = !!u.u16;
> +        break;
> +    case 4:
> +        *(uint32_t *)leaf->write_ptr = !!u.u32;
> +        break;
> +    case 8:
> +        *(uint64_t *)leaf->write_ptr = !!u.u64;
> +        break;
> +    }

Looking at this again, is there really a need for uint64_t support
here? I.e. can't you cap at unsigned long (or even unsigned int),
and perhaps additionally avoid use of fixed width types here
altogether (some trickery may be needed for 32-bit's
sizeof(long) == sizeof(int))?

> --- /dev/null
> +++ b/xen/include/public/hypfs.h
> @@ -0,0 +1,127 @@
> +/******************************************************************************
> + * Xen Hypervisor Filesystem
> + *
> + * Copyright (c) 2019, SUSE Software Solutions Germany GmbH
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to
> + * deal in the Software without restriction, including without limitation the
> + * rights to use, copy, modify, merge, publish, distribute, sublicense, 
> and/or
> + * sell copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL 
> THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + *
> + */
> +
> +#ifndef __XEN_PUBLIC_HYPFS_H__
> +#define __XEN_PUBLIC_HYPFS_H__
> +
> +#include "xen.h"
> +
> +/*
> + * Definitions for the __HYPERVISOR_hypfs_op hypercall.
> + */
> +
> +/* Highest version number of the hypfs interface currently defined. */
> +#define XEN_HYPFS_VERSION      1
> +
> +/* Maximum length of a path in the filesystem. */
> +#define XEN_HYPFS_MAX_PATHLEN 1024

I think it would be nice if the blank padding here matched that
of the other #define-s up and down from here.

> +/*
> + * XEN_HYPFS_OP_get_version
> + *
> + * Read highest interface version supported by the hypervisor.
> + *
> + * Possible return values:
> + * >0: highest supported interface version
> + * <0: negative Xen errno value
> + */
> +#define XEN_HYPFS_OP_get_version     0
> +
> +/*
> + * XEN_HYPFS_OP_read
> + *
> + * Read a filesystem entry.
> + *
> + * Returns the direntry and contents of an entry in the buffer supplied by 
> the
> + * caller (struct xen_hypfs_direntry with the contents following directly
> + * after it).
> + * The data buffer must be at least the size of the direntry returned in 
> order
> + * to have success. If the data buffer was not large enough for all the data

Looks like the "to have success" is stale now?

> + * -ENOBUFS and no entry data is returned, but the direntry will contain the
> + * needed size for the returned data.
> + * The format of the contents is according to its entry type and encoding.
> + * The contents of a directory are multiple struct xen_hypfs_dirlistentry
> + * items.
> + *
> + * arg1: XEN_GUEST_HANDLE(path name)
> + * arg2: length of path name (including trailing zero byte)
> + * arg3: XEN_GUEST_HANDLE(data buffer written by hypervisor)
> + * arg4: data buffer size
> + *
> + * Possible return values:
> + * 0: success (at least the direntry was returned)

As is this?

> +static inline void hypfs_string_set(struct hypfs_entry_leaf *leaf,
> +                                    const char *str)
> +{
> +    leaf->content = str;
> +    leaf->e.size = strlen(str) + 1;
> +}

This looks at least risky to me, as the function name does in
no way indicate that no copy of the string will be made. Hence
its use with e.g. .init.rodata contents or a stack variable
will not produce the intended result.

> +#define HYPFS_UINT_INIT(var, nam, contvar)        \
> +    struct hypfs_entry_leaf __read_mostly var = { \
> +        .e.type = XEN_HYPFS_TYPE_UINT,            \
> +        .e.encoding = XEN_HYPFS_ENC_PLAIN,        \
> +        .e.name = nam,                            \
> +        .e.size = sizeof(contvar),                \
> +        .e.read = hypfs_read_leaf,                \
> +        .content = &contvar,                      \
> +    }
> +
> +#define HYPFS_INT_INIT(var, nam, contvar)         \
> +    struct hypfs_entry_leaf __read_mostly var = { \
> +        .e.type = XEN_HYPFS_TYPE_INT,             \
> +        .e.encoding = XEN_HYPFS_ENC_PLAIN,        \
> +        .e.name = nam,                            \
> +        .e.size = sizeof(contvar),                \
> +        .e.read = hypfs_read_leaf,                \
> +        .content = &contvar,                      \
> +    }
> +
> +#define HYPFS_BOOL_INIT(var, nam, contvar)        \
> +    struct hypfs_entry_leaf __read_mostly var = { \
> +        .e.type = XEN_HYPFS_TYPE_BOOL,            \
> +        .e.encoding = XEN_HYPFS_ENC_PLAIN,        \
> +        .e.name = nam,                            \
> +        .e.size = sizeof(contvar),                \
> +        .e.read = hypfs_read_leaf,                \
> +        .content = &contvar,                      \
> +    }

Quite a lot of redundancy for just a single line of difference
between the instance. Perhaps have another helper macro?

> --- a/xen/include/xlat.lst
> +++ b/xen/include/xlat.lst
> @@ -86,6 +86,8 @@
>  ?    vcpu_hvm_context                hvm/hvm_vcpu.h
>  ?    vcpu_hvm_x86_32                 hvm/hvm_vcpu.h
>  ?    vcpu_hvm_x86_64                 hvm/hvm_vcpu.h
> +?    xen_hypfs_direntry              hypfs.h
> +?    xen_hypfs_dirlistentry          hypfs.h

Where are the checking macros used that these produce?

> --- a/xen/xsm/flask/policy/access_vectors
> +++ b/xen/xsm/flask/policy/access_vectors
> @@ -67,6 +67,8 @@ class xen
>      lockprof
>  # XEN_SYSCTL_cpupool_op
>      cpupool_op
> +# hypfs hypercall
> +    hypfs_op
>  # XEN_SYSCTL_scheduler_op with XEN_DOMCTL_SCHEDOP_getinfo, 
> XEN_SYSCTL_sched_id, XEN_DOMCTL_SCHEDOP_getvcpuinfo
>      getscheduler
>  # XEN_SYSCTL_scheduler_op with XEN_DOMCTL_SCHEDOP_putinfo, 
> XEN_DOMCTL_SCHEDOP_putvcpuinfo

I may not know enough about XSM to see why you can get away without
also modifying flask/hooks.c.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.