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

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



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



 


Rackspace

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