* [PATCH] ARM: rename missed uaccess .fixup section
@ 2020-02-08 2:02 Kees Cook
2020-02-08 7:18 ` Nick Desaulniers
2020-02-08 7:54 ` Ard Biesheuvel
0 siblings, 2 replies; 5+ messages in thread
From: Kees Cook @ 2020-02-08 2:02 UTC (permalink / raw)
To: Nicolas Pitre
Cc: Nick Desaulniers, Manoj Gupta, Nathan Chancellor, Ard Biesheuvel,
Russell King - ARM Linux, clang-built-linux, Linux ARM,
linux-kernel
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/nycvar.YSQ.7.76.1912032147340.17114@knanqh.ubzr
Fixes: c4a84ae39b4a5 ("ARM: 8322/1: keep .text and .fixup regions closer together")
Cc: stable@vger.kernel.org
Reported-by: Nathan Chancellor <natechancellor@gmail.com>
Reported-by: Manoj Gupta <manojgupta@google.com>
Debugged-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] ARM: rename missed uaccess .fixup section
2020-02-08 2:02 [PATCH] ARM: rename missed uaccess .fixup section Kees Cook
@ 2020-02-08 7:18 ` Nick Desaulniers
2020-02-08 7:54 ` Ard Biesheuvel
1 sibling, 0 replies; 5+ messages in thread
From: Nick Desaulniers @ 2020-02-08 7:18 UTC (permalink / raw)
To: Kees Cook
Cc: Nicolas Pitre, Manoj Gupta, Nathan Chancellor, Ard Biesheuvel,
Russell King - ARM Linux, clang-built-linux, Linux ARM, LKML,
Arnd Bergmann
On Sat, Feb 8, 2020 at 3:02 AM Kees Cook <keescook@chromium.org> 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/nycvar.YSQ.7.76.1912032147340.17114@knanqh.ubzr
> 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: stable@vger.kernel.org
> Reported-by: Nathan Chancellor <natechancellor@gmail.com>
> Reported-by: Manoj Gupta <manojgupta@google.com>
> Debugged-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] ARM: rename missed uaccess .fixup section
2020-02-08 2:02 [PATCH] ARM: rename missed uaccess .fixup section Kees Cook
2020-02-08 7:18 ` Nick Desaulniers
@ 2020-02-08 7:54 ` Ard Biesheuvel
2020-02-08 8:55 ` Kees Cook
1 sibling, 1 reply; 5+ messages in thread
From: Ard Biesheuvel @ 2020-02-08 7:54 UTC (permalink / raw)
To: Kees Cook
Cc: Nicolas Pitre, Nick Desaulniers, Manoj Gupta, Nathan Chancellor,
Russell King - ARM Linux, clang-built-linux, Linux ARM,
Linux Kernel Mailing List
On Sat, 8 Feb 2020 at 02:02, Kees Cook <keescook@chromium.org> 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/nycvar.YSQ.7.76.1912032147340.17114@knanqh.ubzr
> Fixes: c4a84ae39b4a5 ("ARM: 8322/1: keep .text and .fixup regions closer together")
> Cc: stable@vger.kernel.org
> Reported-by: Nathan Chancellor <natechancellor@gmail.com>
> Reported-by: Manoj Gupta <manojgupta@google.com>
> Debugged-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] ARM: rename missed uaccess .fixup section
2020-02-08 7:54 ` Ard Biesheuvel
@ 2020-02-08 8:55 ` Kees Cook
2020-02-08 10:04 ` Nick Desaulniers
0 siblings, 1 reply; 5+ messages in thread
From: Kees Cook @ 2020-02-08 8:55 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Nicolas Pitre, Nick Desaulniers, Manoj Gupta, Nathan Chancellor,
Russell King - ARM Linux, clang-built-linux, Linux ARM,
Linux Kernel Mailing List
On Sat, Feb 08, 2020 at 07:54:39AM +0000, Ard Biesheuvel wrote:
> On Sat, 8 Feb 2020 at 02:02, Kees Cook <keescook@chromium.org> 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/nycvar.YSQ.7.76.1912032147340.17114@knanqh.ubzr
> > Fixes: c4a84ae39b4a5 ("ARM: 8322/1: keep .text and .fixup regions closer together")
> > Cc: stable@vger.kernel.org
> > Reported-by: Nathan Chancellor <natechancellor@gmail.com>
> > Reported-by: Manoj Gupta <manojgupta@google.com>
> > Debugged-by: Nick Desaulniers <ndesaulniers@google.com>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
>
> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] ARM: rename missed uaccess .fixup section
2020-02-08 8:55 ` Kees Cook
@ 2020-02-08 10:04 ` Nick Desaulniers
0 siblings, 0 replies; 5+ messages in thread
From: Nick Desaulniers @ 2020-02-08 10:04 UTC (permalink / raw)
To: Kees Cook
Cc: Ard Biesheuvel, Nicolas Pitre, Manoj Gupta, Nathan Chancellor,
Russell King - ARM Linux, clang-built-linux, Linux ARM,
Linux Kernel Mailing List
On Sat, Feb 8, 2020 at 9:55 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Sat, Feb 08, 2020 at 07:54:39AM +0000, Ard Biesheuvel wrote:
> > On Sat, 8 Feb 2020 at 02:02, Kees Cook <keescook@chromium.org> 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/nycvar.YSQ.7.76.1912032147340.17114@knanqh.ubzr
> > > Fixes: c4a84ae39b4a5 ("ARM: 8322/1: keep .text and .fixup regions closer together")
> > > Cc: stable@vger.kernel.org
> > > Reported-by: Nathan Chancellor <natechancellor@gmail.com>
> > > Reported-by: Manoj Gupta <manojgupta@google.com>
> > > Debugged-by: Nick Desaulniers <ndesaulniers@google.com>
> > > Signed-off-by: Kees Cook <keescook@chromium.org>
> >
> > Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
>
> 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 <ndesaulniers@google.com>
>
> -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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-02-08 10:05 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-08 2:02 [PATCH] ARM: rename missed uaccess .fixup section Kees Cook
2020-02-08 7:18 ` Nick Desaulniers
2020-02-08 7:54 ` Ard Biesheuvel
2020-02-08 8:55 ` Kees Cook
2020-02-08 10:04 ` Nick Desaulniers
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).