ARM: rename missed uaccess .fixup section

Commit Message

Kees Cook Feb. 8, 2020, 2:02 a.m. UTC

When the uaccess .fixup section was renamed to .text.fixup, one case was
missed. Under ld.bfd, the orphaned section was moved close to .text
(since they share the "ax" bits), so things would work normally on
uaccess faults. Under ld.lld, the orphaned section was placed outside
the .text section, making it unreachable. Rename the missed section.

Link: https://github.com/ClangBuiltLinux/linux/issues/282
Link: https://bugs.chromium.org/p/chromium/issues/detail?id=1020633#c44
Link: https://lore.kernel.org/r/[e-mail]
Fixes: c4a84ae39b4a5 ("ARM: 8322/1: keep .text and .fixup regions closer together")
Cc: [e-mail]
Reported-by: Nathan Chancellor <[e-mail]>
Reported-by: Manoj Gupta <[e-mail]>
Debugged-by: Nick Desaulniers <[e-mail]>
Signed-off-by: Kees Cook <[e-mail]>
---
I completely missed this the first several times I looked at this
problem. Thank you Nicolas for pushing back on the earlier patch!
Manoj or Nathan, can you test this?
---
 arch/arm/lib/copy_from_user.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Nick Desaulniers Feb. 8, 2020, 7:18 a.m. UTC | #1

On Sat, Feb 8, 2020 at 3:02 AM Kees Cook <[e-mail]> wrote:

Looks like we were both up late debugging this! Great job finding a fix!

>
> When the uaccess .fixup section was renamed to .text.fixup, one case was
> missed. Under ld.bfd, the orphaned section was moved close to .text
> (since they share the "ax" bits), so things would work normally on
> uaccess faults. Under ld.lld, the orphaned section was placed outside
> the .text section, making it unreachable. Rename the missed section.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/282
> Link: https://bugs.chromium.org/p/chromium/issues/detail?id=1020633#c44
> Link: https://lore.kernel.org/r/[e-mail]
> Fixes: c4a84ae39b4a5 ("ARM: 8322/1: keep .text and .fixup regions closer together")

I was curious if the "mix" of `.fixup` and `.text.fixup` in a few
places under arch/arm/ was intentional or not. I should have
investigated that more.

> Cc: [e-mail]
> Reported-by: Nathan Chancellor <[e-mail]>
> Reported-by: Manoj Gupta <[e-mail]>
> Debugged-by: Nick Desaulniers <[e-mail]>
> Signed-off-by: Kees Cook <[e-mail]>

Before this patch:
$ ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- make CC=clang LD=ld.lld -j71
$ readelf -S arch/arm/lib/copy_from_user.o
...
   [ 9] .fixup            PROGBITS        00000000 0004e8 00001c 00
AX  0   0  4
...
$ readelf -S vmlinux
...
  [ 2] .fixup            PROGBITS        c020826c 00126c 00001c 00  AX  0   0  4
  [ 3] .text             PROGBITS        c0300000 002000 d71964 00 WAX
 0   0 4096
....
(Which is bad since .fixup resides before _stext!)
$ readelf -s vmlinux | grep _stext
203324: c0300000     0 NOTYPE  GLOBAL DEFAULT    3 _stext

After this patch is applied:
$ readelf -S arch/arm/lib/copy_from_user.o
...
  [ 9] .text.fixup       PROGBITS        00000000 0004e8 00001c 00  AX  0   0  4
...
$ readelf -S vmlinux
...
  [ 2] .text             PROGBITS        c0300000 002000 d71964 00 WAX
 0   0 4096
...
(So there's no orphaned .fixup section).  I forget if I was just
discussing it w/ Ard or Arnd a few days ago but I think we should
really enable warning on orphan sections during link (lest we continue
to run into issues like this).

$ grep -r \\.fixup arch/arm

turns up another hit in:
arch/arm/boot/compressed/vmlinux.lds.S:    *(.fixup)

Which I think should be fixed, too, maybe in a V2 so I'll hold my
reviewed-by tag for now. (Modifying that locally, I'm able to boot
qemu, and I also don't see any object files with such a section, ie.

$ readelf -S arch/arm/boot/compressed/*.o | grep fixup

comes up empty. So it could be renamed or even removed).

We should also cc stable, since c4a84ae39b4a5 first landed in v4.1-rc1.

Thanks for the patch!

> ---
> I completely missed this the first several times I looked at this
> problem. Thank you Nicolas for pushing back on the earlier patch!
> Manoj or Nathan, can you test this?
> ---
>  arch/arm/lib/copy_from_user.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/lib/copy_from_user.S b/arch/arm/lib/copy_from_user.S
> index 95b2e1ce559c..f8016e3db65d 100644
> --- a/arch/arm/lib/copy_from_user.S
> +++ b/arch/arm/lib/copy_from_user.S
> @@ -118,7 +118,7 @@ ENTRY(arm_copy_from_user)
>
>  ENDPROC(arm_copy_from_user)
>
> -       .pushsection .fixup,"ax"
> +       .pushsection .text.fixup,"ax"
>         .align 0
>         copy_abort_preamble
>         ldmfd   sp!, {r1, r2, r3}
> --
> 2.20.1
>
>
> --
> Kees Cook

Ard Biesheuvel Feb. 8, 2020, 7:54 a.m. UTC | #2

On Sat, 8 Feb 2020 at 02:02, Kees Cook <[e-mail]> wrote:
>
> When the uaccess .fixup section was renamed to .text.fixup, one case was
> missed. Under ld.bfd, the orphaned section was moved close to .text
> (since they share the "ax" bits), so things would work normally on
> uaccess faults. Under ld.lld, the orphaned section was placed outside
> the .text section, making it unreachable. Rename the missed section.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/282
> Link: https://bugs.chromium.org/p/chromium/issues/detail?id=1020633#c44
> Link: https://lore.kernel.org/r/[e-mail]
> Fixes: c4a84ae39b4a5 ("ARM: 8322/1: keep .text and .fixup regions closer together")
> Cc: [e-mail]
> Reported-by: Nathan Chancellor <[e-mail]>
> Reported-by: Manoj Gupta <[e-mail]>
> Debugged-by: Nick Desaulniers <[e-mail]>
> Signed-off-by: Kees Cook <[e-mail]>

Reviewed-by: Ard Biesheuvel <[e-mail]>

As Nick points out, the *(.fixup) line still appears in the
decompressor's linker script, but this is harmless, given that we
don't ever emit anything into that section. But while we're at it, we
might just remove it as well.


> ---
> I completely missed this the first several times I looked at this
> problem. Thank you Nicolas for pushing back on the earlier patch!
> Manoj or Nathan, can you test this?
> ---
>  arch/arm/lib/copy_from_user.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/lib/copy_from_user.S b/arch/arm/lib/copy_from_user.S
> index 95b2e1ce559c..f8016e3db65d 100644
> --- a/arch/arm/lib/copy_from_user.S
> +++ b/arch/arm/lib/copy_from_user.S
> @@ -118,7 +118,7 @@ ENTRY(arm_copy_from_user)
>
>  ENDPROC(arm_copy_from_user)
>
> -       .pushsection .fixup,"ax"
> +       .pushsection .text.fixup,"ax"
>         .align 0
>         copy_abort_preamble
>         ldmfd   sp!, {r1, r2, r3}
> --
> 2.20.1
>
>
> --
> Kees Cook

Kees Cook Feb. 8, 2020, 8:55 a.m. UTC | #3

On Sat, Feb 08, 2020 at 07:54:39AM +0000, Ard Biesheuvel wrote:
> On Sat, 8 Feb 2020 at 02:02, Kees Cook <[e-mail]> wrote:
> >
> > When the uaccess .fixup section was renamed to .text.fixup, one case was
> > missed. Under ld.bfd, the orphaned section was moved close to .text
> > (since they share the "ax" bits), so things would work normally on
> > uaccess faults. Under ld.lld, the orphaned section was placed outside
> > the .text section, making it unreachable. Rename the missed section.
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/282
> > Link: https://bugs.chromium.org/p/chromium/issues/detail?id=1020633#c44
> > Link: https://lore.kernel.org/r/[e-mail]
> > Fixes: c4a84ae39b4a5 ("ARM: 8322/1: keep .text and .fixup regions closer together")
> > Cc: [e-mail]
> > Reported-by: Nathan Chancellor <[e-mail]>
> > Reported-by: Manoj Gupta <[e-mail]>
> > Debugged-by: Nick Desaulniers <[e-mail]>
> > Signed-off-by: Kees Cook <[e-mail]>
> 
> Reviewed-by: Ard Biesheuvel <[e-mail]>

Thanks!

> As Nick points out, the *(.fixup) line still appears in the
> decompressor's linker script, but this is harmless, given that we
> don't ever emit anything into that section. But while we're at it, we
> might just remove it as well.

Agreed. I'll send a separate patch for that.

-Kees

> 
> 
> > ---
> > I completely missed this the first several times I looked at this
> > problem. Thank you Nicolas for pushing back on the earlier patch!
> > Manoj or Nathan, can you test this?
> > ---
> >  arch/arm/lib/copy_from_user.S | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/lib/copy_from_user.S b/arch/arm/lib/copy_from_user.S
> > index 95b2e1ce559c..f8016e3db65d 100644
> > --- a/arch/arm/lib/copy_from_user.S
> > +++ b/arch/arm/lib/copy_from_user.S
> > @@ -118,7 +118,7 @@ ENTRY(arm_copy_from_user)
> >
> >  ENDPROC(arm_copy_from_user)
> >
> > -       .pushsection .fixup,"ax"
> > +       .pushsection .text.fixup,"ax"
> >         .align 0
> >         copy_abort_preamble
> >         ldmfd   sp!, {r1, r2, r3}
> > --
> > 2.20.1
> >
> >
> > --
> > Kees Cook

Nick Desaulniers Feb. 8, 2020, 10:04 a.m. UTC | #4

On Sat, Feb 8, 2020 at 9:55 AM Kees Cook <[e-mail]> wrote:
>
> On Sat, Feb 08, 2020 at 07:54:39AM +0000, Ard Biesheuvel wrote:
> > On Sat, 8 Feb 2020 at 02:02, Kees Cook <[e-mail]> wrote:
> > >
> > > When the uaccess .fixup section was renamed to .text.fixup, one case was
> > > missed. Under ld.bfd, the orphaned section was moved close to .text
> > > (since they share the "ax" bits), so things would work normally on
> > > uaccess faults. Under ld.lld, the orphaned section was placed outside
> > > the .text section, making it unreachable. Rename the missed section.
> > >
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/282
> > > Link: https://bugs.chromium.org/p/chromium/issues/detail?id=1020633#c44
> > > Link: https://lore.kernel.org/r/[e-mail]
> > > Fixes: c4a84ae39b4a5 ("ARM: 8322/1: keep .text and .fixup regions closer together")
> > > Cc: [e-mail]
> > > Reported-by: Nathan Chancellor <[e-mail]>
> > > Reported-by: Manoj Gupta <[e-mail]>
> > > Debugged-by: Nick Desaulniers <[e-mail]>
> > > Signed-off-by: Kees Cook <[e-mail]>
> >
> > Reviewed-by: Ard Biesheuvel <[e-mail]>
>
> Thanks!
>
> > As Nick points out, the *(.fixup) line still appears in the
> > decompressor's linker script, but this is harmless, given that we
> > don't ever emit anything into that section. But while we're at it, we
> > might just remove it as well.
>
> Agreed. I'll send a separate patch for that.

Sure, in that case
Reviewed-by: Nick Desaulniers <[e-mail]>

>
> -Kees
>
> >
> >
> > > ---
> > > I completely missed this the first several times I looked at this
> > > problem. Thank you Nicolas for pushing back on the earlier patch!
> > > Manoj or Nathan, can you test this?
> > > ---
> > >  arch/arm/lib/copy_from_user.S | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/arch/arm/lib/copy_from_user.S b/arch/arm/lib/copy_from_user.S
> > > index 95b2e1ce559c..f8016e3db65d 100644
> > > --- a/arch/arm/lib/copy_from_user.S
> > > +++ b/arch/arm/lib/copy_from_user.S
> > > @@ -118,7 +118,7 @@ ENTRY(arm_copy_from_user)
> > >
> > >  ENDPROC(arm_copy_from_user)
> > >
> > > -       .pushsection .fixup,"ax"
> > > +       .pushsection .text.fixup,"ax"
> > >         .align 0
> > >         copy_abort_preamble
> > >         ldmfd   sp!, {r1, r2, r3}
> > > --
> > > 2.20.1
> > >
> > >
> > > --
> > > Kees Cook
>
> --
> Kees Cook

Patch

diff --git a/arch/arm/lib/copy_from_user.S b/arch/arm/lib/copy_from_user.S
index 95b2e1ce559c..f8016e3db65d 100644
--- a/arch/arm/lib/copy_from_user.S
+++ b/arch/arm/lib/copy_from_user.S
@@ -118,7 +118,7 @@  ENTRY(arm_copy_from_user)
 
 ENDPROC(arm_copy_from_user)
 
-	.pushsection .fixup,"ax"
+	.pushsection .text.fixup,"ax"
 	.align 0
 	copy_abort_preamble
 	ldmfd	sp!, {r1, r2, r3}