Messages in this thread | | | Date | Thu, 7 Sep 2023 11:10:21 +1000 | Subject | Re: [PATCH] fs: binfmt_elf_efpic: fix personality for fdpic ELF | From | 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 >>> >>
| |