Re: [PATCH v5 10/28] xsplice: Implement payload loading

["Jan Beulich" <JBeulich@suse.com>]

>>> On 24.03.16 at 21:00, <konrad.wilk@oracle.com> wrote:
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -100,6 +100,9 @@ unsigned long __read_mostly xen_phys_start;
>  
>  unsigned long __read_mostly xen_virt_end;
>  
> +unsigned long __read_mostly avail_virt_start;
> +unsigned long __read_mostly avail_virt_end;
> +
>  DEFINE_PER_CPU(struct tss_struct, init_tss);
>  
>  char __section(".bss.stack_aligned") cpu0_stack[STACK_SIZE];
> @@ -1211,6 +1214,10 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>                     ~((1UL << L2_PAGETABLE_SHIFT) - 1);
>      destroy_xen_mappings(xen_virt_end, XEN_VIRT_START + BOOTSTRAP_MAP_BASE);
>  
> +    avail_virt_start = xen_virt_end;
> +    avail_virt_end = XEN_VIRT_END - NR_CPUS * PAGE_SIZE;
> +    BUG_ON(avail_virt_end <= avail_virt_start);

Is there a specific reason this needs to be here? I'd prefer the two
symbols above to become static in the file really using them, and
their initialization then be done there too (in an initcall function
perhaps).

> +int arch_xsplice_verify_elf(const struct xsplice_elf *elf, void *data)
> +{
> +
> +    Elf_Ehdr *hdr = data;
> +
> +    if ( !IS_ELF(*hdr) )
> +    {
> +        printk(XENLOG_ERR "%s%s: Not an ELF payload!\n", XSPLICE, elf->name);
> +        return -EINVAL;
> +    }
> +
> +    if ( elf->len < (sizeof *hdr) ||
> +         !IS_ELF(*hdr) ||
> +         hdr->e_ident[EI_CLASS] != ELFCLASS64 ||
> +         hdr->e_ident[EI_DATA] != ELFDATA2LSB ||
> +         hdr->e_ident[EI_OSABI] != ELFOSABI_SYSV ||
> +         hdr->e_machine != EM_X86_64 ||
> +         hdr->e_type != ET_REL ||
> +         hdr->e_phnum != 0 )

Ah, some of the checks missing from the previous patch are here!
But many don't belong here - perhaps everything but the e_machine
check. And even that could be abstracted out so that it can be done
in common code.

> +int arch_xsplice_perform_rel(struct xsplice_elf *elf,
> +                             const struct xsplice_elf_sec *base,
> +                             const struct xsplice_elf_sec *rela)
> +{
> +    dprintk(XENLOG_ERR, "%s%s: SHR_REL relocation unsupported\n",

SHR? DYM SHT?

> +int arch_xsplice_perform_rela(struct xsplice_elf *elf,
> +                              const struct xsplice_elf_sec *base,
> +                              const struct xsplice_elf_sec *rela)
> +{
> +    Elf_RelA *r;

const (and also perhaps better to move down into the for() scope)

> +    unsigned int symndx, i;
> +    uint64_t val;
> +    uint8_t *dest;
> +
> +    if ( !rela->sec->sh_entsize || !rela->sec->sh_size ||
> +         rela->sec->sh_entsize != sizeof(Elf_RelA) )

Needless redundancy and too strict a check again. Also sh_size
should be a multiple of sh_entsize.

> +    {
> +        dprintk(XENLOG_DEBUG, "%s%s: Section relative header is corrupted!\n",

"Section relative"? DYM "Relocation section"? And perhaps the base
section name should be printed as well.

> +                XSPLICE, elf->name);
> +        return -EINVAL;
> +    }
> +
> +    for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
> +    {
> +        r = (Elf_RelA *)(rela->data + i * rela->sec->sh_entsize);

Pointless cast bogusly casting away constness.

> +        if ( (unsigned long)r > (unsigned long)(elf->hdr + elf->len) )

        if ( (unsigned long)(r + 1) > (unsigned long)elf->hdr + elf->len )

> +        {
> +            dprintk(XENLOG_DEBUG, "%s%s: Relative entry %u in %s is past end!\n",
> +                    XSPLICE, elf->name, i, rela->name);
> +            return -EINVAL;
> +        }
> +
> +        symndx = ELF64_R_SYM(r->r_info);
> +        if ( symndx > elf->nsym )

>= (I'm afraid I'll give up pointing out all these off-by-ones - there's
just too many of them. One of you will need to go through all of the
code and audit all the checks to actually be correct.)

> +        {
> +            dprintk(XENLOG_DEBUG, "%s%s: Relative symbol wants symbol@%u which is past end!\n",
> +                    XSPLICE, elf->name, symndx);
> +            return -EINVAL;
> +        }
> +
> +        dest = base->load_addr + r->r_offset;

Missing sanity check on r_offset.

> +        val = r->r_addend + elf->sym[symndx].sym->st_value;
> +
> +        switch ( ELF64_R_TYPE(r->r_info) )
> +        {
> +            case R_X86_64_NONE:

Too deep indentation.

> +                break;
> +
> +            case R_X86_64_64:
> +                *(uint64_t *)dest = val;
> +                break;
> +
> +            case R_X86_64_PLT32:
> +                /*
> +                 * Xen uses -fpic which normally uses PLT relocations
> +                 * except that it sets visibility to hidden which means
> +                 * that they are not used.  However, when gcc cannot
> +                 * inline memcpy it emits memcpy with default visibility
> +                 * which then creates a PLT relocation.  It can just be
> +                 * treated the same as R_X86_64_PC32.
> +                 */
> +                /* Fall through */
> +
> +            case R_X86_64_PC32:

No need for this second comment or the blank line.

> +                *(uint32_t *)dest = val - (uint64_t)dest;

We're dealing with signed quantities here, and the reduction in
width requires checking that no truncation occurs.

> +                break;
> +
> +            default:
> +                printk(XENLOG_ERR "%s%s: Unhandled relocation %lu\n",
> +                       XSPLICE, elf->name, ELF64_R_TYPE(r->r_info));

Some rate limiting mechanism needs to be used here.

> +static find_space_t *find_space_fnc = NULL;

Pointless initializer.

> +void arch_xsplice_register_find_space(find_space_t *cb)
> +{
> +    ASSERT(!find_space_fnc);
> +
> +    find_space_fnc = cb;
> +}
> +
> +static void* xsplice_map_rwx(const mfn_t *mfn, unsigned int pages)

Misplaced *.

> +{
> +    unsigned long cur;
> +    unsigned long start, end;
> +
> +    start = (unsigned long)avail_virt_start;
> +    end = start + pages * PAGE_SIZE;
> +
> +    ASSERT(find_space_fnc);

I don't think this is a good idea: If not set, we'll immediately have a
security issue. Either explicitly return NULL if this is NULL, or make
it point to a dummy (returning an error) until registered.

> +    if ( find_space_fnc(pages, &start, &end) )

Why both start and end? The latter should be derivable from start
and pages.

> +        return NULL;
> +
> +    if ( end >= avail_virt_end )
> +        return NULL;
> +
> +    for ( cur = start; pages--; ++mfn, cur += PAGE_SIZE )
> +    {
> +        /*
> +         * We would like to to RX, but we need to copy data in it first.
> +         * See arch_xsplice_secure for how we lockdown.
> +         */
> +        if ( map_pages_to_xen(cur, mfn_x(*mfn), 1, PAGE_HYPERVISOR_RWX) )
> +        {
> +            if ( cur != start )
> +                destroy_xen_mappings(start, cur);
> +            return NULL;
> +        }
> +    }
> +
> +    return (void*)start;

Missing blank.

> +int arch_xsplice_secure(void *va, unsigned int pages, enum va_type type,
> +                        const mfn_t *mfn)

This and other functions around here: Why arch_*? What prevents
them being put in common code?

> @@ -28,6 +29,15 @@ struct payload {
>      uint32_t state;                      /* One of the XSPLICE_STATE_*. */
>      int32_t rc;                          /* 0 or -XEN_EXX. */
>      struct list_head list;               /* Linked to 'payload_list'. */
> +    void *text_addr;                     /* Virtual address of .text. */
> +    size_t text_size;                    /* .. and its size. */
> +    void *rw_addr;                       /* Virtual address of .data. */
> +    size_t rw_size;                      /* .. and its size (if any). */
> +    void *ro_addr;                       /* Virtual address of .rodata. */
> +    size_t ro_size;                      /* .. and its size (if any). */
> +    size_t payload_pages;                /* Nr of the pages for the text_addr;
> +                                            rw_addr, and ro_addr (if any) */
> +    mfn_t *mfn;                          /* Array of MFNs of the pages. */

const (several times)?

> +static void calc_section(struct xsplice_elf_sec *sec, size_t *size)
> +{
> +    size_t align_size = ROUNDUP(*size, sec->sec->sh_addralign);
> +
> +    sec->sec->sh_entsize = align_size;
> +    *size = sec->sec->sh_size + align_size;
> +}
> +
> +static int find_hole(size_t pages, unsigned long *hole_start,
> +                     unsigned long *hole_end)
> +{
> +    struct payload *data, *data2;
> +
> +    spin_lock_recursive(&payload_lock);
> +    list_for_each_entry ( data, &payload_list, list )
> +    {
> +        list_for_each_entry ( data2, &payload_list, list )
> +        {
> +            unsigned long start, end;
> +
> +            start = (unsigned long)data2->text_addr;
> +            end = start + data2->payload_pages * PAGE_SIZE;
> +            if ( *hole_end > start && *hole_start < end )
> +            {
> +                *hole_start = end;
> +                *hole_end = end + pages * PAGE_SIZE;
> +                break;
> +            }
> +        }
> +        if ( &data2->list == &payload_list )
> +            break;
> +    }
> +    spin_unlock_recursive(&payload_lock);
> +
> +    return 0;
> +}

How will the caller know you didn't find a hole? If via the value
pointed to by the two function arguments, what use is the return
value?

Also - how well will this O(n^2) lookup work once there are enough
payloads? I think this calls for the alternative vmap() extension I've
been suggesting earlier.

> +static int move_payload(struct payload *payload, struct xsplice_elf *elf)
> +{
> +    uint8_t *buf;
> +    unsigned int i;
> +    size_t size = 0;
> +
> +    /* Compute text regions. */
> +    for ( i = 0; i < elf->hdr->e_shnum; i++ )
> +    {
> +        if ( (elf->sec[i].sec->sh_flags & (SHF_ALLOC|SHF_EXECINSTR)) ==
> +             (SHF_ALLOC|SHF_EXECINSTR) )
> +            calc_section(&elf->sec[i], &payload->text_size);
> +    }
> +
> +    /* Compute rw data. */
> +    for ( i = 0; i < elf->hdr->e_shnum; i++ )
> +    {
> +        if ( (elf->sec[i].sec->sh_flags & SHF_ALLOC) &&
> +             !(elf->sec[i].sec->sh_flags & SHF_EXECINSTR) &&
> +             (elf->sec[i].sec->sh_flags & SHF_WRITE) )
> +            calc_section(&elf->sec[i], &payload->rw_size);
> +    }
> +
> +    /* Compute ro data. */
> +    for ( i = 0; i < elf->hdr->e_shnum; i++ )
> +    {
> +        if ( (elf->sec[i].sec->sh_flags & SHF_ALLOC) &&
> +             !(elf->sec[i].sec->sh_flags & SHF_EXECINSTR) &&
> +             !(elf->sec[i].sec->sh_flags & SHF_WRITE) )
> +            calc_section(&elf->sec[i], &payload->ro_size);
> +    }

So you handle X, W, and r/o, but you do nothing for WX. If you
don#t want to allow for this, you need to error out if there's any
such section.

> +    /*
> +     * Total of all three regions - RX, RW, and RO. We have to have
> +     * keep them in seperate pages so we PAGE_ALIGN the RX and RW to have
> +     * them on seperate pages. The last one will by default fall on its
> +     * own page.
> +     */
> +     size = PAGE_ALIGN(payload->text_size) + PAGE_ALIGN(payload->rw_size) +
> +            payload->ro_size;
> +
> +    size = PFN_UP(size);
> +    buf = arch_xsplice_alloc_payload(size, &payload->mfn);
> +    if ( !buf ) {

Coding style.

> +        printk(XENLOG_ERR "%s%s: Could not allocate memory for payload!\n",
> +               XSPLICE, elf->name);
> +        return -ENOMEM;
> +    }
> +
> +    payload->payload_pages = size;
> +    payload->text_addr = buf;
> +    payload->rw_addr = payload->text_addr + PAGE_ALIGN(payload->text_size);
> +    payload->ro_addr = payload->rw_addr + PAGE_ALIGN(payload->rw_size);
> +
> +    for ( i = 0; i < elf->hdr->e_shnum; i++ )
> +    {
> +        if ( elf->sec[i].sec->sh_flags & SHF_ALLOC )
> +        {
> +            if ( (elf->sec[i].sec->sh_flags & SHF_EXECINSTR) )
> +                 buf = payload->text_addr;
> +            else if ( (elf->sec[i].sec->sh_flags & SHF_WRITE) )
> +                buf = payload->rw_addr;
> +             else
> +                buf = payload->ro_addr;
> +
> +            elf->sec[i].load_addr = buf + elf->sec[i].sec->sh_entsize;
> +
> +            /* Don't copy NOBITS - such as BSS. */

Not copying is correct, but you need to zero these.

> +            if ( elf->sec[i].sec->sh_type != SHT_NOBITS )
> +            {
> +                memcpy(elf->sec[i].load_addr, elf->sec[i].data,
> +                       elf->sec[i].sec->sh_size);
> +                dprintk(XENLOG_DEBUG, "%s%s: Loaded %s at 0x%p\n", XSPLICE,

0x%p is bogus.

> +static int secure_payload(struct payload *payload, struct xsplice_elf *elf)
> +{
> +    int rc;
> +    unsigned int text_pages, rw_pages, ro_pages;
> +
> +    ASSERT(payload->mfn);
> +
> +    text_pages = PFN_UP(payload->text_size);
> +    ASSERT(text_pages);

Where is the earlier check that allows you to ASSERT() here? I
anyway wonder whether requiring non-empty .text is really
necessary / useful.

> +static int load_payload_data(struct payload *payload, void *raw, ssize_t len)
> +{
> +    struct xsplice_elf elf;
> +    int rc = 0;
> +
> +    memset(&elf, 0, sizeof(elf));
> +    elf.name = payload->name;
> +    elf.len = len;

Perhaps better done via initializer?

> @@ -382,8 +635,9 @@ static void xsplice_printall(unsigned char key)
>      }
>  
>      list_for_each_entry ( data, &payload_list, list )
> -        printk(" name=%s state=%s(%d)\n", data->name,
> -               state2str(data->state), data->state);
> +        printk(" name=%s state=%s(%d) %p (.data=%p, .rodata=%p) using %zu pages.\n",

No full stop at the end of log messages please.

> --- a/xen/common/xsplice_elf.c
> +++ b/xen/common/xsplice_elf.c
> @@ -213,6 +213,97 @@ static int elf_get_sym(struct xsplice_elf *elf, const 
> void *data)
>      return 0;
>  }
>  
> +int xsplice_elf_resolve_symbols(struct xsplice_elf *elf)
> +{
> +    unsigned int i;
> +
> +    /*
> +     * The first entry of an ELF symbol table is the "undefined symbol index".
> +     * aka reserved so we skip it.
> +     */
> +    ASSERT( elf->sym );

Stray blanks.

> +    for ( i = 1; i < elf->nsym; i++ )
> +    {
> +        uint16_t idx = elf->sym[i].sym->st_shndx;
> +
> +        switch ( idx )
> +        {
> +            case SHN_COMMON:

Too deep indentation.

> +                printk(XENLOG_ERR "%s%s: Unexpected common symbol: %s\n",
> +                       XSPLICE, elf->name, elf->sym[i].name);
> +                return -EINVAL;
> +                break;
> +
> +            case SHN_UNDEF:
> +                printk(XENLOG_ERR "%s%s: Unknown symbol: %s\n",
> +                       XSPLICE, elf->name, elf->sym[i].name);
> +                return -ENOENT;
> +                break;
> +
> +            case SHN_ABS:
> +                dprintk(XENLOG_DEBUG, "%s%s: Absolute symbol: %s => 0x%"PRIx64"\n",

%# instead of 0x% please.

> +                      XSPLICE, elf->name, elf->sym[i].name,
> +                      elf->sym[i].sym->st_value);
> +                break;
> +
> +            default:
> +                if ( elf->sec[idx].sec->sh_flags & SHF_ALLOC )

What if idx is beyond the range of valid section index, namely
another of the unhandled values in the reserved range?

> +                {
> +                    elf->sym[i].sym->st_value +=
> +                        (unsigned long)elf->sec[idx].load_addr;
> +                    if ( elf->sym[i].name )
> +                        printk(XENLOG_DEBUG "%s%s: Symbol resolved: %s => 0x%"PRIx64"(%s)\n",
> +                               XSPLICE, elf->name, elf->sym[i].name,
> +                               (uint64_t)elf->sym[i].sym->st_value,

Instead of casts like this, please introduce ELF-specific format macros.

> +int xsplice_elf_perform_relocs(struct xsplice_elf *elf)
> +{
> +    struct xsplice_elf_sec *rela, *base;
> +    unsigned int i;
> +    int rc;
> +
> +    /*
> +     * The first entry of an ELF symbol table is the "undefined symbol index".
> +     * aka reserved so we skip it.
> +     */
> +    ASSERT( elf->sym );
> +    for ( i = 1; i < elf->hdr->e_shnum; i++ )
> +    {
> +        rela = &elf->sec[i];
> +
> +        if ( (rela->sec->sh_type != SHT_RELA ) &&
> +             (rela->sec->sh_type != SHT_REL ) )

Stray blanks.

> +            continue;
> +
> +         /* Is it a valid relocation section? */
> +         if ( rela->sec->sh_info >= elf->hdr->e_shnum )
> +            continue;
> +
> +         base = &elf->sec[rela->sec->sh_info];
> +
> +         /* Don't relocate non-allocated sections. */
> +         if ( !(base->sec->sh_flags & SHF_ALLOC) )
> +            continue;
> +
> +        if ( elf->sec[i].sec->sh_type == SHT_RELA )
> +            rc = arch_xsplice_perform_rela(elf, base, rela);
> +        else /* SHT_REL */
> +            rc = arch_xsplice_perform_rel(elf, base, rela);

Please at least validate sh_link somewhere above.

> --- a/xen/include/xen/xsplice_elf.h
> +++ b/xen/include/xen/xsplice_elf.h
> @@ -15,6 +15,8 @@ struct xsplice_elf_sec {
>                                        elf_resolve_section_names. */
>      const void *data;              /* Pointer to the section (done by
>                                        elf_resolve_sections). */
> +    uint8_t *load_addr;            /* A pointer to the allocated destination.
> +                                      Done by load_payload_data. */

void perhaps? And const?

> @@ -38,6 +40,9 @@ struct xsplice_elf_sec *xsplice_elf_sec_by_name(const struct xsplice_elf *elf,
>  int xsplice_elf_load(struct xsplice_elf *elf, void *data);
>  void xsplice_elf_free(struct xsplice_elf *elf);
>  
> +int xsplice_elf_resolve_symbols(struct xsplice_elf *elf);
> +int xsplice_elf_perform_relocs(struct xsplice_elf *elf);

const?

Jan

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