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

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

>>> On 29.02.16 at 17:20, <roger.pau@citrix.com> wrote:
> 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:
>>>>>>> -    /* 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?

0 < sh_link < elf_shdr_count(elf)

Jan


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