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

[Jan Beulich <jbeulich@xxxxxxxx>]

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