Re: [PATCH v7 04/12] xen: add basic hypervisor filesystem support

[Jürgen Groß <jgross@xxxxxxxx>]

On 03.04.20 16:23, Jan Beulich wrote:
> On 02.04.2020 17:46, Juergen Gross wrote:
> > Add the infrastructure for the hypervisor filesystem.
> >
> > This includes the hypercall interface and the base functions for
> > entry creation, deletion and modification.
> >
> > In order not to have to repeat the same pattern multiple times in case
> > adding a new node should BUG_ON() failure, the helpers for adding a
> > node (hypfs_add_dir() and hypfs_add_leaf()) get a nofault parameter
> > causing the BUG() in case of a failure.
> >
> > When supporting writable leafs the entry's write pointer will need to
> > be set to the function performing the write to the variable holding the
> > content. In case there are no special constraints this will be
> > hypfs_write_bool() for type XEN_HYPFS_TYPE_BOOL and hypfs_write_leaf()
> > for the other entry types.
> Seeing your HYPFS_*_INIT_WRITABLE() macros I find this a pretty
> strange requirement. Why can't the macros set the write hook right
> away?
Okay, will expand the macros.

> > +int hypfs_write_leaf(struct hypfs_entry_leaf *leaf,
> > +                     XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen)
> > +{
> > +    char *buf;
> > +    int ret;
> > +
> > +    if ( leaf->e.type != XEN_HYPFS_TYPE_STRING &&
> > +         leaf->e.type != XEN_HYPFS_TYPE_BLOB && ulen != leaf->e.size )
> > +        return -EDOM;
> > +
> > +    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 &&
> > +         memchr(buf, 0, ulen) != (buf + ulen - 1) )
> > +        goto out;
> > +
> > +    ret = 0;
> > +    memcpy(leaf->write_ptr, buf, ulen);
> > +    leaf->e.size = ulen;
> > +
> > + out:
> > +    xfree(buf);
> > +    return ret;
> > +}
> > +
> > +int hypfs_write_bool(struct hypfs_entry_leaf *leaf,
> > +                     XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen)
> > +{
> > +    bool buf;
> > +
> > +    ASSERT(leaf->e.type == XEN_HYPFS_TYPE_BOOL && leaf->e.size == 
> > sizeof(bool));
> > +
> > +    if ( ulen != leaf->e.max_size )
> Why max_size here when the ASSERT() checks size?
Just for consistency with the other write functions.

> > +static int hypfs_write(struct hypfs_entry *entry,
> > +                       XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen)
> > +{
> > +    struct hypfs_entry_leaf *l;
> > +
> > +    if ( !entry->write )
> > +        return -EACCES;
> > +
> > +    if ( ulen > entry->max_size )
> > +        return -ENOSPC;
> max_size being zero for non-writable entries, perhaps use -EACCES
> also for this special case? Together with the other comment above,
> maybe the ->write check wants replacing this way?
Checking the write function being not NULL is a nice security addon,
as I avoid to call into a non existing function. Basically both tests
would be equivalent, but this one is IMO better to avoid crashes.


Juergen