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/[email protected]
Fixes: c4a84ae39b4a5 ("ARM: 8322/1: keep .text and .fixup regions closer together")
Cc: [email protected]
Reported-by: Nathan Chancellor <[email protected]>
Reported-by: Manoj Gupta <[email protected]>
Debugged-by: Nick Desaulniers <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
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
On Sat, Feb 8, 2020 at 3:02 AM Kees Cook <[email protected]> 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/[email protected]
> 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: [email protected]
> Reported-by: Nathan Chancellor <[email protected]>
> Reported-by: Manoj Gupta <[email protected]>
> Debugged-by: Nick Desaulniers <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
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
--
Thanks,
~Nick Desaulniers
On Sat, 8 Feb 2020 at 02:02, Kees Cook <[email protected]> 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/[email protected]
> Fixes: c4a84ae39b4a5 ("ARM: 8322/1: keep .text and .fixup regions closer together")
> Cc: [email protected]
> Reported-by: Nathan Chancellor <[email protected]>
> Reported-by: Manoj Gupta <[email protected]>
> Debugged-by: Nick Desaulniers <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
Reviewed-by: Ard Biesheuvel <[email protected]>
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
On Sat, Feb 08, 2020 at 07:54:39AM +0000, Ard Biesheuvel wrote:
> On Sat, 8 Feb 2020 at 02:02, Kees Cook <[email protected]> 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/[email protected]
> > Fixes: c4a84ae39b4a5 ("ARM: 8322/1: keep .text and .fixup regions closer together")
> > Cc: [email protected]
> > Reported-by: Nathan Chancellor <[email protected]>
> > Reported-by: Manoj Gupta <[email protected]>
> > Debugged-by: Nick Desaulniers <[email protected]>
> > Signed-off-by: Kees Cook <[email protected]>
>
> Reviewed-by: Ard Biesheuvel <[email protected]>
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
--
Kees Cook
On Sat, Feb 8, 2020 at 9:55 AM Kees Cook <[email protected]> wrote:
>
> On Sat, Feb 08, 2020 at 07:54:39AM +0000, Ard Biesheuvel wrote:
> > On Sat, 8 Feb 2020 at 02:02, Kees Cook <[email protected]> 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/[email protected]
> > > Fixes: c4a84ae39b4a5 ("ARM: 8322/1: keep .text and .fixup regions closer together")
> > > Cc: [email protected]
> > > Reported-by: Nathan Chancellor <[email protected]>
> > > Reported-by: Manoj Gupta <[email protected]>
> > > Debugged-by: Nick Desaulniers <[email protected]>
> > > Signed-off-by: Kees Cook <[email protected]>
> >
> > Reviewed-by: Ard Biesheuvel <[email protected]>
>
> 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 <[email protected]>
>
> -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
--
Thanks,
~Nick Desaulniers