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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.04.16 at 05:28, <konrad.wilk@xxxxxxxxxx> wrote:
>> > +static int move_payload(struct payload *payload, struct xsplice_elf *elf)
>> > +{
> ..snip..
>> > +    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.
> 
> I am not seeing it. I deleted the line and added it back using
> spaces just in case. But I really don't see the indentation isse
> you are seeing?

Just count the number of blanks - I count 12 ahead of the "else if"
but 13 ahead of the bare "else". And it's still the same in the
updated patch below. Checking what the list archives say (in case
this is an artifact of my mail client) ... Same there (and even more
easily visible in the browser).

> +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 < sizeof(Elf_RelA) ||
> +         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 )
> +            goto bad_offset;

There's one more thing to consider here, which I only notice now:
For "NONE" relocations this check should not be done. Since these

> +        dest = base->load_addr + r->r_offset;
> +        val = r->r_addend + elf->sym[symndx].sym->st_value;

don't touch the possibly out of bounds destination yet, I think
the above should just be dropped here in favor of doing things
below in the individual case statements. But to deal with overflow,
the check above would need to be moved there, i.e. not dropped
entirely.

> +static int move_payload(struct payload *payload, struct xsplice_elf *elf)
> +{
> +    void *text_buf, *ro_buf, *rw_buf;
> +    unsigned int i;
> +    size_t size = 0;
> +    unsigned int *offset;
> +    int rc = 0;
> +
> +    offset = xmalloc_array(unsigned int, elf->hdr->e_shnum);
> +    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]);
> +        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. These are .rel.text, rel.*, .symtab, .strtab,
> +             * and .shstrtab. For the non-relocate we allocate and copy these
> +             * via other means - and the .rel we can ignore as we only use it
> +             * once during loading.
> +             */
> +            offset[i] = UINT_MAX;
> +        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;
> +        }
> +        else /* Such as .comment, or .debug_str. */
> +        {
> +            dprintk(XENLOG_DEBUG, XSPLICE "%s: Ignoring %s section!\n",
> +                    elf->name, elf->sec[i].name);
> +            offset[i] = UINT_MAX;
> +        }

See earlier reply regarding this entire loop body.

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

Again, see earlier reply.

> +        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:
> +            /* 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",

"Out of bounds symbol section"?

Also just %#x now that idx is "unsigned int".

Jan

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