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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.04.2020 17:05, Jürgen Groß wrote:
> On 03.04.20 16:23, Jan Beulich wrote:
>> On 02.04.2020 17:46, Juergen Gross wrote:
>>> +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.

In which case perhaps extend the ASSERT() to also check max_size?

>>> +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.

In which case perhaps ASSERT(entry->max_size) between the two if()s?

Jan