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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.01.14 at 20:08, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote:
> @@ -601,6 +602,23 @@ ret_t 
> do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
>      }
>      break;
>  
> +    case XENPF_get_symbol:
> +    {
> +        char name[XEN_KSYM_NAME_LEN + 1];
> +        XEN_GUEST_HANDLE_64(char) nameh;

Why _64?

> +
> +        guest_from_compat_handle(nameh, op->u.symdata.u.name);
> +
> +        ret = xensyms_read(&op->u.symdata.symnum, &op->u.symdata.type,
> +                           &op->u.symdata.address, name);
> +
> +        if ( !ret && copy_to_guest(nameh, name, XEN_KSYM_NAME_LEN + 1) )

Afaict symbols_expand_symbol() always zero terminates its
output, so I can't see why you're not properly using strlen() here.
The way you do it now you're leaking hypervisor stack contents
to the caller.

> +int xensyms_read(uint32_t *symnum, uint32_t *type, uint64_t *address, char 
> *name)
> +{
> +    if ( *symnum > symbols_num_syms )
> +        return -ERANGE;
> +    if ( *symnum == symbols_num_syms )
> +        return 0;
> +
> +    spin_lock(&symbols_mutex);
> +
> +    if ( *symnum == 0 )
> +        next_offset = next_symbol = 0;
> +    if ( next_symbol != *symnum )
> +        /* Non-sequential access */
> +        next_offset = get_symbol_offset(*symnum);
> +
> +    *type = symbols_get_symbol_type(next_offset);
> +    next_offset = symbols_expand_symbol(next_offset, name);
> +    *address = symbols_offsets[*symnum] + SYMBOLS_ORIGIN;
> +
> +    next_symbol = ++(*symnum);

Pointless parentheses.

> +#define XENPF_get_symbol   61
> +#define XEN_KSYM_NAME_LEN 127
> +struct xenpf_symdata {
> +    /* IN variables */
> +    uint32_t symnum;
> +
> +    /* OUT variables */
> +    uint32_t type;
> +    uint64_t address;
> +
> +    union {
> +        XEN_GUEST_HANDLE(char) name;
> +        uint64_t pad;
> +    } u;

Since you need to do translation anyway, I don't see what good
the padding field (and hence the union) here does.

> --- a/xen/include/xen/symbols.h
> +++ b/xen/include/xen/symbols.h
> @@ -2,8 +2,8 @@
>  #define _XEN_SYMBOLS_H
>  
>  #include <xen/types.h>
> -
> -#define KSYM_NAME_LEN 127
> +#include <public/xen.h>

I don't think you really need this one.

> +#include <public/platform.h>
>  
>  /* Lookup an address. */
>  const char *symbols_lookup(unsigned long addr,
> @@ -11,4 +11,7 @@ const char *symbols_lookup(unsigned long addr,
>                             unsigned long *offset,
>                             char *namebuf);
>  
> +extern int xensyms_read(uint32_t *symnum, uint32_t *type,
> +                        uint64_t *address, char *name);
> +

Please be consistent at least within individual files: There's no
"extern" in the existing function declaration here, so there
shouldn't be one here.

Jan


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