Thread (1 message) 1 message, 1 author, 2024-07-08

Re: [PATCH] binfmt_elf: Fail execution of shared objects with ELIBEXEC

From: Florian Weimer <hidden>
Date: 2024-07-08 18:00:22
Also in: linux-fsdevel, linux-integrity, linux-mm, linux-security-module, lkml

Possibly related (same subject, not in this thread)

* Eric W. Biederman:
As written I find the logic of the patch confusing, and slightly wrong.

The program header value e_entry is a virtual address, possibly adjusted
by load_bias.  Which makes testing it against the file offset of a
PT_LOAD segment wrong.  It needs to test against elf_ppnt->p_vaddr.
I think we need to test both against zero, or maybe invert the logic: if
something is mapped at virtual address zero that doesn't come from a
zero file offset, we disable the ELIBEXEC check.
I think performing an early sanity check to avoid very confusing crashes
seems sensible (as long as it is inexpensive).  This appears inexpensive
enough that we don't care.  This code is also before begin_new_exec
so it is early enough to be meaningful.
Yeah, it was quite confusing when it was after begin_new_exec because
the ELIBEXEC error is visible under strace, and then the SIGSEGV comes …
I think the check should simply test if e_entry is mapped.  So a range
check please to see if e_entry falls in a PT_LOAD segment.
It's usually mapped even with e_entry ==0 because the ELF header is
loaded at virtual address zero for ET_DYN using the default linker flags
(and this is the case we care about).  With -z noseparate-code, it is
even mapped executable.
Having code start at virtual address 0 is a perfectly fine semantically
and might happen in embedded scenarios.
To keep supporting this case, we need to check that the ELF header is at
address zero, because we make a leap of faith and assume it's not really
executable even if it is mapped as such because due to its role in the
file format, it does not contain executable instructions.  That's why
the patch is focused on the ELF header.

I could remove all these checks and just return ELIBEXEC for a zero
entry point.  I think this is valid based on the ELF specification, but
it may have a backwards compatibility impact.
The program header is not required to be mapped or be first, (AKA
p_offset and p_vaddr can have a somewhat arbitrary relationship) so any
mention of the program header in your logic seems confusing to me.
It's the ELF header.
I think your basic structure will work.  Just the first check needs to
check if e_entry is lands inside the virtual address of a PT_LOAD
segment.  The second check should just be checking a variable to see if
e_entry was inside any PT_LOAD segment, and there is no interpreter.
I think the range check doesn't help here.  Just checking p_vaddr for
zero in addition to p_offset should be sufficient.  If you agree, can
test and send an updated patch.

Thanks,
Florian
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help