[RFC,1/3] xen/hypfs: add support for bool leafs in dynamic directories

Commit Message

Jürgen Groß Dec. 9, 2020, 4:16 p.m. UTC

Add support for reading fixed sized leafs and writing bool leafs in
dynamic directories with the backing variable being a member of the
structure anchored in struct hypfs_dyndir->data.

This adds the related leaf read and write functions and a helper
macro HYPFS_STRUCT_ELEM() for referencing the element.

Signed-off-by: Juergen Gross <[e-mail]>
---
 xen/common/hypfs.c      | 53 +++++++++++++++++++++++++++++++++++------
 xen/include/xen/hypfs.h |  7 ++++++
 2 files changed, 53 insertions(+), 7 deletions(-)

Comments

Jan Beulich Jan. 8, 2021, 3:38 p.m. UTC | #1

On 09.12.2020 17:16, Juergen Gross wrote:
> --- a/xen/common/hypfs.c
> +++ b/xen/common/hypfs.c
> @@ -501,17 +501,26 @@ int hypfs_read_dir(const struct hypfs_entry *entry,
>      return 0;
>  }
>  
> -int hypfs_read_leaf(const struct hypfs_entry *entry,
> -                    XEN_GUEST_HANDLE_PARAM(void) uaddr)
> +static int hypfs_read_leaf_off(const struct hypfs_entry *entry,
> +                               XEN_GUEST_HANDLE_PARAM(void) uaddr,
> +                               void *off)
>  {
>      const struct hypfs_entry_leaf *l;
>      unsigned int size = entry->funcs->getsize(entry);
> +    const void *ptr;
>  
>      ASSERT(this_cpu(hypfs_locked) != hypfs_unlocked);
>  
>      l = container_of(entry, const struct hypfs_entry_leaf, e);
> +    ptr = off ? off + (unsigned long)l->u.content : l->u.content;

This is very irritating - you effectively add together two
pointers. And even if this was correct for some reason, it
would seem better readable as

    ptr = l->u.content + (unsigned long)off;

to me.

> --- a/xen/include/xen/hypfs.h
> +++ b/xen/include/xen/hypfs.h
> @@ -160,6 +160,8 @@ static inline void hypfs_string_set_reference(struct hypfs_entry_leaf *leaf,
>      HYPFS_FIXEDSIZE_INIT(var, XEN_HYPFS_TYPE_BOOL, nam, contvar, \
>                           &hypfs_bool_wr_funcs, 1)
>  
> +#define HYPFS_STRUCT_ELEM(type, elem)    (((type *)NULL)->elem)

Kind of similar here - this very much looks like a NULL
deref, avoidable by a user only if it takes the address of
the construct. If there's really some non-pointer value
to be encoded here, it would be better to avoid misuse by
making the construct safe in a self-contained way.

Jan

Patch

diff --git a/xen/common/hypfs.c b/xen/common/hypfs.c
index 087c63b92f..caee48cc97 100644
--- a/xen/common/hypfs.c
+++ b/xen/common/hypfs.c
@@ -501,17 +501,26 @@  int hypfs_read_dir(const struct hypfs_entry *entry,
     return 0;
 }
 
-int hypfs_read_leaf(const struct hypfs_entry *entry,
-                    XEN_GUEST_HANDLE_PARAM(void) uaddr)
+static int hypfs_read_leaf_off(const struct hypfs_entry *entry,
+                               XEN_GUEST_HANDLE_PARAM(void) uaddr,
+                               void *off)
 {
     const struct hypfs_entry_leaf *l;
     unsigned int size = entry->funcs->getsize(entry);
+    const void *ptr;
 
     ASSERT(this_cpu(hypfs_locked) != hypfs_unlocked);
 
     l = container_of(entry, const struct hypfs_entry_leaf, e);
+    ptr = off ? off + (unsigned long)l->u.content : l->u.content;
+
+    return copy_to_guest(uaddr, ptr, size) ? -EFAULT : 0;
+}
 
-    return copy_to_guest(uaddr, l->u.content, size) ?  -EFAULT : 0;
+int hypfs_read_leaf(const struct hypfs_entry *entry,
+                    XEN_GUEST_HANDLE_PARAM(void) uaddr)
+{
+    return hypfs_read_leaf_off(entry, uaddr, NULL);
 }
 
 static int hypfs_read(const struct hypfs_entry *entry,
@@ -587,11 +596,12 @@  int hypfs_write_leaf(struct hypfs_entry_leaf *leaf,
     return ret;
 }
 
-int hypfs_write_bool(struct hypfs_entry_leaf *leaf,
-                     XEN_GUEST_HANDLE_PARAM(const_void) uaddr,
-                     unsigned int ulen)
+static int hypfs_write_bool_off(struct hypfs_entry_leaf *leaf,
+                                XEN_GUEST_HANDLE_PARAM(const_void) uaddr,
+                                unsigned int ulen, void *off)
 {
     bool buf;
+    bool *ptr;
 
     ASSERT(this_cpu(hypfs_locked) == hypfs_write_locked);
     ASSERT(leaf->e.type == XEN_HYPFS_TYPE_BOOL &&
@@ -604,11 +614,19 @@  int hypfs_write_bool(struct hypfs_entry_leaf *leaf,
     if ( copy_from_guest(&buf, uaddr, ulen) )
         return -EFAULT;
 
-    *(bool *)leaf->u.write_ptr = buf;
+    ptr = off ? off + (unsigned long)leaf->u.write_ptr : leaf->u.write_ptr;
+    *ptr = buf;
 
     return 0;
 }
 
+int hypfs_write_bool(struct hypfs_entry_leaf *leaf,
+                     XEN_GUEST_HANDLE_PARAM(const_void) uaddr,
+                     unsigned int ulen)
+{
+    return hypfs_write_bool_off(leaf, uaddr, ulen, NULL);
+}
+
 int hypfs_write_custom(struct hypfs_entry_leaf *leaf,
                        XEN_GUEST_HANDLE_PARAM(const_void) uaddr,
                        unsigned int ulen)
@@ -644,6 +662,27 @@  int hypfs_write_custom(struct hypfs_entry_leaf *leaf,
     return ret;
 }
 
+int hypfs_dyndir_id_read_leaf(const struct hypfs_entry *entry,
+                              XEN_GUEST_HANDLE_PARAM(void) uaddr)
+{
+    struct hypfs_dyndir_id *dyndata;
+
+    dyndata = hypfs_get_dyndata();
+
+    return hypfs_read_leaf_off(entry, uaddr, dyndata->data);
+}
+
+int hypfs_dyndir_id_write_bool(struct hypfs_entry_leaf *leaf,
+                               XEN_GUEST_HANDLE_PARAM(const_void) uaddr,
+                               unsigned int ulen)
+{
+    struct hypfs_dyndir_id *dyndata;
+
+    dyndata = hypfs_get_dyndata();
+
+    return hypfs_write_bool_off(leaf, uaddr, ulen, dyndata->data);
+}
+
 int hypfs_write_deny(struct hypfs_entry_leaf *leaf,
                      XEN_GUEST_HANDLE_PARAM(const_void) uaddr,
                      unsigned int ulen)
diff --git a/xen/include/xen/hypfs.h b/xen/include/xen/hypfs.h
index 34073faff8..670dc68b48 100644
--- a/xen/include/xen/hypfs.h
+++ b/xen/include/xen/hypfs.h
@@ -160,6 +160,8 @@  static inline void hypfs_string_set_reference(struct hypfs_entry_leaf *leaf,
     HYPFS_FIXEDSIZE_INIT(var, XEN_HYPFS_TYPE_BOOL, nam, contvar, \
                          &hypfs_bool_wr_funcs, 1)
 
+#define HYPFS_STRUCT_ELEM(type, elem)    (((type *)NULL)->elem)
+
 extern struct hypfs_entry_dir hypfs_root;
 
 int hypfs_add_dir(struct hypfs_entry_dir *parent,
@@ -204,6 +206,11 @@  struct hypfs_entry *hypfs_gen_dyndir_id_entry(
     const struct hypfs_entry_dir *template, unsigned int id, void *data);
 unsigned int hypfs_dynid_entry_size(const struct hypfs_entry *template,
                                     unsigned int id);
+int hypfs_dyndir_id_read_leaf(const struct hypfs_entry *entry,
+                              XEN_GUEST_HANDLE_PARAM(void) uaddr);
+int hypfs_dyndir_id_write_bool(struct hypfs_entry_leaf *leaf,
+                               XEN_GUEST_HANDLE_PARAM(const_void) uaddr,
+                               unsigned int ulen);
 #endif
 
 #endif /* __XEN_HYPFS_H__ */