| Message ID | 1406818852-31856-3-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
>>> On 31.07.14 at 17:00, <julien.grall@linaro.org> wrote: > --- /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. */ "... and adds a NUL-terminator ...". Also not sure about "finished" here. And please fix the comment style. > +int safe_copy_string_from_guest(XEN_GUEST_HANDLE(char) u_buf, char **buf, > + unsigned long size, unsigned long max_size) size_t please. Further I think this function would benefit from using the definitions in xen/err.h, eliminating the need to return the buffer pointer via indirection. Jan
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@linaro.org> > Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov> > Cc: 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> > Cc: Tim Deegan <tim@xen.org> > > --- > 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 >
Hi Jan, On 08/01/2014 09:40 AM, Jan Beulich wrote: >>>> On 31.07.14 at 17:00, <julien.grall@linaro.org> wrote: >> --- /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. */ > > "... and adds a NUL-terminator ...". Also not sure about "finished" > here. And please fix the comment style. Will fix it in the next version. >> +int safe_copy_string_from_guest(XEN_GUEST_HANDLE(char) u_buf, char **buf, >> + unsigned long size, unsigned long max_size) > > size_t please. Will do. > Further I think this function would benefit from using the definitions > in xen/err.h, eliminating the need to return the buffer pointer via > indirection. Good idea. I will use this solution in the next version. Regards,
Hi Stefano, On 08/06/2014 02:56 PM, Stefano Stabellini wrote: >> 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? I don't remember why. I will reuse size_t here. Regards,
On 07/31/2014 11:00 AM, 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@linaro.org> > Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov> > Cc: 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> > Cc: Tim Deegan <tim@xen.org> Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
On Fri, 2014-08-01 at 09:40 +0100, Jan Beulich wrote: > >>> On 31.07.14 at 17:00, <julien.grall@linaro.org> wrote: > > --- /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. */ > > "... and adds a NUL-terminator ...". Also not sure about "finished" > here. It does sound odd. "correctly terminated" would be more usual, but given the context is repetitive. Perhaps: "and add a NULL to make sure that the string is correctly terminated"? Ian.
>>> On 09.09.14 at 14:52, <Ian.Campbell@citrix.com> wrote: > On Fri, 2014-08-01 at 09:40 +0100, Jan Beulich wrote: >> >>> On 31.07.14 at 17:00, <julien.grall@linaro.org> wrote: >> > --- /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. */ >> >> "... and adds a NUL-terminator ...". Also not sure about "finished" >> here. > > It does sound odd. "correctly terminated" would be more usual, but given > the context is repetitive. Perhaps: "and add a NULL to make sure that > the string is correctly terminated"? With s/add/adds/, maybe. Albeit personally I dislike using NULL for other than pointers - I'd prefer NUL or NIL for the purpose here. Jan
On Tue, 2014-09-09 at 14:17 +0100, Jan Beulich wrote: > >>> On 09.09.14 at 14:52, <Ian.Campbell@citrix.com> wrote: > > On Fri, 2014-08-01 at 09:40 +0100, Jan Beulich wrote: > >> >>> On 31.07.14 at 17:00, <julien.grall@linaro.org> wrote: > >> > --- /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. */ > >> > >> "... and adds a NUL-terminator ...". Also not sure about "finished" > >> here. > > > > It does sound odd. "correctly terminated" would be more usual, but given > > the context is repetitive. Perhaps: "and add a NULL to make sure that > > the string is correctly terminated"? > > With s/add/adds/, maybe. Yes. > Albeit personally I dislike using NULL for > other than pointers - I'd prefer NUL or NIL for the purpose here. Me too, that was a typo, sorry! Ian.
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) +{ + 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;
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@linaro.org> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov> Cc: 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> Cc: Tim Deegan <tim@xen.org> --- 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