Re: [PATCH v4 07/33] xen: guestcopy: Provide an helper to safely copy string from guest

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 19/03/15 19:29, Julien Grall wrote:
> Flask code already provides a 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
>     - Add comment to explain the extra +1
>     - Return the buffer directly and use the macros provided by
>     xen/err.h to return an error code if necessary.
>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
>
> ---
>     Changes in v4:
>         - Use -ENOBUFS rather than -ENOENT
>         - Fix coding style in comment
>         - Typoes in commit message
>         - Convert the new flask_copying_string (for DT) in
>         safe_copy_string_from_guest
>         - Add Ian and Daniel's ack
>
>     Changes in v3:
>         - Use macros of xen/err.h to return either the buffer or an
>         error code
>         - Reuse size_t instead of unsigned long
>         - Update comment and commit message
>
>     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         | 31 ++++++++++++++++++++++++++
>  xen/include/xen/guest_access.h |  5 +++++
>  xen/xsm/flask/flask_op.c       | 49 +++++++++++-------------------------------
>  4 files changed, 50 insertions(+), 36 deletions(-)
>  create mode 100644 xen/common/guestcopy.c
>
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index 1956091..cf15887 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..1645cbd
> --- /dev/null
> +++ b/xen/common/guestcopy.c
> @@ -0,0 +1,31 @@
> +#include <xen/config.h>
> +#include <xen/lib.h>
> +#include <xen/guest_access.h>
> +#include <xen/err.h>
> +
> +/*
> + * The function copies a string from the guest and adds a NUL to
> + * make sure the string is correctly terminated.
> + */
> +void *safe_copy_string_from_guest(XEN_GUEST_HANDLE(char) u_buf,
> +                                  size_t size, size_t max_size)

If it is a NUL terminated string, you should return char* rather than void*.

Furthermore, two size parameters serves no useful purpose.  The caller
must always be in a position to decide a plausible upper bound.

> +{
> +    char *tmp;
> +
> +    if ( size > max_size )
> +        return ERR_PTR(-ENOBUFS);
> +
> +    /* Add an extra +1 to append \0 */
> +    tmp = xmalloc_array(char, size + 1);

Need to check that size + 1 doesn't overflow to 0.

> +    if ( !tmp )
> +        return ERR_PTR(-ENOMEM);
> +
> +    if ( copy_from_guest(tmp, u_buf, size) )
> +    {
> +        xfree(tmp);
> +        return ERR_PTR(-EFAULT);
> +    }
> +    tmp[size] = 0;

'\0' please.

~Andrew