[PATCH] alpha: Fix personality flag propagation across an exec

John Paul Adrian Glaubitz posted 1 patch 1 year, 1 month ago
arch/alpha/include/asm/elf.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
[PATCH] alpha: Fix personality flag propagation across an exec
Posted by John Paul Adrian Glaubitz 1 year, 1 month ago
It was observed that on alpha, the misc/setarch test of
the util-linux testsuite failed with the following error:

   misc: setarch                        ...
          : options                     ... OK
          : uname26                     ... OK
          : uname26-version             ... FAILED (misc/setarch-uname26-version)
          : show                        ... OK
     ... FAILED (1 from 4 sub-tests)

Running the setarch binary manually confirmed that setting
the kernel version with the help --uname-2.6 flag does not
work and the version remains unchanged.

It turned out that on alpha, the personality flags are not
propagated but overridden during an exec. The same issue was
previously fixed on arm in commit 5e143436d044 ("ARM: 6878/1:
fix personality flag propagation across an exec") and on powerpc
in commit a91a03ee31a5 ("powerpc: Keep 3 high personality bytes
across exec"). This patch fixes the issue on alpha.

With the patch applied, the misc/setarch test succeeds on
alpha as expected:

   misc: setarch                        ...
          : options                     ... OK
          : uname26                     ... OK
          : uname26-version             ... OK
          : show                        ... OK
     ... OK (all 4 sub-tests PASSED)

However, as a side-effect, a warning is printed on the kernel
message buffer which might indicate another unreleated bug:

[   39.964823] pid=509, couldn't seal address 0, ret=-12.

Signed-off-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
---
 arch/alpha/include/asm/elf.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/alpha/include/asm/elf.h b/arch/alpha/include/asm/elf.h
index 4d7c46f50382..81f8473bb7c0 100644
--- a/arch/alpha/include/asm/elf.h
+++ b/arch/alpha/include/asm/elf.h
@@ -138,8 +138,8 @@ extern int dump_elf_task(elf_greg_t *dest, struct task_struct *task);
 })
 
 #define SET_PERSONALITY(EX)					\
-	set_personality(((EX).e_flags & EF_ALPHA_32BIT)		\
-	   ? PER_LINUX_32BIT : PER_LINUX)
+	set_personality((((EX).e_flags & EF_ALPHA_32BIT)	\
+	   ? PER_LINUX_32BIT : PER_LINUX) | (current->personality & (~PER_MASK)))
 
 extern int alpha_l1i_cacheshape;
 extern int alpha_l1d_cacheshape;
-- 
2.39.5
Re: [PATCH] alpha: Fix personality flag propagation across an exec
Posted by Arnd Bergmann 1 year, 1 month ago
On Fri, Jan 3, 2025, at 15:01, John Paul Adrian Glaubitz wrote:

> 
>  #define SET_PERSONALITY(EX)					\
> -	set_personality(((EX).e_flags & EF_ALPHA_32BIT)		\
> -	   ? PER_LINUX_32BIT : PER_LINUX)
> +	set_personality((((EX).e_flags & EF_ALPHA_32BIT)	\
> +	   ? PER_LINUX_32BIT : PER_LINUX) | (current->personality & (~PER_MASK)))

This looks wrong to me: since ADDR_LIMIT_32BIT is not part of
PER_MASK, executing a regular binary from a taso binary no longer
reverts back to the entire 64-bit address space.

It seems that the behavior on most other architectures changed in 2012
commit 16f3e95b3209 ("cross-arch: don't corrupt personality flags upon
exec()").

At the time, the same bug existed on mips, parisc and tile, but those
got fixed quickly.

There are two related bits I don't quite understand:

- Do we still care about EF_ALPHA_32BIT? I see that it gets set by
 "alpha-linux-ld.bfd --taso", but could not find any documentation
 on what that flag is actually good for. On all other architectures,
 the address space limit gets enforced through a per-thread setting
 like TIF_32BIT, not through the personality that gets inherited
 by the child processes.

- all architectures other than x86 mask out the lower byte. Why
  not that one?

       Arnd
Re: [PATCH] alpha: Fix personality flag propagation across an exec
Posted by Arnd Bergmann 1 year, 1 month ago
On Thu, Jan 9, 2025, at 09:01, Arnd Bergmann wrote:
> On Fri, Jan 3, 2025, at 15:01, John Paul Adrian Glaubitz wrote:
>
>> 
>>  #define SET_PERSONALITY(EX)					\
>> -	set_personality(((EX).e_flags & EF_ALPHA_32BIT)		\
>> -	   ? PER_LINUX_32BIT : PER_LINUX)
>> +	set_personality((((EX).e_flags & EF_ALPHA_32BIT)	\
>> +	   ? PER_LINUX_32BIT : PER_LINUX) | (current->personality & (~PER_MASK)))
>
> This looks wrong to me: since ADDR_LIMIT_32BIT is not part of
> PER_MASK, executing a regular binary from a taso binary no longer
> reverts back to the entire 64-bit address space.
>
> It seems that the behavior on most other architectures changed in 2012
> commit 16f3e95b3209 ("cross-arch: don't corrupt personality flags upon
> exec()").
>
> At the time, the same bug existed on mips, parisc and tile, but those
> got fixed quickly.

Correction: from what I can tell, mips still has the bug (and now
also loongarch), it's just in SET_PERSONALITY2() now instead of
SET_PERSONALITY():

        current->personality &= ~READ_IMPLIES_EXEC;
        ...
        p = personality(current->personality);                          \
        if (p != PER_LINUX32 && p != PER_LINUX)                         \
                set_personality(PER_LINUX);                             \

personality() only returns the lower 8 bits (execution domain),
so if any of them are set (BSD/HPUX/IRIX32/IRIX64/...), both
the upper and the lower bits are cleared, otherwise neither
of them are.

The behavior on the other architectures is that we clear the
lower bits but keep the upper ones.

      Arnd
Re: [PATCH] alpha: Fix personality flag propagation across an exec
Posted by John Paul Adrian Glaubitz 1 year, 1 month ago
Hi Arnd,

thanks a lot for your feedback!

On Thu, 2025-01-09 at 09:43 +0100, Arnd Bergmann wrote:
> On Thu, Jan 9, 2025, at 09:01, Arnd Bergmann wrote:
> > On Fri, Jan 3, 2025, at 15:01, John Paul Adrian Glaubitz wrote:
> > 
> > > 
> > >  #define SET_PERSONALITY(EX)					\
> > > -	set_personality(((EX).e_flags & EF_ALPHA_32BIT)		\
> > > -	   ? PER_LINUX_32BIT : PER_LINUX)
> > > +	set_personality((((EX).e_flags & EF_ALPHA_32BIT)	\
> > > +	   ? PER_LINUX_32BIT : PER_LINUX) | (current->personality & (~PER_MASK)))
> > 
> > This looks wrong to me: since ADDR_LIMIT_32BIT is not part of
> > PER_MASK, executing a regular binary from a taso binary no longer
> > reverts back to the entire 64-bit address space.
> > 
> > It seems that the behavior on most other architectures changed in 2012
> > commit 16f3e95b3209 ("cross-arch: don't corrupt personality flags upon
> > exec()").
> > 
> > At the time, the same bug existed on mips, parisc and tile, but those
> > got fixed quickly.
> 
> Correction: from what I can tell, mips still has the bug (and now
> also loongarch), it's just in SET_PERSONALITY2() now instead of
> SET_PERSONALITY():
> 
>         current->personality &= ~READ_IMPLIES_EXEC;
>         ...
>         p = personality(current->personality);                          \
>         if (p != PER_LINUX32 && p != PER_LINUX)                         \
>                 set_personality(PER_LINUX);                             \
> 
> personality() only returns the lower 8 bits (execution domain),
> so if any of them are set (BSD/HPUX/IRIX32/IRIX64/...), both
> the upper and the lower bits are cleared, otherwise neither
> of them are.
> 
> The behavior on the other architectures is that we clear the
> lower bits but keep the upper ones.

So, if I understand this correctly, we should just use PER_MASK on alpha
for 64-bit executables and allow the bits to be cleared for 32-bit binaries?

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913
Re: [PATCH] alpha: Fix personality flag propagation across an exec
Posted by Kees Cook 1 year, 1 month ago
On Fri, Jan 03, 2025 at 03:01:46PM +0100, John Paul Adrian Glaubitz wrote:
> It was observed that on alpha, the misc/setarch test of
> the util-linux testsuite failed with the following error:
> 
>    misc: setarch                        ...
>           : options                     ... OK
>           : uname26                     ... OK
>           : uname26-version             ... FAILED (misc/setarch-uname26-version)
>           : show                        ... OK
>      ... FAILED (1 from 4 sub-tests)
> 
> Running the setarch binary manually confirmed that setting
> the kernel version with the help --uname-2.6 flag does not
> work and the version remains unchanged.
> 
> It turned out that on alpha, the personality flags are not
> propagated but overridden during an exec. The same issue was
> previously fixed on arm in commit 5e143436d044 ("ARM: 6878/1:
> fix personality flag propagation across an exec") and on powerpc
> in commit a91a03ee31a5 ("powerpc: Keep 3 high personality bytes
> across exec"). This patch fixes the issue on alpha.

Good catch!

> 
> With the patch applied, the misc/setarch test succeeds on
> alpha as expected:
> 
>    misc: setarch                        ...
>           : options                     ... OK
>           : uname26                     ... OK
>           : uname26-version             ... OK
>           : show                        ... OK
>      ... OK (all 4 sub-tests PASSED)
> 
> However, as a side-effect, a warning is printed on the kernel
> message buffer which might indicate another unreleated bug:
> 
> [   39.964823] pid=509, couldn't seal address 0, ret=-12.

This is from mseal vs MMAP_PAGE_ZERO in fs/binfmt_elf.c

                error = vm_mmap(NULL, 0, PAGE_SIZE, PROT_READ | PROT_EXEC,
                                MAP_FIXED | MAP_PRIVATE, 0);

                retval = do_mseal(0, PAGE_SIZE, 0);
                if (retval)
                        pr_warn_ratelimited("pid=%d, couldn't seal address 0, ret=%d.\n",
                                            task_pid_nr(current), retval);

-12 is ENOMEM, which implies, I think, that check_mm_seal() failed. I
note that "error" isn't being checked, so if the vm_mmap() failed, I
think the do_mseal() would fail with ENOMEM?

> Signed-off-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>

Reviewed-by: Kees Cook <kees@kernel.org>

-Kees

> ---
>  arch/alpha/include/asm/elf.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/alpha/include/asm/elf.h b/arch/alpha/include/asm/elf.h
> index 4d7c46f50382..81f8473bb7c0 100644
> --- a/arch/alpha/include/asm/elf.h
> +++ b/arch/alpha/include/asm/elf.h
> @@ -138,8 +138,8 @@ extern int dump_elf_task(elf_greg_t *dest, struct task_struct *task);
>  })
>  
>  #define SET_PERSONALITY(EX)					\
> -	set_personality(((EX).e_flags & EF_ALPHA_32BIT)		\
> -	   ? PER_LINUX_32BIT : PER_LINUX)
> +	set_personality((((EX).e_flags & EF_ALPHA_32BIT)	\
> +	   ? PER_LINUX_32BIT : PER_LINUX) | (current->personality & (~PER_MASK)))
>  
>  extern int alpha_l1i_cacheshape;
>  extern int alpha_l1d_cacheshape;
> -- 
> 2.39.5
> 

-- 
Kees Cook
Re: [PATCH] alpha: Fix personality flag propagation across an exec
Posted by Jeff Xu 1 year, 1 month ago
On Wed, Jan 8, 2025 at 2:49 PM Kees Cook <kees@kernel.org> wrote:
>
> On Fri, Jan 03, 2025 at 03:01:46PM +0100, John Paul Adrian Glaubitz wrote:
> > It was observed that on alpha, the misc/setarch test of
> > the util-linux testsuite failed with the following error:
> >
> >    misc: setarch                        ...
> >           : options                     ... OK
> >           : uname26                     ... OK
> >           : uname26-version             ... FAILED (misc/setarch-uname26-version)
> >           : show                        ... OK
> >      ... FAILED (1 from 4 sub-tests)
> >
> > Running the setarch binary manually confirmed that setting
> > the kernel version with the help --uname-2.6 flag does not
> > work and the version remains unchanged.
> >
> > It turned out that on alpha, the personality flags are not
> > propagated but overridden during an exec. The same issue was
> > previously fixed on arm in commit 5e143436d044 ("ARM: 6878/1:
> > fix personality flag propagation across an exec") and on powerpc
> > in commit a91a03ee31a5 ("powerpc: Keep 3 high personality bytes
> > across exec"). This patch fixes the issue on alpha.
>
> Good catch!
>
> >
> > With the patch applied, the misc/setarch test succeeds on
> > alpha as expected:
> >
> >    misc: setarch                        ...
> >           : options                     ... OK
> >           : uname26                     ... OK
> >           : uname26-version             ... OK
> >           : show                        ... OK
> >      ... OK (all 4 sub-tests PASSED)
> >
> > However, as a side-effect, a warning is printed on the kernel
> > message buffer which might indicate another unreleated bug:
> >
> > [   39.964823] pid=509, couldn't seal address 0, ret=-12.
>
> This is from mseal vs MMAP_PAGE_ZERO in fs/binfmt_elf.c
>
>                 error = vm_mmap(NULL, 0, PAGE_SIZE, PROT_READ | PROT_EXEC,
>                                 MAP_FIXED | MAP_PRIVATE, 0);
>
>                 retval = do_mseal(0, PAGE_SIZE, 0);
>                 if (retval)
>                         pr_warn_ratelimited("pid=%d, couldn't seal address 0, ret=%d.\n",
>                                             task_pid_nr(current), retval);
>
> -12 is ENOMEM, which implies, I think, that check_mm_seal() failed. I
> note that "error" isn't being checked, so if the vm_mmap() failed, I
> think the do_mseal() would fail with ENOMEM?
>
Yes. do_mseal would fail with NOMEM if the address was not found.

It is likely that alpha doesn't allow creating a page on zero address
? i.e.  MMAP_PAGE_ZERO personality never worked on alpha.

-Jeff

> > Signed-off-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
>
> Reviewed-by: Kees Cook <kees@kernel.org>
>
> -Kees
>
> > ---
> >  arch/alpha/include/asm/elf.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/alpha/include/asm/elf.h b/arch/alpha/include/asm/elf.h
> > index 4d7c46f50382..81f8473bb7c0 100644
> > --- a/arch/alpha/include/asm/elf.h
> > +++ b/arch/alpha/include/asm/elf.h
> > @@ -138,8 +138,8 @@ extern int dump_elf_task(elf_greg_t *dest, struct task_struct *task);
> >  })
> >
> >  #define SET_PERSONALITY(EX)                                  \
> > -     set_personality(((EX).e_flags & EF_ALPHA_32BIT)         \
> > -        ? PER_LINUX_32BIT : PER_LINUX)
> > +     set_personality((((EX).e_flags & EF_ALPHA_32BIT)        \
> > +        ? PER_LINUX_32BIT : PER_LINUX) | (current->personality & (~PER_MASK)))
> >
> >  extern int alpha_l1i_cacheshape;
> >  extern int alpha_l1d_cacheshape;
> > --
> > 2.39.5
> >
>
> --
> Kees Cook
>