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

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

[Greg Ungerer]

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.

Signed-off-by: Greg Ungerer <gerg@kernel.org>
---

Is anyone out there using elf-fdpic on ARM?
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

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

[Kees Cook]

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.

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

But I'm less familiar with this area, so please let me know what I'm
missing. :)

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

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

-- 
Kees Cook

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

[Greg Ungerer]

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

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

[Greg Ungerer]

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