Re: [Xen-devel] [PATCH v9 11/27] xsplice: Implement payload loading

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.04.16 at 17:34, <konrad.wilk@xxxxxxxxxx> wrote:
> From: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> 
> Add support for loading xsplice payloads. This is somewhat similar to
> the Linux kernel module loader, implementing the following steps:
> - Verify the elf file.
> - Parse the elf file.
> - Allocate a region of memory mapped within a free area of
>   [xen_virt_end, XEN_VIRT_END].
> - Copy allocated sections into the new region. Split them in three
>   regions - .text, .data, and .rodata. MUST have at least .text.
> - Resolve section symbols. All other symbols must be absolute addresses.
>   (Note that patch titled "xsplice,symbols: Implement symbol name resolution
>    on address" implements that)
> - Perform relocations.
> - Secure the the regions (.text,.data,.rodata) with proper permissions.
> 
> We capitalize on the vmalloc callback API (see patch titled:
> "rm/x86/vmap: Add v[z|m]alloc_xen, and vm_init_type") to allocate
> a region of memory within the [xen_virt_end, XEN_VIRT_END] for the code.
> 
> We also use the "x86/mm: Introduce modify_xen_mappings()"
> to change the virtual address page-table permissions.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Acked-by: Julien Grall <julien.grall@xxxxxxx>

By now I guess you can guess what I think about the above vs ...

> v9:
>   - Rebase on different spinlock usage in xsplice_upload.
>   - Do proper bound and overflow checking.
>   - Added 'const' on [text,ro,rw]_addr.
>   - Made 'calc_section' and 'move_payload' use an dynamically
>     allocated array for computed offsets instead of modifying sh_entsize.
>   - Remove arch_xsplice_[alloc_payload|free] and use vzalloc_xen and
>     vfree.
>   - Collapse for loop in move_payload.
>   - Move xsplice.o in Makefile
>   - Add more checks in arch_xsplice_perform_rela (r_offset and
>      sh_size % sh_entsize)
>   - Use int32_t and int64_t in arch_xsplice_perform_rela.
>   - Tighten the list of sh_flags we check
>   - Use intermediate on 'buf' so that we can do 'const void *'
>   - Use intermediate in xsplice_elf_resolve_symbols for 'const' of elf->sym.
>   - Fail if (and only) SHF_ALLOC and SHT_NOBITS section is seen.

... this long list.

> +int arch_xsplice_perform_rela(struct xsplice_elf *elf,
> +                              const struct xsplice_elf_sec *base,
> +                              const struct xsplice_elf_sec *rela)
> +{
> +    const Elf_RelA *r;
> +    unsigned int symndx, i;
> +    uint64_t val;
> +    uint8_t *dest;
> +
> +    /* Nothing to do. */
> +    if ( !rela->sec->sh_size )
> +        return 0;
> +
> +    if ( !rela->sec->sh_entsize ||
> +         rela->sec->sh_entsize < sizeof(Elf_RelA) ||

Same thing here as in v8.1 in another patch: The first check is
pointless now that you have the second one.

> +         rela->sec->sh_size % rela->sec->sh_entsize )
> +    {
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Section relative header is 
> corrupted!\n",
> +                elf->name);
> +        return -EINVAL;
> +    }
> +
> +    for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
> +    {
> +        r = rela->data + i * rela->sec->sh_entsize;
> +
> +        symndx = ELF64_R_SYM(r->r_info);
> +
> +        if ( symndx > elf->nsym )
> +        {
> +            dprintk(XENLOG_ERR, XSPLICE "%s: Relative relocation wants 
> symbol@%u which is past end!\n",
> +                    elf->name, symndx);
> +            return -EINVAL;
> +        }
> +
> +        if ( r->r_offset > base->sec->sh_size )

>= at the very least. The size of the relocated location would really
also need to be taken into account, but that can only be done in the
switch below.

> +void arch_xsplice_init(void)

__init

> +static void free_payload_data(struct payload *payload)
> +{
> +    /* Set to zero until "move_payload". */
> +    if ( !payload->text_addr )
> +        return;
> +
> +    vfree((void *)payload->text_addr);
> +
> +    payload->pages = 0;

I think what you check and what you clear should match up, such that
redundant invocation of the function wouldn't lead to problems.

> +static int move_payload(struct payload *payload, struct xsplice_elf *elf)
> +{
> +    uint8_t *text_buf, *ro_buf, *rw_buf;

Any particular reason for them not being void *?

> +    unsigned int i;
> +    size_t size = 0;
> +    unsigned int *offset;
> +    int rc = 0;
> +
> +    offset = xzalloc_array(unsigned int, elf->hdr->e_shnum);

Why not xmalloc_array()?

> +    if ( !offset )
> +        return -ENOMEM;
> +
> +    /* Compute size of different regions. */
> +    for ( i = 1; 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, &offset[i]);

This silently accepts writable text sections, yet the portion of the
memory this gets placed in will be mapped RX.

> +        else 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, &offset[i]);
> +        else 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, &offset[i]);
> +        else if ( !elf->sec[i].sec->sh_flags ||
> +                  (elf->sec[i].sec->sh_flags & SHF_EXECINSTR) ||
> +                  (elf->sec[i].sec->sh_flags & SHF_MASKPROC) )
> +            /* Do nothing.*/;
> +        else if ( (elf->sec[i].sec->sh_flags & SHF_ALLOC) &&
> +                  (elf->sec[i].sec->sh_type == SHT_NOBITS) )
> +        {
> +            dprintk(XENLOG_DEBUG, XSPLICE "%s: Not supporting %s section!\n",
> +                    elf->name, elf->sec[i].name);
> +            rc = -EOPNOTSUPP;
> +            goto out;
> +        }

I saw this in the changelog, but I don't really understand these last
two conditionals. Wouldn't you want to bail on _any_ sections which
have SHF_ALLOC set but don't get mapped to one of the three
blocks? And wouldn't you (silently) ignore any sections with SHF_ALLOC
clear?

> +        else /* Such as .comment. */
> +            dprintk(XENLOG_DEBUG, XSPLICE "%s: Ignoring %s section!\n",
> +                    elf->name, elf->sec[i].name);
> +    }
> +
> +    /*
> +     * 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); /* Nr of pages. */
> +    text_buf = vzalloc_xen(size * PAGE_SIZE);
> +    if ( !text_buf )
> +    {
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Could not allocate memory for 
> payload!\n",
> +                elf->name);
> +        rc = -ENOMEM;
> +        goto out;
> +    }
> +    rw_buf = text_buf +  + PAGE_ALIGN(payload->text_size);
> +    ro_buf = rw_buf + PAGE_ALIGN(payload->rw_size);
> +
> +    payload->pages = size;
> +    payload->text_addr = text_buf;
> +    payload->rw_addr = rw_buf;
> +    payload->ro_addr = ro_buf;
> +
> +    for ( i = 1; i < elf->hdr->e_shnum; i++ )
> +    {
> +        if ( elf->sec[i].sec->sh_flags & SHF_ALLOC )
> +        {
> +            uint8_t *buf;

Perhaps void * again? And missing a blank line afterwards.

> +            if ( (elf->sec[i].sec->sh_flags & SHF_EXECINSTR) )
> +                buf = text_buf;
> +            else if ( (elf->sec[i].sec->sh_flags & SHF_WRITE) )
> +                buf = rw_buf;
> +             else

The indentation here is still one off.

> +                buf = ro_buf;
> +
> +            elf->sec[i].load_addr = buf + offset[i];
> +
> +            /*
> +             * Don't copy NOBITS - such as BSS. We don't memset BSS as
> +             * arch_xsplice_alloc_payload has zeroed it out for us.

Stale comment - the mentioned function doesn't exist anymore.
(Same elsewhere further down in xsplice.h.) And I really think
using memset() here would be better than using vzalloc_xen()
above. Or is there a particular reason to zero the whole area,
just to overwrite almost everything of it again right afterwards?

> +int xsplice_elf_resolve_symbols(struct xsplice_elf *elf)
> +{
> +    unsigned int i;
> +    int rc = 0;
> +
> +    ASSERT(elf->sym);
> +
> +    for ( i = 1; i < elf->nsym; i++ )
> +    {
> +        unsigned int idx = elf->sym[i].sym->st_shndx;
> +        Elf_Sym *sym = (Elf_Sym *)elf->sym[i].sym;

Well, I admit that this is the more straightforward solution, but it
opens up all of what sym points to for writing. I.e. I'd have
considered it much better to really only do the casting away of
const in the one spot where you need it (see below).

> +        switch ( idx )
> +        {
> +        case SHN_COMMON:
> +            dprintk(XENLOG_ERR, XSPLICE "%s: Unexpected common symbol: %s\n",
> +                    elf->name, elf->sym[i].name);
> +            rc = -EINVAL;
> +            break;
> +
> +        case SHN_UNDEF:
> +            dprintk(XENLOG_ERR, XSPLICE "%s: Unknown symbol: %s\n",
> +                    elf->name, elf->sym[i].name);
> +            rc = -ENOENT;
> +            break;
> +
> +        case SHN_ABS:
> +            dprintk(XENLOG_DEBUG, XSPLICE "%s: Absolute symbol: %s => 
> %#"PRIxElfAddr"\n",
> +                    elf->name, elf->sym[i].name, sym->st_value);
> +            break;
> +
> +        default:
> +            if ( idx < elf->hdr->e_shnum &&
> +                 !(elf->sec[idx].sec->sh_flags & SHF_ALLOC) )
> +                break;

I'm afraid I got confused on v8.1 and misguided you here. For some
reason I thought the check originally sat past the switch() statement,
which obviously it can't. With the now redundant idx range check it
is pretty clear that should really have remained where it was.

> +            /* SHN_COMMON and SHN_ABS are above. */
> +            if ( idx >= SHN_LORESERVE )
> +                rc = -EOPNOTSUPP;
> +            else if ( idx >= elf->hdr->e_shnum )
> +                rc = -EINVAL;
> +
> +            if ( rc )
> +            {
> +                dprintk(XENLOG_ERR, XSPLICE "%s: Unknown type=%#"PRIx16"\n",
> +                        elf->name, idx);
> +                break;
> +            }
> +
> +            sym->st_value += (unsigned long)elf->sec[idx].load_addr;

*(<type> *)&sym->st_value += ...

But anyway.

> +int xsplice_elf_perform_relocs(struct xsplice_elf *elf)
> +{
> +    struct xsplice_elf_sec *r, *base;
> +    unsigned int i;
> +    int rc = 0;
> +
> +    /*
> +     * The first entry of an ELF symbol table is the "undefined symbol 
> index".
> +     * aka reserved so we skip it.
> +     */

Same comment as on v8.1: "This comment seems at least misplaced,
if not pointless."

Jan

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