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

Re: [Xen-devel] [PATCH v6 01/19] common/symbols: Export hypervisor symbols to privileged guest

On 05/16/2014 04:05 AM, Jan Beulich wrote:

+    case XENPF_get_symbol:
+    {
+        char name[XEN_KSYM_NAME_LEN + 1];
This is a fairly large object you place on the stack. Considering that
there's a lock being held already, did you consider making this static?

+        XEN_GUEST_HANDLE(char) nameh;
+        guest_from_compat_handle(nameh, op->u.symdata.name);
+        ret = xensyms_read(&op->u.symdata.symnum, &op->u.symdata.type,
+                           &op->u.symdata.address, name);
+        if ( !ret && copy_to_guest(nameh, name, strlen(name)) )
I think I commented on this earlier on already - you end up calling
strlen() on uninitialized data if xensyms_read() takes its second
early exit path. Additionally there's no way for the caller to
distinguish that success case from other success cases.

" name[0] = '\0' " in xensyms_read() should address both of these concerns.

Considering that xensyms_read() is there only to implement this sub-
hypercall, I wonder whether you wouldn't be better of passing it the
handle, and have it do the copying. It's holding a lock itself, so the
static buffer could be placed there, along with the other statics it
already uses (which btw should go into the only function that make
use of them).

I didn't want to expose xensyms_read() to any handles to keep it a purely "internal" (for the lack of a better term) routine.

But if 'name' is to become static I don't want to introduce another lock (or move existing one up from xensyms_read()) so I guess I would have to pass the handle.

And finally (I think I commented on this too earlier on) you still don't
have the caller pass in the size of the buffer it wants the symbol
copied to, and instead bake into the hypercall interface an arbitrary
(derived from current Xen internals) fixed name length. That's non-

So do you suggest passing in (and out) buffer size?

--- a/xen/arch/x86/x86_64/platform_hypercall.c
+++ b/xen/arch/x86/x86_64/platform_hypercall.c
@@ -32,6 +32,8 @@ CHECK_pf_pcpu_version;
  #undef xen_pf_enter_acpi_sleep
+#define xenpf_symdata compat_pf_symdata
Did you check that you really need this? There's no explicit instance of
the structure, so I would think it's not needed.

Isn't this required for auto-building compat interfaces?

--- a/xen/include/public/platform.h
+++ b/xen/include/public/platform.h
@@ -527,6 +527,21 @@ struct xenpf_core_parking {
  typedef struct xenpf_core_parking xenpf_core_parking_t;
+#define XENPF_get_symbol 61
+#define XEN_KSYM_NAME_LEN 127
+struct xenpf_symdata {
+    /* IN variables */
+    uint32_t symnum;
+    /* OUT variables */
+    uint32_t type;
Do you really need 32 bits here? Looks like this really is a char, i.e.
you could leave 3 bytes for future extensibility.

Yes, this is a char. I can replace it with a padded union.


Xen-devel mailing list



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