Re: [Xen-devel] [PATCH v2 02/21] xen: guestcopy: Provide an helper to safely copy string from guest

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Thu, 31 Jul 2014, Julien Grall wrote:
> Flask code already provides an helper to copy a string from guest. In a later
> patch, the new DT hypercalls will need a similar function.
> 
> To avoid code duplication, copy the flask helper (flask_copying_string) to
> common code:
>     - Rename into safe_copy_string_from_guest
>     - Update arguments. The size provided by the hypercall is unsigned
>     not signed
>     - Add comment to explain the extra +1
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> Cc: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
> Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Keir Fraser <keir@xxxxxxx>
> Cc: Tim Deegan <tim@xxxxxxx>
> 
> ---
>     Changes in v2:
>         - Rename copy_string_from_guest into safe_copy_string_from_guest
>         - Update commit message and comment in the code
> ---
>  xen/common/Makefile            |    1 +
>  xen/common/guestcopy.c         |   29 +++++++++++++++++++++++++++++
>  xen/include/xen/guest_access.h |    5 +++++
>  xen/xsm/flask/flask_op.c       |   29 +++--------------------------
>  4 files changed, 38 insertions(+), 26 deletions(-)
>  create mode 100644 xen/common/guestcopy.c
> 
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index 3683ae3..627b6e3 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -9,6 +9,7 @@ obj-y += event_2l.o
>  obj-y += event_channel.o
>  obj-y += event_fifo.o
>  obj-y += grant_table.o
> +obj-y += guestcopy.o
>  obj-y += irq.o
>  obj-y += kernel.o
>  obj-y += keyhandler.o
> diff --git a/xen/common/guestcopy.c b/xen/common/guestcopy.c
> new file mode 100644
> index 0000000..c01555c
> --- /dev/null
> +++ b/xen/common/guestcopy.c
> @@ -0,0 +1,29 @@
> +#include <xen/config.h>
> +#include <xen/lib.h>
> +#include <xen/guest_access.h>
> +
> +/* The function copies a string from the guest and add a NUL-terminated to
> + * make sure the string is correctly finished. */
> +int safe_copy_string_from_guest(XEN_GUEST_HANDLE(char) u_buf, char **buf,
> +                                unsigned long size, unsigned long max_size)

Why did you change the size type from size_t to unsigned long?
If you really need to move away from size_t, wouldn't it be better to
still be using an explicitly sized type, such as uint32_t or uint64_t?



> +{
> +    char *tmp;
> +
> +    if ( size > max_size )
> +        return -ENOENT;
> +
> +    /* Add an extra +1 to append \0 */
> +    tmp = xmalloc_array(char, size + 1);
> +    if ( !tmp )
> +        return -ENOMEM;
> +
> +    if ( copy_from_guest(tmp, u_buf, size) )
> +    {
> +        xfree(tmp);
> +        return -EFAULT;
> +    }
> +    tmp[size] = 0;
> +
> +    *buf = tmp;
> +    return 0;
> +}
> diff --git a/xen/include/xen/guest_access.h b/xen/include/xen/guest_access.h
> index 373454e..4c669ad 100644
> --- a/xen/include/xen/guest_access.h
> +++ b/xen/include/xen/guest_access.h
> @@ -8,6 +8,8 @@
>  #define __XEN_GUEST_ACCESS_H__
>  
>  #include <asm/guest_access.h>
> +#include <xen/types.h>
> +#include <public/xen.h>
>  
>  #define copy_to_guest(hnd, ptr, nr)                     \
>      copy_to_guest_offset(hnd, 0, ptr, nr)
> @@ -27,4 +29,7 @@
>  #define __clear_guest(hnd, nr)                          \
>      __clear_guest_offset(hnd, 0, nr)
>  
> +int safe_copy_string_from_guest(XEN_GUEST_HANDLE(char) u_buf, char **buf,
> +                                unsigned long size, unsigned long max_size);
> +
>  #endif /* __XEN_GUEST_ACCESS_H__ */
> diff --git a/xen/xsm/flask/flask_op.c b/xen/xsm/flask/flask_op.c
> index 7743aac..840a635 100644
> --- a/xen/xsm/flask/flask_op.c
> +++ b/xen/xsm/flask/flask_op.c
> @@ -76,29 +76,6 @@ static int domain_has_security(struct domain *d, u32 perms)
>                          perms, NULL);
>  }
>  
> -static int flask_copyin_string(XEN_GUEST_HANDLE(char) u_buf, char **buf,
> -                               size_t size, size_t max_size)
> -{
> -    char *tmp;
> -
> -    if ( size > max_size )
> -        return -ENOENT;
> -
> -    tmp = xmalloc_array(char, size + 1);
> -    if ( !tmp )
> -        return -ENOMEM;
> -
> -    if ( copy_from_guest(tmp, u_buf, size) )
> -    {
> -        xfree(tmp);
> -        return -EFAULT;
> -    }
> -    tmp[size] = 0;
> -
> -    *buf = tmp;
> -    return 0;
> -}
> -
>  #endif /* COMPAT */
>  
>  static int flask_security_user(struct xen_flask_userlist *arg)
> @@ -112,7 +89,7 @@ static int flask_security_user(struct xen_flask_userlist 
> *arg)
>      if ( rv )
>          return rv;
>  
> -    rv = flask_copyin_string(arg->u.user, &user, arg->size, PAGE_SIZE);
> +    rv = safe_copy_string_from_guest(arg->u.user, &user, arg->size, 
> PAGE_SIZE);
>      if ( rv )
>          return rv;
>  
> @@ -227,7 +204,7 @@ static int flask_security_context(struct 
> xen_flask_sid_context *arg)
>      if ( rv )
>          return rv;
>  
> -    rv = flask_copyin_string(arg->context, &buf, arg->size, PAGE_SIZE);
> +    rv = safe_copy_string_from_guest(arg->context, &buf, arg->size, 
> PAGE_SIZE);
>      if ( rv )
>          return rv;
>  
> @@ -324,7 +301,7 @@ static int flask_security_resolve_bool(struct 
> xen_flask_boolean *arg)
>      if ( arg->bool_id != -1 )
>          return 0;
>  
> -    rv = flask_copyin_string(arg->name, &name, arg->size, bool_maxstr);
> +    rv = safe_copy_string_from_guest(arg->name, &name, arg->size, 
> bool_maxstr);
>      if ( rv )
>          return rv;
>  
> -- 
> 1.7.10.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel