Re: [PATCH] fs: binfmt_elf_efpic: fix personality for fdpic ELF

[Greg Ungerer <gerg@kernel.org>]

On 12/7/23 13:43, Greg Ungerer wrote:
> Hi Kees,
> 
> On 12/7/23 02:11, Kees Cook wrote:
>> On Tue, Jul 11, 2023 at 11:39:55PM +1000, Greg Ungerer wrote:
>>> The elf-fdpic loader hard sets the process personality to either
>>> PER_LINUX_FDPIC for true elf-fdpic binaries or to PER_LINUX for
>>> normal ELF binaries (in this case they would be constant displacement
>>> compiled with -pie for example). The problem with that is that it
>>> will lose any other bits that may be in the ELF header personality
>>> (such as the "bug emulation" bits).
>>>
>>> On the ARM architecture the ADDR_LIMIT_32BIT flag is used to signify
>>> a normal 32bit binary - as opposed to a legacy 26bit address binary.
>>> This matters since start_thread() will set the ARM CPSR register as
>>> required based on this flag. If the elf-fdpic loader loses this bit
>>> the process will be mis-configured and crash out pretty quickly.
>>>
>>> Modify elf-fdpic loaders personality setting for ELF binaries so that
>>> it preserves the upper three bytes by using the SET_PERSONALITY macro
>>> to set it. This macro in the generic case sets PER_LINUX but and
>>> preserves the upper bytes. Architectures can override this for their
>>> specific use case, and ARM does exactly this.
>>
>> Thanks for tracking this down!
>>
>> There are some twisty macros in use across all the architectures here!
>>
>> I notice the bare set_personality() call remains, though. Is that right?
>>
>> For example, ARM (and sh and xtensa) also sets:
>>
>> #define elf_check_fdpic(x) ((x)->e_ident[EI_OSABI] == ELFOSABI_ARM_FDPIC)
>>
>> so it's possible the first half of the "if" below could get executed,
>> and ARM (and possibly other architectures) would again lose the other
>> flags, if I'm reading correctly.
> 
> Yes, it is all a little confusing, and the fdpic handling is a little different
> to the standard ELF handling in binfmt_elf.c (with its use of SET_PERSONALITY2).
> 
> 
>> (And the fact that PER_LINUX is actually 0x0 is oddly handled, leaving
>> it implicit in most architectures.)
>>
>> What seems perhaps more correct is to remove the "if" entirely and make
>> sure that SET_PERSONALITY() checks the header flags on all architectures?
> 
> I had thought along those same lines as well. Changing it to be something more
> like this:
> 
>      SET_PERSONALITY(exec_params.hdr);
>      if (elf_check_fdpic(&exec_params.hdr))
>              current->personality |= FDPIC_FUNCPTRS;
> 
> Which I think better handles any arch specifics via the SET_PERSONALITY() use.
> But I chickened out since I can't test fdpic binaries at this time.

I have done some more extensive testing, with fdpic setups now, and this
definitely suffers the same problem. So it needs a fix more like this.
I am generating a v2 that essentially does the above - so it fixes both
the normal ELF and ELF-FDPOIC binary cases.

Regards
Greg


>> But I'm less familiar with this area, so please let me know what I'm
>> missing. :)
> 
> Me too :-)
> It is definitely broken for loading standard ELF binaries on a noMMU system
> using binfmt_elf_fdpic.c, which is what led me down this path. It loses the
> ADDR_LIMIT_32BIT bit in the personality and that causes application crashing.
> 
> 
>>> Signed-off-by: Greg Ungerer <gerg@kernel.org>
>>> ---
>>>
>>> Is anyone out there using elf-fdpic on ARM?
>>
>> It would seem you're the first? :) (_Should_ it be usable on ARM?)
> 
> I was assuming that it must have worked at some time. The binfmt_elf_fdpic
> loader was enabled for ARM in commit 50b2b2e691cd ("ARM: add ELF_FDPIC support")
> by Nicolas Pitre. But that was way back in 2017.
> 
> Regards
> Greg
> 
> 
>> -Kees
>>
>>> This seems to break it rather badly due to the loss of that ADDR_LIMIT_32BIT
>>> bit from the process personality.
>>>
>>>   fs/binfmt_elf_fdpic.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
>>> index a05eafcacfb2..f29ae1d96fd7 100644
>>> --- a/fs/binfmt_elf_fdpic.c
>>> +++ b/fs/binfmt_elf_fdpic.c
>>> @@ -348,7 +348,7 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm)
>>>       if (elf_check_fdpic(&exec_params.hdr))
>>>           set_personality(PER_LINUX_FDPIC);
>>>       else
>>> -        set_personality(PER_LINUX);
>>> +        SET_PERSONALITY(exec_params.hdr);
>>>       if (elf_read_implies_exec(&exec_params.hdr, executable_stack))
>>>           current->personality |= READ_IMPLIES_EXEC;
>>> -- 
>>> 2.25.1
>>>
>>