[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [RFC 02/19] xen: guestcopy: Provide an helper to copy string from guest

Hi Jan,

On 17/06/14 09:01, Jan Beulich wrote:
On 16.06.14 at 18:17, <julien.grall@xxxxxxxxxx> wrote:

While generally I'm okay with adding such a helper, it should be done
a little more cleanly I think:

--- /dev/null
+++ b/xen/common/guestcopy.c
@@ -0,0 +1,28 @@
+#include <xen/config.h>
+#include <xen/lib.h>
+#include <xen/guest_access.h>
+int copy_string_from_guest(XEN_GUEST_HANDLE(char) u_buf, char **buf,
+                           unsigned long size, unsigned long max_size)
Both of these ought to be size_t (as it was in the flask original).

Hrrmmm... I'm not sure why I made this change. I though the hypercall uses unsigned long but it uses uint32_t.

I will use size_t in the next version.

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

ENOBUFS would seem the better error code here.


+    /* Add an extra +1 to append \0. We can't assume the guest will
+     * provide a valid string */

Now this is the case for flask, but for a generic string copying
routine I don't think this is desirable. It seems especially wrong to
aid the guest with putting a NUL where none was. If you really
want this, I guess you would be better off adding two variants:
One which demands the string to be NUL-terminated (in which
case passing in a size is sort of bogus), and one which takes a
size and inserts a NUL.

A malicious guest could pass a big buffer without a NUL-terminated. If we don't limit the size and check the NUL-terminated character the guest could respectively exhaust Xen memory and exploit it.

Therefore we can't rely on the guest to provide a valid string. This solution will avoid to check in every caller that the string is correctly terminated.


Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.