[PATCH] fs/binfmt_elf.c: disallow zero entry point address

[PATCH] fs/binfmt_elf.c: disallow zero entry point address

[H.J. Lu]

According to gABI, the entry point address in the ELF header gives the
virtual address to which the system first transfers control, thus
starting the process.  If the file has no associated entry point, this
member holds zero.  Update the ELF loader to disallow an ELF binary
with zero entry point address.  This fixes:

https://bugzilla.kernel.org/show_bug.cgi?id=215303

Tested by booting Fedora 35 and running a shared library with zero entry
point address:

$ readelf -h load.so | grep "Entry point address:"
  Entry point address:               0x0
$ ./load.so
bash: ./load.so: cannot execute binary file: Exec format error
$

Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
---
 fs/binfmt_elf.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index bd78587194dc..bb427c97dc02 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -850,6 +850,8 @@ static int load_elf_binary(struct linux_binprm *bprm)
 
 	if (elf_ex->e_type != ET_EXEC && elf_ex->e_type != ET_DYN)
 		goto out;
+	if (elf_ex->e_entry == 0)
+		goto out;
 	if (!elf_check_arch(elf_ex))
 		goto out;
 	if (elf_check_fdpic(elf_ex))
-- 
2.33.1

Re: [PATCH] fs/binfmt_elf.c: disallow zero entry point address

[Alexey Dobriyan]

On 12/11/21, H.J. Lu <hjl.tools@gmail.com> wrote:
> According to gABI, the entry point address in the ELF header gives the
> virtual address to which the system first transfers control, thus
> starting the process.  If the file has no associated entry point, this
> member holds zero.  Update the ELF loader to disallow an ELF binary
> with zero entry point address.  This fixes:
>
> https://bugzilla.kernel.org/show_bug.cgi?id=215303
>
> Tested by booting Fedora 35 and running a shared library with zero entry
> point address:
>
> $ readelf -h load.so | grep "Entry point address:"
>   Entry point address:               0x0
> $ ./load.so
> bash: ./load.so: cannot execute binary file: Exec format error

Why not let it segfault?

> +	if (elf_ex->e_entry == 0)
> +		goto out;

Re: [PATCH] fs/binfmt_elf.c: disallow zero entry point address

[H.J. Lu]

On Sat, Dec 11, 2021 at 11:38 PM Alexey Dobriyan <adobriyan@gmail.com> wrote:
>
> On 12/11/21, H.J. Lu <hjl.tools@gmail.com> wrote:
> > According to gABI, the entry point address in the ELF header gives the
> > virtual address to which the system first transfers control, thus
> > starting the process.  If the file has no associated entry point, this
> > member holds zero.  Update the ELF loader to disallow an ELF binary
> > with zero entry point address.  This fixes:
> >
> > https://bugzilla.kernel.org/show_bug.cgi?id=215303
> >
> > Tested by booting Fedora 35 and running a shared library with zero entry
> > point address:
> >
> > $ readelf -h load.so | grep "Entry point address:"
> >   Entry point address:               0x0
> > $ ./load.so
> > bash: ./load.so: cannot execute binary file: Exec format error
>
> Why not let it segfault?
>
> > +     if (elf_ex->e_entry == 0)
> > +             goto out;

Why let it segfault?

-- 
H.J.

Re: [PATCH] fs/binfmt_elf.c: disallow zero entry point address

[Linus Torvalds]

On Sun, Dec 12, 2021 at 5:52 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Sat, Dec 11, 2021 at 11:38 PM Alexey Dobriyan <adobriyan@gmail.com> wrote:
> >
> > Why not let it segfault?
>
> Why let it segfault?

That's not my main worry - what if somebody has a code section with a
zero vaddr and intentionally put the entry at the beginning?

Maybe it's not supposed to work by some paper standatd, but afaik
currently it _would_ work.

All these things are relative to the load address, so a zero e_entry
doesn't mean NULL, and may be a perfectly valid address.

No?

            Linus

Re: [PATCH] fs/binfmt_elf.c: disallow zero entry point address

[H.J. Lu]

On Sun, Dec 12, 2021 at 10:29 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Sun, Dec 12, 2021 at 5:52 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >
> > On Sat, Dec 11, 2021 at 11:38 PM Alexey Dobriyan <adobriyan@gmail.com> wrote:
> > >
> > > Why not let it segfault?
> >
> > Why let it segfault?
>
> That's not my main worry - what if somebody has a code section with a
> zero vaddr and intentionally put the entry at the beginning?
>
> Maybe it's not supposed to work by some paper standatd, but afaik
> currently it _would_ work.
>
> All these things are relative to the load address, so a zero e_entry
> doesn't mean NULL, and may be a perfectly valid address.
>
> No?

According to the ELF specification, zero entry point value means
there is no entry point.  Such ELF binary doesn't conform to the
ELF specification.

-- 
H.J.

Re: [PATCH] fs/binfmt_elf.c: disallow zero entry point address

[Linus Torvalds]

On Sun, Dec 12, 2021 at 11:06 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> According to the ELF specification, zero entry point value means
> there is no entry point.  Such ELF binary doesn't conform to the
> ELF specification.

Nobody cares about paper specifications.

All that matters is REALITY.

So let me quote my email again, since you clearly didn't actually read
it (read that "maybe it's not supposed to work" part):

> That's not my main worry - what if somebody has a code section with a
> zero vaddr and intentionally put the entry at the beginning?
>
> Maybe it's not supposed to work by some paper standatd, but afaik
> currently it _would_ work.

I'm not sure this can happen currently (maybe all tools effectively
make it so that the ELF headers etc are part of the loaded image).

But no, paper specifications have absolutely no meaning if they don't
match realty.

And the reality is that I don't think we've ever checked e_entry being
zero, which means that maybe people have used it.

So convince me that the above cannot happen. I'm perfectly willing to
be convinced, but "some random paper standard that we've never
followed" is not the thing to quote.

             Linus

Re: [PATCH] fs/binfmt_elf.c: disallow zero entry point address

[H.J. Lu]

On Sun, Dec 12, 2021 at 11:15 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Sun, Dec 12, 2021 at 11:06 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >
> > According to the ELF specification, zero entry point value means
> > there is no entry point.  Such ELF binary doesn't conform to the
> > ELF specification.
>
> Nobody cares about paper specifications.
>
> All that matters is REALITY.
>
> So let me quote my email again, since you clearly didn't actually read
> it (read that "maybe it's not supposed to work" part):
>
> > That's not my main worry - what if somebody has a code section with a
> > zero vaddr and intentionally put the entry at the beginning?
> >
> > Maybe it's not supposed to work by some paper standatd, but afaik
> > currently it _would_ work.
>
> I'm not sure this can happen currently (maybe all tools effectively
> make it so that the ELF headers etc are part of the loaded image).
>
> But no, paper specifications have absolutely no meaning if they don't
> match realty.
>
> And the reality is that I don't think we've ever checked e_entry being
> zero, which means that maybe people have used it.
>
> So convince me that the above cannot happen. I'm perfectly willing to
> be convinced, but "some random paper standard that we've never
> followed" is not the thing to quote.
>

On Linux, the start of the first PT_LOAD segment is the ELF
header and the address 0 points to the ELF magic bytes which
isn't a valid code sequence.

-- 
H.J.

Re: [PATCH] fs/binfmt_elf.c: disallow zero entry point address

[Linus Torvalds]

On Sun, Dec 12, 2021 at 11:15 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I'm not sure this can happen currently (maybe all tools effectively
> make it so that the ELF headers etc are part of the loaded image).

Side note: if that ends up being the case (ie e_entry always
effectively relative to the head of the image), then I think a better
fix would be to make that explicit, something like

        if (elf_ex->e_entry < header_sizes)
                goto out;

but the logic on exactly how things get loaded is so messy that I'm
not sure just what the situation is.

We've had things like old tool chains generate messy binaries before,
to the point where we've had to revert much more important changes (ie
the whole mess with MAP_FIXED_NOREPLACE and overlapping sections).

                 Linus

Re: [PATCH] fs/binfmt_elf.c: disallow zero entry point address

[Linus Torvalds]

[ Crossed emails ]

On Sun, Dec 12, 2021 at 11:30 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Linux, the start of the first PT_LOAD segment is the ELF
> header and the address 0 points to the ELF magic bytes which
> isn't a valid code sequence.

Yeah, then I think a much more valid argument (and patch) is _that_ argument.

So that kind of explanation, along with a patch more along the line of that

        if (elf_ex->e_entry < header_sizes)
                goto out;

I suggested, and not talking about paper standards that may or may not
be relevant.

That would be much more palatable to me - it's a _technical_ argument,
not a "some paper standard that we clearly have never followed"
argument.

                Linus

Re: [PATCH] fs/binfmt_elf.c: disallow zero entry point address

[H.J. Lu]

On Sun, Dec 12, 2021 at 11:35:56AM -0800, Linus Torvalds wrote:
> [ Crossed emails ]
> 
> On Sun, Dec 12, 2021 at 11:30 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >
> > On Linux, the start of the first PT_LOAD segment is the ELF
> > header and the address 0 points to the ELF magic bytes which
> > isn't a valid code sequence.
> 
> Yeah, then I think a much more valid argument (and patch) is _that_ argument.
> 
> So that kind of explanation, along with a patch more along the line of that
> 
>         if (elf_ex->e_entry < header_sizes)
>                 goto out;
> 
> I suggested, and not talking about paper standards that may or may not
> be relevant.
> 
> That would be much more palatable to me - it's a _technical_ argument,
> not a "some paper standard that we clearly have never followed"
> argument.
> 
>                 Linus

I sent out the v2 patch with

	if (elf_ex->e_entry < sizeof(*elf_ex))
		goto out;


H.J.

Re: [PATCH] fs/binfmt_elf.c: disallow zero entry point address

[Alexey Dobriyan]

On Sun, Dec 12, 2021 at 05:52:14AM -0800, H.J. Lu wrote:
> On Sat, Dec 11, 2021 at 11:38 PM Alexey Dobriyan <adobriyan@gmail.com> wrote:
> >
> > On 12/11/21, H.J. Lu <hjl.tools@gmail.com> wrote:
> > > According to gABI, the entry point address in the ELF header gives the
> > > virtual address to which the system first transfers control, thus
> > > starting the process.  If the file has no associated entry point, this
> > > member holds zero.  Update the ELF loader to disallow an ELF binary
> > > with zero entry point address.  This fixes:
> > >
> > > https://bugzilla.kernel.org/show_bug.cgi?id=215303
> > >
> > > Tested by booting Fedora 35 and running a shared library with zero entry
> > > point address:
> > >
> > > $ readelf -h load.so | grep "Entry point address:"
> > >   Entry point address:               0x0
> > > $ ./load.so
> > > bash: ./load.so: cannot execute binary file: Exec format error
> >
> > Why not let it segfault?
> >
> > > +     if (elf_ex->e_entry == 0)
> > > +             goto out;
> 
> Why let it segfault?

Such babysitting adds a branch for everyone to catch small number of
binaries.

e_entry can point to kernelspace, and it should segfault on the first
instruction (correctly).

It can iincorrectly point to unmapped area or VMA with wrong permissions,
with the same effect. But now check is more costly.