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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.05.14 at 17:53, <boris.ostrovsky@xxxxxxxxxx> wrote:
> Export Xen's symbols as {<address><type><name>} triplet via new 
> XENPF_get_symbol
> hypercall

I already voiced my reservations on a very early version of this series.
While I can see the need of exposing these internals, I also see the
potential for abuse. I'd clearly want at least one other common code
maintainer's opinion here; sadly you didn't properly Cc them all (done
now).

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

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

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.

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

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

Also I think in a comment you should specify here how the loop
termination is to be recognized - after all that's part of the ABI
you introduce.

Jan


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