Re: [Xen-devel] [PATCH v2 01/13] Export hypervisor symbols

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.09.13 at 11:42, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote:
> +    case XENPF_get_symbols:
> +    {
> +        ret = xensyms_read(&op->u.symdata);
> +        if ( ret >= 0 && __copy_field_to_guest(u_xenpf_op, op, u.symdata) )
> +            ret = -EFAULT;
> +    }
> +    break;

This yields a positive return value if a symbol was found, 0 if none
was found, and negative on error. Can we avoid this non-standard
first aspect?

> --- a/xen/arch/x86/x86_64/platform_hypercall.c
> +++ b/xen/arch/x86/x86_64/platform_hypercall.c
> @@ -35,7 +35,7 @@ CHECK_pf_pcpu_version;
>  #undef xen_pf_pcpu_version
>  
>  #define xenpf_enter_acpi_sleep compat_pf_enter_acpi_sleep
> -

Please retain this blank line.

> +#define xenpf_symdata        compat_pf_symdata

This needs to be accompanied by an entry in xen/include/xlat.lst,
and a respective CHECK_ invocation. I admit that the line
immediately above is incomplete too, and hence served as a bad
example (I'm preparing a fix as I write this).

> +static uint64_t next_symbol, next_offset;
> +static DEFINE_SPINLOCK(symbols_mutex);
> +
> +int xensyms_read(struct xenpf_symdata *symdata)
> +{
> +    if ( symdata->xen_symnum > symbols_num_syms )
> +        return -EINVAL;

This should be a more specific error code (-ERANGE perhaps).

> +    else if ( symdata->xen_symnum == symbols_num_syms )

Pointless "else".

> +        return 0;
> +
> +    spin_lock(&symbols_mutex);
> +
> +    if ( symdata->xen_symnum == 0 )
> +        next_offset = next_symbol = 0;
> +    else if ( next_symbol != symdata->xen_symnum )
> +        /* Non-sequential access */
> +        next_offset = get_symbol_offset(symdata->xen_symnum);
> +
> +    symdata->type = symbols_get_symbol_type(next_offset);
> +    next_offset = symbols_expand_symbol(next_offset, symdata->name,
> +        sizeof(symdata->name));
> +    symdata->address = symbols_offsets[symdata->xen_symnum] + SYMBOLS_ORIGIN;
> +
> +    next_symbol = symdata->xen_symnum + 1;
> +
> +    spin_unlock(&symbols_mutex);
> +
> +    return strlen(symdata->name);

Altogether the changes you do appear to allow the nul terminator
to be written outside of the passed in array. Hence (a) you need
to avoid corrupting memory and (b) whether using strlen() here is
appropriate depends on how you deal with (a).

> --- a/xen/include/public/platform.h
> +++ b/xen/include/public/platform.h
> @@ -527,6 +527,27 @@ struct xenpf_core_parking {
>  typedef struct xenpf_core_parking xenpf_core_parking_t;
>  DEFINE_XEN_GUEST_HANDLE(xenpf_core_parking_t);
>  
> +#define XENPF_get_symbols  61

Now that you retrieve them one at a time, perhaps better
XENPF_get_symbol?

> +
> +struct xenpf_symdata {
> +    /* IN variables */
> +    uint64_t xen_symnum;
> +
> +    /* OUT variables */
> +    uint64_t address;
> +    uint64_t type;

"type" and "xen_symnum" could easily be less than 64 bits wide.

Also, what's the point of the "xen_" prefix in the latter?

> +    /*
> +     * KSYM_NAME_LEN is 128 bytes. However, we cannot be larger than pad in
> +     * xen_platform_op below (which is 128 bytes as well). Since the largest
> +     * symbol is around 50 bytes it's probably more trouble than it's worth
> +     * to try to deal with symbols that are close to 128 bytes in length.
> +     */
> +#define XEN_SYMS_MAX_LEN (128 - 3 * 8)
> +    char name[XEN_SYMS_MAX_LEN];

No, this ought to be a handle pointing to a char array. Symbols
alone might be only around 50 bytes, but the moment we get to
the point of prefixing static symbols with their file names this will
break.

Jan


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