Re: [Xen-devel] [PATCH v3 04/24] xen: guestcopy: Provide an helper to safely copy string from guest

2015-02-20 Thread Ian Campbell 
On Tue, 2015-01-13 at 14:25 +, Julien Grall wrote:
> Flask code already provides an helper to copy a string from guest. In a later

"a helper".

> 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 directly the buffer and use the macros provided by

"Return the buffer directly and...".

> xen/err.h to return an error code if necessary.
> 
> Signed-off-by: Julien Grall 
> Cc: Daniel De Graaf 
> Cc: Ian Jackson 
> Cc: Jan Beulich 
> Cc: Keir Fraser 

Acked-by: Ian Campbell 




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

Re: [Xen-devel] [PATCH v3 04/24] xen: guestcopy: Provide an helper to safely copy string from guest

2015-01-20 Thread Jan Beulich 
>>> On 20.01.15 at 13:45,  wrote:
> On 19/01/15 16:51, Jan Beulich wrote:
> On 13.01.15 at 15:25,  wrote:
>>> +void *safe_copy_string_from_guest(XEN_GUEST_HANDLE(char) u_buf,
>>> +  size_t size, size_t max_size)
>> 
>> Is the "safe_" prefix really meaningful? I.e. is this function more
>> safe than e.g. copy_from_guest()?
> 
> It's safe in the sense, the function adds a NUL to make sure the strings
> is correctly terminated.
> 
> On the first v1, you said that name "copy_string_from_guest" doesn't
> match the behavior of the generic helper [1]. So which name do you
> suggest for this function?

Ah, so the safe_ here follows safe_str{cat,cpy}() - that's fine then
I guess.

Jan


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

Re: [Xen-devel] [PATCH v3 04/24] xen: guestcopy: Provide an helper to safely copy string from guest

2015-01-20 Thread Julien Grall 
Hi Jan,

On 19/01/15 16:51, Jan Beulich wrote:
 On 13.01.15 at 15:25,  wrote:
>> --- /dev/null
>> +++ b/xen/common/guestcopy.c
>> @@ -0,0 +1,30 @@
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +/* The function copies a string from the guest and adds a NUL to
>> + * make sure the string is correctly terminated.
>> + */
> 
> Coding style.

Ok.

> 
>> +void *safe_copy_string_from_guest(XEN_GUEST_HANDLE(char) u_buf,
>> +  size_t size, size_t max_size)
> 
> Is the "safe_" prefix really meaningful? I.e. is this function more
> safe than e.g. copy_from_guest()?

It's safe in the sense, the function adds a NUL to make sure the strings
is correctly terminated.

On the first v1, you said that name "copy_string_from_guest" doesn't
match the behavior of the generic helper [1]. So which name do you
suggest for this function?

> 
>> +{
>> +char *tmp;
>> +
>> +if ( size > max_size )
>> +return ERR_PTR(-ENOENT);
> 
> -ENOBUFS?

I will use it.

Regards,

[1] http://lists.xen.org/archives/html/xen-devel/2014-06/msg02361.html

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v3 04/24] xen: guestcopy: Provide an helper to safely copy string from guest

2015-01-19 Thread Jan Beulich 
>>> On 13.01.15 at 15:25,  wrote:
> --- /dev/null
> +++ b/xen/common/guestcopy.c
> @@ -0,0 +1,30 @@
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/* The function copies a string from the guest and adds a NUL to
> + * make sure the string is correctly terminated.
> + */

Coding style.

> +void *safe_copy_string_from_guest(XEN_GUEST_HANDLE(char) u_buf,
> +  size_t size, size_t max_size)

Is the "safe_" prefix really meaningful? I.e. is this function more
safe than e.g. copy_from_guest()?

> +{
> +char *tmp;
> +
> +if ( size > max_size )
> +return ERR_PTR(-ENOENT);

-ENOBUFS?

Jan


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

Re: [Xen-devel] [PATCH v3 04/24] xen: guestcopy: Provide an helper to safely copy string from guest

2015-01-13 Thread Daniel De Graaf 

On 01/13/2015 09:25 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
 - Add comment to explain the extra +1
 - Return directly the buffer 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 

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

[Xen-devel] [PATCH v3 04/24] xen: guestcopy: Provide an helper to safely copy string from guest

2015-01-13 Thread Julien Grall 
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
- Add comment to explain the extra +1
- Return directly the buffer and use the macros provided by
xen/err.h to return an error code if necessary.

Signed-off-by: Julien Grall 
Cc: Daniel De Graaf 
Cc: Ian Jackson 
Cc: Jan Beulich 
Cc: Keir Fraser 

---
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 | 30 +
 xen/include/xen/guest_access.h |  5 +
 xen/xsm/flask/flask_op.c   | 43 ++
 4 files changed, 46 insertions(+), 33 deletions(-)
 create mode 100644 xen/common/guestcopy.c

diff --git a/xen/common/Makefile b/xen/common/Makefile
index 9ce75bb..3da774a 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -10,6 +10,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 000..d974f5c
--- /dev/null
+++ b/xen/common/guestcopy.c
@@ -0,0 +1,30 @@
+#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)
+{
+char *tmp;
+
+if ( size > max_size )
+return ERR_PTR(-ENOENT);
+
+/* Add an extra +1 to append \0 */
+tmp = xmalloc_array(char, size + 1);
+if ( !tmp )
+return ERR_PTR(-ENOMEM);
+
+if ( copy_from_guest(tmp, u_buf, size) )
+{
+xfree(tmp);
+return ERR_PTR(-EFAULT);
+}
+tmp[size] = 0;
+
+return tmp;
+}
diff --git a/xen/include/xen/guest_access.h b/xen/include/xen/guest_access.h
index 373454e..55645e6 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 
+#include 
+#include 
 
 #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)
 
+void *safe_copy_string_from_guest(XEN_GUEST_HANDLE(char) u_buf,
+ size_t size, size_t 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..b14d306 100644
--- a/xen/xsm/flask/flask_op.c
+++ b/xen/xsm/flask/flask_op.c
@@ -12,6 +12,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -76,29 +77,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,9 +90,9 @@ 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);
-if ( rv )
-return rv;
+user = safe_copy_string_from_guest(arg->u.user, arg->size, PAGE_SIZE);
+if ( IS_ERR(user) )
+return PTR_ERR(user);
 
 rv = security_get_user_sids(arg->start_sid, user, &sids, &nsids);
 if ( rv < 0 )
@@ -227,9 +205,9 @@ 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);
-if ( rv )
-return rv;
+buf = safe_copy_string_from_guest(arg->context, arg->size, PAGE_SIZE);
+if ( IS_ERR(buf) )
+return PTR_ERR(buf);
 
 rv = security_context_to_sid(buf, arg->size, &arg->sid);
 if ( rv < 0 )
@@ -319,14 +297,13 @@ static int flask_security_setavc_threshold(struct 
xen_flask_setavc_threshold *ar
 static int flask_security_resolve_bool(struct xen_fla