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

[Greg Ungerer <gerg@kernel.org>]

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.


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