From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v4 07/33] xen: guestcopy: Provide an helper to safely copy string from guest Date: Tue, 31 Mar 2015 14:24:16 +0100 Message-ID: <551AA000.6090603@citrix.com> References: <1426793399-6283-1-git-send-email-julien.grall@linaro.org> <1426793399-6283-8-git-send-email-julien.grall@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1426793399-6283-8-git-send-email-julien.grall@linaro.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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 > Acked-by: Daniel De Graaf > Acked-by: Ian Campbell > Cc: Ian Jackson > Cc: Jan Beulich > Cc: Keir Fraser > > --- > 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 > +#include > +#include > +#include > + > +/* > + * 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