Re: [PATCH v4 3/4] libelf: rewrite symtab/strtab loading

["Roger Pau Monné" <roger.pau@citrix.com>]

El 29/2/16 a les 13:14, Jan Beulich ha escrit:
>>>> On 29.02.16 at 11:57, <roger.pau@citrix.com> wrote:
>> El 29/2/16 a les 10:31, Jan Beulich ha escrit:
>>>>>> On 26.02.16 at 18:02, <roger.pau@citrix.com> wrote:
>>>> The layout is as follows (I should add this to the patch itself as a
>>>> comment, since I guess this is still quite confusing):
>>>>
>>>> +------------------------+
>>>> |                        |
>>>> | KERNEL                 |
>>>> |                        |
>>>> +------------------------+ pend
>>>> | ALIGNMENT              |
>>>> +------------------------+ bsd_symtab_pstart
>>>> |                        |
>>>> | size                   | bsd_symtab_pend - bsd_symtab_pstart
>>>> |                        |
>>>> +------------------------+
>>>> |                        |
>>>> | ELF header             |
>>>> |                        |
>>>> +------------------------+
>>>> |                        |
>>>> | ELF section header 0   | Dummy section header
>>>> |                        |
>>>> +------------------------+
>>>> |                        |
>>>> | ELF section header 1   | SYMTAB section header
>>>> |                        |
>>>> +------------------------+
>>>> |                        |
>>>> | ELF section header 2   | STRTAB section header
>>>> |                        |
>>>> +------------------------+
>>>> |                        |
>>>> | SYMTAB                 |
>>>> |                        |
>>>> +------------------------+
>>>> |                        |
>>>> | STRTAB                 |
>>>> |                        |
>>>> +------------------------+ bsd_symtab_pend
>>>>
>>>> There are always only tree sections because that's all we need, section
>>>> 0 is ignored, section 1 is used to describe the SYMTAB, and section 2 is
>>>> used to describe the STRTAB.
>>>
>>> All fine, but this still doesn't clarify how the kernel learns where
>>> bsd_symtab_pstart is.
>>
>> The BSDs linker scripts places an "end" symbol after all the loaded
>> sections:
>>
>> https://svnweb.freebsd.org/base/head/sys/conf/ldscript.amd64?revision=284870& 
>> view=co
> 
> That's fine. But how do they know it is legitimate to even touch what
> follows, not to speak of assign meaning to the value found there?

The kernel signals that it wants it's SYMTAB/STRTAB loaded using the
XEN_ELFNOTE_BSD_SYMTAB ELFNOTE. Then AFAICT it's just a matter of
'faith', there's no signal from Xen to the guest kernel in order to
confirm that the SYMTAB has indeed been loaded...

There's the ELF magic in the header, that one can check in order to make
sure, but apart from that I don't think there's anything else.

> And are there no alignment/padding considerations necessary?

bsd_symtab_pstart is aligned to 4 or 8 bytes (depending on the kernel
bitness).

IMHO, the best way to solve this would be to pass the SYMTAB and STRTAB
as modules for PVH (using the new module list that was introduced with
the new boot ABI), and use the module command line to signal which one
is the strtab and which one is the symtab.

I think this can be done in a backwards compatible way, but this is out
of the scope of this patch.

>>>>>>      sz = elf_round_up(elf, sz);
>>>>>>  
>>>>>> -    /* Space for the symbol and string tables. */
>>>>>> +    /* Space for the symbol and string table. */
>>>>>>      for ( i = 0; i < elf_shdr_count(elf); i++ )
>>>>>>      {
>>>>>>          shdr = elf_shdr_by_index(elf, i);
>>>>>>          if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )
>>>>>>              /* input has an insane section header count field */
>>>>>>              break;
>>>>>> -        type = elf_uval(elf, shdr, sh_type);
>>>>>> -        if ( (type == SHT_STRTAB) || (type == SHT_SYMTAB) )
>>>>>> -            sz = elf_round_up(elf, sz + elf_uval(elf, shdr, sh_size));
>>>>>> +
>>>>>> +        if ( elf_uval(elf, shdr, sh_type) != SHT_SYMTAB )
>>>>>> +            continue;
>>>>>> +
>>>>>> +        sz = elf_round_up(elf, sz + elf_uval(elf, shdr, sh_size));
>>>>>> +        shdr = elf_shdr_by_index(elf, elf_uval(elf, shdr, sh_link));
>>>>>> +
>>>>>> +        if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )
>>>>>> +            /* input has an insane section header count field */
>>>>>> +            break;
>>>>>> +
>>>>>> +        if ( elf_uval(elf, shdr, sh_type) != SHT_STRTAB )
>>>>>> +            /* Invalid symtab -> strtab link */
>>>>>> +            break;
>>>>>
>>>>> This is not sufficient - what if sh_link is out of bounds, but the
>>>>> bogusly accessed field happens to hold SHT_STRTAB? (Oddly
>>>>> enough you have at least an SHN_UNDEF check in the second
>>>>> loop below.)
>>>>
>>>> The out-of-bounds access would be detected by elf_access_ok (if it's out
>>>> of the scope of the ELF file, which is all we care about). In fact the
>>>> elf_access_ok above could be removed because elf_uval already performs
>>>> out-of-bounds checks. The result is definitely no worse that what we are
>>>> doing ATM.
>>>
>>> No, the out of bounds check should be more strict than just
>>> considering the whole image: The image is broken if the link
>>> points to a non-existing section.
>>
>> Ah, do you mean I should mark the image as broken if either of the
>> checks fail?
> 
> Perhaps, but my main point continue to be that there is a check
> missing here.

I'm quite sure I'm missing something, but what kind of extra checks do
you envision?

AFAICT, we already check that the section we are trying to load is not
out-of-bounds (both elf_access_ok and elf_uval make sure of that), and
that it has the right type (STRTAB). Apart from that, I don't know what
else to check. There's no signature in the section headers in order to
check it's integrity.

Roger.

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