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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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-
> extensible.
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;
> >   CHECK_pf_enter_acpi_sleep;
> >   #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_XEN_GUEST_HANDLE(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.

-boris

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