diff mbox

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

Message ID 1421159133-31526-5-git-send-email-julien.grall@linaro.org
State Superseded, archived
Headers show

Commit Message

Julien Grall Jan. 13, 2015, 2:25 p.m. UTC 
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 <julien.grall@linaro.org>
Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>

---
    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

Comments

Julien Grall Jan. 20, 2015, 12:45 p.m. UTC | #1 
Hi Jan,

On 19/01/15 16:51, Jan Beulich wrote:
>>>> On 13.01.15 at 15:25, <julien.grall@linaro.org> wrote:
>> --- /dev/null
>> +++ b/xen/common/guestcopy.c
>> @@ -0,0 +1,30 @@
>> +#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.
>> + */
> 
> 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
diff mbox

Patch

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 0000000..d974f5c
--- /dev/null
+++ b/xen/common/guestcopy.c
@@ -0,0 +1,30 @@ 
+#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)
+{
+    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 <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)
 
+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 <xen/event.h>
 #include <xsm/xsm.h>
 #include <xen/guest_access.h>
+#include <xen/err.h>
 
 #include <public/xsm/flask_op.h>
 
@@ -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_flask_boolean *arg)
 {
     char *name;
-    int rv;
 
     if ( arg->bool_id != -1 )
         return 0;
 
-    rv = flask_copyin_string(arg->name, &name, arg->size, bool_maxstr);
-    if ( rv )
-        return rv;
+    name = safe_copy_string_from_guest(arg->name, arg->size, bool_maxstr);
+    if ( IS_ERR(name) )
+        return PTR_ERR(name);
 
     arg->bool_id = security_find_bool(name);
     arg->size = 0;