[PATCH] x86/boot: Refuse to build with data relocations

[PATCH] x86/boot: Refuse to build with data relocations

[Kees Cook]

The compressed kernel is built with -fPIC/-fPIE so that it can run in any
location a bootloader happens to put it. However, since ELF relocation
processing is not happening (and all the relocation information has
already been stripped at link time), none of the code can use data
relocations (e.g. static assignments of pointers). This is already noted
in a warning comment at the top of misc.c, but this adds an explicit
check for the condition during the linking stage to block any such bugs
from appearing.

If this was in place with the earlier bug in pagetable.c, the build
would fail like this:

  ...
    CC      arch/x86/boot/compressed/pagetable.o
    DATAREL arch/x86/boot/compressed/vmlinux
  error: arch/x86/boot/compressed/pagetable.o has data relocations!
  make[2]: *** [arch/x86/boot/compressed/vmlinux] Error 1
  ...

A clean build shows the new check:

  ...
    CC      arch/x86/boot/compressed/pagetable.o
    DATAREL arch/x86/boot/compressed/vmlinux
    LD      arch/x86/boot/compressed/vmlinux
  ...

Suggested-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/x86/boot/compressed/Makefile | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
index cfdd8c3f8af2..25d477fcd5b4 100644
--- a/arch/x86/boot/compressed/Makefile
+++ b/arch/x86/boot/compressed/Makefile
@@ -85,7 +85,25 @@ vmlinux-objs-$(CONFIG_EFI_STUB) += $(obj)/eboot.o $(obj)/efi_stub_$(BITS).o \
 	$(objtree)/drivers/firmware/efi/libstub/lib.a
 vmlinux-objs-$(CONFIG_EFI_MIXED) += $(obj)/efi_thunk_$(BITS).o
 
+# The compressed kernel is built with -fPIC/-fPIE so that a boot loader
+# can place it anywhere in memory and it will still run. However, since
+# it is executed as-is without any ELF relocation processing performed
+# (and has already had all relocation sections stripped from the binary),
+# none of the code can use data relocations (e.g. static assignments of
+# pointer values), since they will be meaningless at runtime. This check
+# will refuse to link the vmlinux if any of these relocations are found.
+quiet_cmd_check_data_rel = DATAREL $@
+define cmd_check_data_rel
+	for obj in $(filter %.o,$^); do \
+		readelf -S $$obj | grep -qF .data.rel && { \
+			echo "error: $$obj has data relocations!" >&2; \
+			exit 1; \
+		} || true; \
+	done
+endef
+
 $(obj)/vmlinux: $(vmlinux-objs-y) FORCE
+	$(call if_changed,check_data_rel)
 	$(call if_changed,ld)
 	@:
 
-- 
2.6.3


-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH] x86/boot: Refuse to build with data relocations

[H. Peter Anvin]

On May 12, 2016 1:31:04 PM PDT, Kees Cook <keescook@chromium.org> wrote:
>The compressed kernel is built with -fPIC/-fPIE so that it can run in
>any
>location a bootloader happens to put it. However, since ELF relocation
>processing is not happening (and all the relocation information has
>already been stripped at link time), none of the code can use data
>relocations (e.g. static assignments of pointers). This is already
>noted
>in a warning comment at the top of misc.c, but this adds an explicit
>check for the condition during the linking stage to block any such bugs
>from appearing.
>
>If this was in place with the earlier bug in pagetable.c, the build
>would fail like this:
>
>  ...
>    CC      arch/x86/boot/compressed/pagetable.o
>    DATAREL arch/x86/boot/compressed/vmlinux
>  error: arch/x86/boot/compressed/pagetable.o has data relocations!
>  make[2]: *** [arch/x86/boot/compressed/vmlinux] Error 1
>  ...
>
>A clean build shows the new check:
>
>  ...
>    CC      arch/x86/boot/compressed/pagetable.o
>    DATAREL arch/x86/boot/compressed/vmlinux
>    LD      arch/x86/boot/compressed/vmlinux
>  ...
>
>Suggested-by: Ingo Molnar <mingo@kernel.org>
>Signed-off-by: Kees Cook <keescook@chromium.org>
>---
> arch/x86/boot/compressed/Makefile | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
>diff --git a/arch/x86/boot/compressed/Makefile
>b/arch/x86/boot/compressed/Makefile
>index cfdd8c3f8af2..25d477fcd5b4 100644
>--- a/arch/x86/boot/compressed/Makefile
>+++ b/arch/x86/boot/compressed/Makefile
>@@ -85,7 +85,25 @@ vmlinux-objs-$(CONFIG_EFI_STUB) += $(obj)/eboot.o
>$(obj)/efi_stub_$(BITS).o \
> 	$(objtree)/drivers/firmware/efi/libstub/lib.a
> vmlinux-objs-$(CONFIG_EFI_MIXED) += $(obj)/efi_thunk_$(BITS).o
> 
>+# The compressed kernel is built with -fPIC/-fPIE so that a boot
>loader
>+# can place it anywhere in memory and it will still run. However,
>since
>+# it is executed as-is without any ELF relocation processing performed
>+# (and has already had all relocation sections stripped from the
>binary),
>+# none of the code can use data relocations (e.g. static assignments
>of
>+# pointer values), since they will be meaningless at runtime. This
>check
>+# will refuse to link the vmlinux if any of these relocations are
>found.
>+quiet_cmd_check_data_rel = DATAREL $@
>+define cmd_check_data_rel
>+	for obj in $(filter %.o,$^); do \
>+		readelf -S $$obj | grep -qF .data.rel && { \
>+			echo "error: $$obj has data relocations!" >&2; \
>+			exit 1; \
>+		} || true; \
>+	done
>+endef
>+
> $(obj)/vmlinux: $(vmlinux-objs-y) FORCE
>+	$(call if_changed,check_data_rel)
> 	$(call if_changed,ld)
> 	@:
> 

It would be far better to warn on the *type* of relocations rather than in which section they feel.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

Re: [PATCH] x86/boot: Refuse to build with data relocations

[Kees Cook]

On Thu, May 12, 2016 at 3:29 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On May 12, 2016 1:31:04 PM PDT, Kees Cook <keescook@chromium.org> wrote:
>>The compressed kernel is built with -fPIC/-fPIE so that it can run in
>>any
>>location a bootloader happens to put it. However, since ELF relocation
>>processing is not happening (and all the relocation information has
>>already been stripped at link time), none of the code can use data
>>relocations (e.g. static assignments of pointers). This is already
>>noted
>>in a warning comment at the top of misc.c, but this adds an explicit
>>check for the condition during the linking stage to block any such bugs
>>from appearing.
>>
>>If this was in place with the earlier bug in pagetable.c, the build
>>would fail like this:
>>
>>  ...
>>    CC      arch/x86/boot/compressed/pagetable.o
>>    DATAREL arch/x86/boot/compressed/vmlinux
>>  error: arch/x86/boot/compressed/pagetable.o has data relocations!
>>  make[2]: *** [arch/x86/boot/compressed/vmlinux] Error 1
>>  ...
>>
>>A clean build shows the new check:
>>
>>  ...
>>    CC      arch/x86/boot/compressed/pagetable.o
>>    DATAREL arch/x86/boot/compressed/vmlinux
>>    LD      arch/x86/boot/compressed/vmlinux
>>  ...
>>
>>Suggested-by: Ingo Molnar <mingo@kernel.org>
>>Signed-off-by: Kees Cook <keescook@chromium.org>
>>---
>> arch/x86/boot/compressed/Makefile | 18 ++++++++++++++++++
>> 1 file changed, 18 insertions(+)
>>
>>diff --git a/arch/x86/boot/compressed/Makefile
>>b/arch/x86/boot/compressed/Makefile
>>index cfdd8c3f8af2..25d477fcd5b4 100644
>>--- a/arch/x86/boot/compressed/Makefile
>>+++ b/arch/x86/boot/compressed/Makefile
>>@@ -85,7 +85,25 @@ vmlinux-objs-$(CONFIG_EFI_STUB) += $(obj)/eboot.o
>>$(obj)/efi_stub_$(BITS).o \
>>       $(objtree)/drivers/firmware/efi/libstub/lib.a
>> vmlinux-objs-$(CONFIG_EFI_MIXED) += $(obj)/efi_thunk_$(BITS).o
>>
>>+# The compressed kernel is built with -fPIC/-fPIE so that a boot
>>loader
>>+# can place it anywhere in memory and it will still run. However,
>>since
>>+# it is executed as-is without any ELF relocation processing performed
>>+# (and has already had all relocation sections stripped from the
>>binary),
>>+# none of the code can use data relocations (e.g. static assignments
>>of
>>+# pointer values), since they will be meaningless at runtime. This
>>check
>>+# will refuse to link the vmlinux if any of these relocations are
>>found.
>>+quiet_cmd_check_data_rel = DATAREL $@
>>+define cmd_check_data_rel
>>+      for obj in $(filter %.o,$^); do \
>>+              readelf -S $$obj | grep -qF .data.rel && { \
>>+                      echo "error: $$obj has data relocations!" >&2; \
>>+                      exit 1; \
>>+              } || true; \
>>+      done
>>+endef
>>+
>> $(obj)/vmlinux: $(vmlinux-objs-y) FORCE
>>+      $(call if_changed,check_data_rel)
>>       $(call if_changed,ld)
>>       @:
>>
>
> It would be far better to warn on the *type* of relocations rather than in which section they feel.

I'm open to specific changes. What's the best way to detect what you want here?

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH] x86/boot: Refuse to build with data relocations

[H. Peter Anvin]

On 05/12/16 15:54, Kees Cook wrote:
>>
>> It would be far better to warn on the *type* of relocations rather than in which section they feel.
> 
> I'm open to specific changes. What's the best way to detect what you want here?
> 

Use readelf -r and look for inappropriate relocation types (which are
basically the same ones that we should have to muck with for the main
kernel in relocs.c.)

	-hpa

Re: [PATCH] x86/boot: Refuse to build with data relocations

[Ingo Molnar]

* H. Peter Anvin <hpa@zytor.com> wrote:

> On 05/12/16 15:54, Kees Cook wrote:
> >>
> >> It would be far better to warn on the *type* of relocations rather than in which section they feel.
> > 
> > I'm open to specific changes. What's the best way to detect what you want here?
> > 
> 
> Use readelf -r and look for inappropriate relocation types (which are
> basically the same ones that we should have to muck with for the main
> kernel in relocs.c.)

I suspect initially we are good if we don't allow any relocations in 
arch/x86/boot/compressed/vmlinux:

 fomalhaut:~/linux/linux> readelf -r arch/x86/boot/compressed/vmlinux | grep -q  'There are no relocations in this file' ; echo $?
 0

versus a regular object file with lots of relocations:

 fomalhaut:~/linux/linux> readelf -r arch/x86/built-in.o | grep -q 'There are no relocations in this file' ; echo $?
 1

I.e. the relevant portion of Kees's patch would do something like:

quiet_cmd_check_data_rel = DATAREL $@
define cmd_check_data_rel
       for obj in $(filter %.o,$^); do \
               readelf -r $$obj | grep -qF 'There are no relocations in this file' && exit 0 || { \
                       echo "error: $$obj has data relocations!" >&2; \
                       exit 1; \
               } \
       done
endef

(totally untested)

Agreed?

Thanks,

	Ingo

Re: [PATCH] x86/boot: Refuse to build with data relocations

[Kees Cook]

On Mon, May 16, 2016 at 3:30 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * H. Peter Anvin <hpa@zytor.com> wrote:
>
>> On 05/12/16 15:54, Kees Cook wrote:
>> >>
>> >> It would be far better to warn on the *type* of relocations rather than in which section they feel.
>> >
>> > I'm open to specific changes. What's the best way to detect what you want here?
>> >
>>
>> Use readelf -r and look for inappropriate relocation types (which are
>> basically the same ones that we should have to muck with for the main
>> kernel in relocs.c.)
>
> I suspect initially we are good if we don't allow any relocations in
> arch/x86/boot/compressed/vmlinux:

No, examining vmlinux is already too late, since it was built with a
linker script that explicitly drops all sections it doesn't know how
to handle (including these "bad" relocations).

The "good" relocations are those that are either PC-relative or
resolved during .o linking (For example, all the assembler .o files
have non-PC relocations that are fully resolved after getting linked
together into the vmlinux, since those are organized by section.)

So, we do specifically need to look at the _section_, not the _type_,
to draw any meaningful conclusion. I think my original patch is
correct and sufficient.

-Kees

>
>  fomalhaut:~/linux/linux> readelf -r arch/x86/boot/compressed/vmlinux | grep -q  'There are no relocations in this file' ; echo $?
>  0
>
> versus a regular object file with lots of relocations:
>
>  fomalhaut:~/linux/linux> readelf -r arch/x86/built-in.o | grep -q 'There are no relocations in this file' ; echo $?
>  1
>
> I.e. the relevant portion of Kees's patch would do something like:
>
> quiet_cmd_check_data_rel = DATAREL $@
> define cmd_check_data_rel
>        for obj in $(filter %.o,$^); do \
>                readelf -r $$obj | grep -qF 'There are no relocations in this file' && exit 0 || { \
>                        echo "error: $$obj has data relocations!" >&2; \
>                        exit 1; \
>                } \
>        done
> endef
>
> (totally untested)
>
> Agreed?
>
> Thanks,
>
>         Ingo



-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH] x86/boot: Refuse to build with data relocations

[H. Peter Anvin]

On 05/17/16 01:13, Kees Cook wrote:
> On Mon, May 16, 2016 at 3:30 AM, Ingo Molnar <mingo@kernel.org> wrote:
>>
>> * H. Peter Anvin <hpa@zytor.com> wrote:
>>
>>> On 05/12/16 15:54, Kees Cook wrote:
>>>>>
>>>>> It would be far better to warn on the *type* of relocations rather than in which section they feel.
>>>>
>>>> I'm open to specific changes. What's the best way to detect what you want here?
>>>>
>>>
>>> Use readelf -r and look for inappropriate relocation types (which are
>>> basically the same ones that we should have to muck with for the main
>>> kernel in relocs.c.)
>>
>> I suspect initially we are good if we don't allow any relocations in
>> arch/x86/boot/compressed/vmlinux:
> 
> No, examining vmlinux is already too late, since it was built with a
> linker script that explicitly drops all sections it doesn't know how
> to handle (including these "bad" relocations).

Either look at the inputs, or add the -q option to the link line
(--emit-relocs); that preserves the relocations into the output file
(the same we use to generate the relocation tables to be able to
relocate the kernel proper.)

> The "good" relocations are those that are either PC-relative or
> resolved during .o linking (For example, all the assembler .o files
> have non-PC relocations that are fully resolved after getting linked
> together into the vmlinux, since those are organized by section.)
> 
> So, we do specifically need to look at the _section_, not the _type_,
> to draw any meaningful conclusion. I think my original patch is
> correct and sufficient.

No.  This is completely wrong, because it *IS* the TYPE that matters.
You can end up with absolute relocations in text because someone is
careless writing assembly, for example.

Adding -q to the linker command line will preserve the relocations,
again, what we do to generate the relocation tables for the kernel
proper as well as the realmode stub.

I have to admit that peeking at the object code I have to wonder if we
wouldn't be better off just doing what we do for the kernel and just
relocate it explicitly.  We already have to relocate the GOT; the code
to do general relocation is no more complex and we already have done it
multiple times.

I *do* see a disturbing number of absolute references in the current
object; some of those are references to absolute symbols, however.  This
is where leveraging our existing relocs program would be awesome,
because it already has the necessary logic to deal with absolute symbols
and so on.

	-hpa

Re: [PATCH] x86/boot: Refuse to build with data relocations

[Kees Cook]

On Tue, May 17, 2016 at 5:31 AM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 05/17/16 01:13, Kees Cook wrote:
>> On Mon, May 16, 2016 at 3:30 AM, Ingo Molnar <mingo@kernel.org> wrote:
>>>
>>> * H. Peter Anvin <hpa@zytor.com> wrote:
>>>
>>>> On 05/12/16 15:54, Kees Cook wrote:
>>>>>>
>>>>>> It would be far better to warn on the *type* of relocations rather than in which section they feel.
>>>>>
>>>>> I'm open to specific changes. What's the best way to detect what you want here?
>>>>>
>>>>
>>>> Use readelf -r and look for inappropriate relocation types (which are
>>>> basically the same ones that we should have to muck with for the main
>>>> kernel in relocs.c.)
>>>
>>> I suspect initially we are good if we don't allow any relocations in
>>> arch/x86/boot/compressed/vmlinux:
>>
>> No, examining vmlinux is already too late, since it was built with a
>> linker script that explicitly drops all sections it doesn't know how
>> to handle (including these "bad" relocations).
>
> Either look at the inputs, or add the -q option to the link line
> (--emit-relocs); that preserves the relocations into the output file
> (the same we use to generate the relocation tables to be able to
> relocate the kernel proper.)

(FWIW, this adds about 20K to the resulting vmlinux.)

>> The "good" relocations are those that are either PC-relative or
>> resolved during .o linking (For example, all the assembler .o files
>> have non-PC relocations that are fully resolved after getting linked
>> together into the vmlinux, since those are organized by section.)
>>
>> So, we do specifically need to look at the _section_, not the _type_,
>> to draw any meaningful conclusion. I think my original patch is
>> correct and sufficient.
>
> No.  This is completely wrong, because it *IS* the TYPE that matters.
> You can end up with absolute relocations in text because someone is
> careless writing assembly, for example.
>
> Adding -q to the linker command line will preserve the relocations,
> again, what we do to generate the relocation tables for the kernel
> proper as well as the realmode stub.
>
> I have to admit that peeking at the object code I have to wonder if we
> wouldn't be better off just doing what we do for the kernel and just
> relocate it explicitly.  We already have to relocate the GOT; the code
> to do general relocation is no more complex and we already have done it
> multiple times.

I think if we can just avoid it, we can continue to not have to deal
with both the larger code and complexity. We haven't been using these
relocations, and there's no driving reason to have them now. This
whole thread was to just reliably detect them, since prior to this
attempt, there was just a warning in a comment.

> I *do* see a disturbing number of absolute references in the current
> object; some of those are references to absolute symbols, however.  This
> is where leveraging our existing relocs program would be awesome,
> because it already has the necessary logic to deal with absolute symbols
> and so on.

I spent some time last night initially adding ET_REL support to the
relocs tool, which is how I ended up deciding that the section was
more interesting than the type. Regardless, with the -q on vmlinux,
here's the output that changes between a regular build and one with an
intentionally bad relocation (from readelf -r):

-Relocation section '.rela.data' at offset 0x8cbb60 contains 2 entries:
+Relocation section '.rela.data' at offset 0x8cbb78 contains 3 entries:
   Offset          Info           Type           Sym. Value  Sym. Name + Addend
 0000006c7422  00070000000a R_X86_64_32  00000000006c7420 .data + 0
 0000006c74b0  00c900000001 R_X86_64_64  00000000006c4100 efi_call + 0
+0000006c74e0  000300000001 R_X86_64_64  00000000006bc9c0 .text + 4de0

And similarly, with my modified relocs tool, I see R_X86_64_64
relocations in the regular build too, so it didn't seem meaningful.
The relocations the tool flags are almost entirely in .head.text, and
the remaining two match the readelf report above:

reloc section   reloc type      symbol  symbol section
...
.data   R_X86_64_32     .data   .data
.data   R_X86_64_64     efi_call        .text

And a 32-bit build shows a ton of R_386_32 in .text.

So, I don't understand why some of these types are okay when others
aren't, and it seems like the .rel.data section on the .o files holds
the main clue.

Here's my change for relocs, FWIW:

https://git.kernel.org/cgit/linux/kernel/git/kees/linux.git/commit/?h=kaslr/relocs&id=d4c4ae0b86d0b895727609296207c4f87ecabf26

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH] x86/boot: Refuse to build with data relocations

[H. Peter Anvin]

On 05/17/16 06:53, Kees Cook wrote:
>>
>> Either look at the inputs, or add the -q option to the link line
>> (--emit-relocs); that preserves the relocations into the output file
>> (the same we use to generate the relocation tables to be able to
>> relocate the kernel proper.)
> 
> (FWIW, this adds about 20K to the resulting vmlinux.)
> 

Sure, but unless I'm completely out to sea this is only a matter during
the build; it is not carried into any actual product files.

>> I have to admit that peeking at the object code I have to wonder if we
>> wouldn't be better off just doing what we do for the kernel and just
>> relocate it explicitly.  We already have to relocate the GOT; the code
>> to do general relocation is no more complex and we already have done it
>> multiple times.
> 
> I think if we can just avoid it, we can continue to not have to deal
> with both the larger code and complexity. We haven't been using these
> relocations, and there's no driving reason to have them now. This
> whole thread was to just reliably detect them, since prior to this
> attempt, there was just a warning in a comment.

Well, actually we *do* deal with a fair bit of it.  I worry that the
current situation is a lot more fragile than we give it credit for, and
that makes me nervous (see below.)

>> I *do* see a disturbing number of absolute references in the current
>> object; some of those are references to absolute symbols, however.  This
>> is where leveraging our existing relocs program would be awesome,
>> because it already has the necessary logic to deal with absolute symbols
>> and so on.
> 
> I spent some time last night initially adding ET_REL support to the
> relocs tool, which is how I ended up deciding that the section was
> more interesting than the type. Regardless, with the -q on vmlinux,
> here's the output that changes between a regular build and one with an
> intentionally bad relocation (from readelf -r):
> 
> -Relocation section '.rela.data' at offset 0x8cbb60 contains 2 entries:
> +Relocation section '.rela.data' at offset 0x8cbb78 contains 3 entries:
>    Offset          Info           Type           Sym. Value  Sym. Name + Addend
>  0000006c7422  00070000000a R_X86_64_32  00000000006c7420 .data + 0
>  0000006c74b0  00c900000001 R_X86_64_64  00000000006c4100 efi_call + 0
> +0000006c74e0  000300000001 R_X86_64_64  00000000006bc9c0 .text + 4de0
> 
> And similarly, with my modified relocs tool, I see R_X86_64_64
> relocations in the regular build too, so it didn't seem meaningful.
> The relocations the tool flags are almost entirely in .head.text, and
> the remaining two match the readelf report above:
> 
> reloc section   reloc type      symbol  symbol section
> ...
> .data   R_X86_64_32     .data   .data
> .data   R_X86_64_64     efi_call        .text
> 
> And a 32-bit build shows a ton of R_386_32 in .text.
> 
> So, I don't understand why some of these types are okay when others
> aren't, and it seems like the .rel.data section on the .o files holds
> the main clue.

I think there is something way more subtle going on here, and it bothers
me exactly because it is subtle.  It may be that it is OK right now, but
there are alarm bells going on all over my brain on this.  I'm going to
stare at this for a bit and see if I can make sense of it; but if it
turns out that what we have is something really problematic it might be
better to apply a big hammer and avoid future breakage once and for all.

	-hpa

Re: [PATCH] x86/boot: Refuse to build with data relocations

[Kees Cook]

On Tue, May 17, 2016 at 12:56 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 05/17/16 06:53, Kees Cook wrote:
>>>
>>> Either look at the inputs, or add the -q option to the link line
>>> (--emit-relocs); that preserves the relocations into the output file
>>> (the same we use to generate the relocation tables to be able to
>>> relocate the kernel proper.)
>>
>> (FWIW, this adds about 20K to the resulting vmlinux.)
>>
>
> Sure, but unless I'm completely out to sea this is only a matter during
> the build; it is not carried into any actual product files.

Ah, yes, found it. You are totally correct:

OBJCOPYFLAGS_vmlinux.bin :=  -R .comment -S
$(obj)/vmlinux.bin: vmlinux FORCE
        $(call if_changed,objcopy)

The -S on vmlinux.bin drops all of it what we retained with the
earlier -q on vmlinux

>>> I have to admit that peeking at the object code I have to wonder if we
>>> wouldn't be better off just doing what we do for the kernel and just
>>> relocate it explicitly.  We already have to relocate the GOT; the code
>>> to do general relocation is no more complex and we already have done it
>>> multiple times.
>>
>> I think if we can just avoid it, we can continue to not have to deal
>> with both the larger code and complexity. We haven't been using these
>> relocations, and there's no driving reason to have them now. This
>> whole thread was to just reliably detect them, since prior to this
>> attempt, there was just a warning in a comment.
>
> Well, actually we *do* deal with a fair bit of it.  I worry that the
> current situation is a lot more fragile than we give it credit for, and
> that makes me nervous (see below.)

Right, the GOT is already adjusted, though it's easy, since it's
already a table. :)

>
>>> I *do* see a disturbing number of absolute references in the current
>>> object; some of those are references to absolute symbols, however.  This
>>> is where leveraging our existing relocs program would be awesome,
>>> because it already has the necessary logic to deal with absolute symbols
>>> and so on.
>>
>> I spent some time last night initially adding ET_REL support to the
>> relocs tool, which is how I ended up deciding that the section was
>> more interesting than the type. Regardless, with the -q on vmlinux,
>> here's the output that changes between a regular build and one with an
>> intentionally bad relocation (from readelf -r):
>>
>> -Relocation section '.rela.data' at offset 0x8cbb60 contains 2 entries:
>> +Relocation section '.rela.data' at offset 0x8cbb78 contains 3 entries:
>>    Offset          Info           Type           Sym. Value  Sym. Name + Addend
>>  0000006c7422  00070000000a R_X86_64_32  00000000006c7420 .data + 0
>>  0000006c74b0  00c900000001 R_X86_64_64  00000000006c4100 efi_call + 0
>> +0000006c74e0  000300000001 R_X86_64_64  00000000006bc9c0 .text + 4de0
>>
>> And similarly, with my modified relocs tool, I see R_X86_64_64
>> relocations in the regular build too, so it didn't seem meaningful.
>> The relocations the tool flags are almost entirely in .head.text, and
>> the remaining two match the readelf report above:
>>
>> reloc section   reloc type      symbol  symbol section
>> ...
>> .data   R_X86_64_32     .data   .data
>> .data   R_X86_64_64     efi_call        .text
>>
>> And a 32-bit build shows a ton of R_386_32 in .text.
>>
>> So, I don't understand why some of these types are okay when others
>> aren't, and it seems like the .rel.data section on the .o files holds
>> the main clue.
>
> I think there is something way more subtle going on here, and it bothers
> me exactly because it is subtle.  It may be that it is OK right now, but
> there are alarm bells going on all over my brain on this.  I'm going to
> stare at this for a bit and see if I can make sense of it; but if it
> turns out that what we have is something really problematic it might be
> better to apply a big hammer and avoid future breakage once and for all.

Sounds good. I would just like to decouple this from the KASLR
improvements. This fragility hasn't changed as a result of that work,
but I'd really like to have that series put to bed -- I've spent a lot
of time already cleaning up it and other areas of the compressed
kernel code. :)

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH] x86/boot: Refuse to build with data relocations

[H. Peter Anvin]

On 05/17/16 12:28, Kees Cook wrote:
>>
>> I think there is something way more subtle going on here, and it bothers
>> me exactly because it is subtle.  It may be that it is OK right now, but
>> there are alarm bells going on all over my brain on this.  I'm going to
>> stare at this for a bit and see if I can make sense of it; but if it
>> turns out that what we have is something really problematic it might be
>> better to apply a big hammer and avoid future breakage once and for all.
> 
> Sounds good. I would just like to decouple this from the KASLR
> improvements. This fragility hasn't changed as a result of that work,
> but I'd really like to have that series put to bed -- I've spent a lot
> of time already cleaning up it and other areas of the compressed
> kernel code. :)
> 

Agreed; this is orthogonal to kASLR.

	-hpa

Re: [PATCH] x86/boot: Refuse to build with data relocations

[Ingo Molnar]

* Kees Cook <keescook@chromium.org> wrote:

> > I think there is something way more subtle going on here, and it bothers me 
> > exactly because it is subtle.  It may be that it is OK right now, but there 
> > are alarm bells going on all over my brain on this.  I'm going to stare at 
> > this for a bit and see if I can make sense of it; but if it turns out that 
> > what we have is something really problematic it might be better to apply a big 
> > hammer and avoid future breakage once and for all.
> 
> Sounds good. I would just like to decouple this from the KASLR improvements. 
> This fragility hasn't changed as a result of that work, but I'd really like to 
> have that series put to bed -- I've spent a lot of time already cleaning up it 
> and other areas of the compressed kernel code. :)

So I disagree on that: while technically kASLR is independent of relocations, your 
series already introduced such a relocation bug and I don't want to further 
increase complexity via kASLR without first increasing robustness.

So could we try something to either detect or avoid such subtle and hard to debug 
relocation bugs in very early boot code?

Thanks,

	Ingo

Re: [PATCH] x86/boot: Refuse to build with data relocations

[Kees Cook]

On Wed, May 18, 2016 at 4:29 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Kees Cook <keescook@chromium.org> wrote:
>
>> > I think there is something way more subtle going on here, and it bothers me
>> > exactly because it is subtle.  It may be that it is OK right now, but there
>> > are alarm bells going on all over my brain on this.  I'm going to stare at
>> > this for a bit and see if I can make sense of it; but if it turns out that
>> > what we have is something really problematic it might be better to apply a big
>> > hammer and avoid future breakage once and for all.
>>
>> Sounds good. I would just like to decouple this from the KASLR improvements.
>> This fragility hasn't changed as a result of that work, but I'd really like to
>> have that series put to bed -- I've spent a lot of time already cleaning up it
>> and other areas of the compressed kernel code. :)
>
> So I disagree on that: while technically kASLR is independent of relocations, your
> series already introduced such a relocation bug and I don't want to further
> increase complexity via kASLR without first increasing robustness.

Well, in my defense, the bug was never actually reachable.

> So could we try something to either detect or avoid such subtle and hard to debug
> relocation bugs in very early boot code?

I've sent this (the readelf patch which detects the bug from the KASLR
series), but hpa wants to do a more comprehensive version. Could we
temporarily use my version of this, since it appears to accomplish at
least a subset of the new goal?

And on a related topic, how would you like me to send Thomas Garnier's
memory base randomization series? Pull request, or as a series like
I've done with the other KASLR improvements?

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH] x86/boot: Refuse to build with data relocations

[Ingo Molnar]

* Kees Cook <keescook@chromium.org> wrote:

> On Wed, May 18, 2016 at 4:29 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Kees Cook <keescook@chromium.org> wrote:
> >
> >> > I think there is something way more subtle going on here, and it bothers me
> >> > exactly because it is subtle.  It may be that it is OK right now, but there
> >> > are alarm bells going on all over my brain on this.  I'm going to stare at
> >> > this for a bit and see if I can make sense of it; but if it turns out that
> >> > what we have is something really problematic it might be better to apply a big
> >> > hammer and avoid future breakage once and for all.
> >>
> >> Sounds good. I would just like to decouple this from the KASLR improvements.
> >> This fragility hasn't changed as a result of that work, but I'd really like to
> >> have that series put to bed -- I've spent a lot of time already cleaning up it
> >> and other areas of the compressed kernel code. :)
> >
> > So I disagree on that: while technically kASLR is independent of relocations, your
> > series already introduced such a relocation bug and I don't want to further
> > increase complexity via kASLR without first increasing robustness.
> 
> Well, in my defense, the bug was never actually reachable.

Hm, the changelog says a crash/reboot might happen:

commit 434a6c9f90f7ab5ade619455df01ef5ebea533ee
Author: Kees Cook <keescook@chromium.org>
Date:   Mon May 9 13:22:04 2016 -0700

    x86/KASLR: Initialize mapping_info every time
    
    As it turns out, mapping_info DOES need to be initialized every
    time, because pgt_data address could be changed during kernel
    relocation. So it can not be build time assigned.
    
    Without this, page tables were not being corrected updated, which
    could cause reboots when a physical address beyond 2G was chosen.

is the changelog wrong?

> > So could we try something to either detect or avoid such subtle and hard to 
> > debug relocation bugs in very early boot code?
> 
> I've sent this (the readelf patch which detects the bug from the KASLR series), 
> but hpa wants to do a more comprehensive version. Could we temporarily use my 
> version of this, since it appears to accomplish at least a subset of the new 
> goal?

Yeah, that's fine with me.

> And on a related topic, how would you like me to send Thomas Garnier's memory 
> base randomization series? Pull request, or as a series like I've done with the 
> other KASLR improvements?

A series (size limited if necessary) would be nice!

Thanks,

	Ingo

Re: [PATCH] x86/boot: Refuse to build with data relocations

[Kees Cook]

On Thu, May 19, 2016 at 11:41 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Kees Cook <keescook@chromium.org> wrote:
>
>> On Wed, May 18, 2016 at 4:29 AM, Ingo Molnar <mingo@kernel.org> wrote:
>> >
>> > * Kees Cook <keescook@chromium.org> wrote:
>> >
>> >> > I think there is something way more subtle going on here, and it bothers me
>> >> > exactly because it is subtle.  It may be that it is OK right now, but there
>> >> > are alarm bells going on all over my brain on this.  I'm going to stare at
>> >> > this for a bit and see if I can make sense of it; but if it turns out that
>> >> > what we have is something really problematic it might be better to apply a big
>> >> > hammer and avoid future breakage once and for all.
>> >>
>> >> Sounds good. I would just like to decouple this from the KASLR improvements.
>> >> This fragility hasn't changed as a result of that work, but I'd really like to
>> >> have that series put to bed -- I've spent a lot of time already cleaning up it
>> >> and other areas of the compressed kernel code. :)
>> >
>> > So I disagree on that: while technically kASLR is independent of relocations, your
>> > series already introduced such a relocation bug and I don't want to further
>> > increase complexity via kASLR without first increasing robustness.
>>
>> Well, in my defense, the bug was never actually reachable.
>
> Hm, the changelog says a crash/reboot might happen:
>
> commit 434a6c9f90f7ab5ade619455df01ef5ebea533ee
> Author: Kees Cook <keescook@chromium.org>
> Date:   Mon May 9 13:22:04 2016 -0700
>
>     x86/KASLR: Initialize mapping_info every time
>
>     As it turns out, mapping_info DOES need to be initialized every
>     time, because pgt_data address could be changed during kernel
>     relocation. So it can not be build time assigned.
>
>     Without this, page tables were not being corrected updated, which
>     could cause reboots when a physical address beyond 2G was chosen.
>
> is the changelog wrong?

It's not wrong in some much as that it is talking about the future.
i.e. without this fix, the future changes would have failed to work. I
talked about it a bit in the v7 "0" email:

- found and fixed a typo in my refactorings that was obscuring a bug in
  my changes to the page table ident mapping code (physical address was
  never being added to identity maps).

So, since the later patches were never in the tree, this bug was never
exposed, and I found and fixed it before sending the v7 batch.

>> > So could we try something to either detect or avoid such subtle and hard to
>> > debug relocation bugs in very early boot code?
>>
>> I've sent this (the readelf patch which detects the bug from the KASLR series),
>> but hpa wants to do a more comprehensive version. Could we temporarily use my
>> version of this, since it appears to accomplish at least a subset of the new
>> goal?
>
> Yeah, that's fine with me.

Do you want me to resend the series? (It'd be unchanged from the v8
and accompanying reloc-detector patch):

https://lkml.org/lkml/2016/5/10/676 <- v8
https://lkml.org/lkml/2016/5/12/721 <- check for reloc sections

>> And on a related topic, how would you like me to send Thomas Garnier's memory
>> base randomization series? Pull request, or as a series like I've done with the
>> other KASLR improvements?
>
> A series (size limited if necessary) would be nice!

Okay, I'll send that out.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH] x86/boot: Refuse to build with data relocations

[Josh Poimboeuf]

On Thu, May 12, 2016 at 01:31:04PM -0700, Kees Cook wrote:
> diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
> index cfdd8c3f8af2..25d477fcd5b4 100644
> --- a/arch/x86/boot/compressed/Makefile
> +++ b/arch/x86/boot/compressed/Makefile
> @@ -85,7 +85,25 @@ vmlinux-objs-$(CONFIG_EFI_STUB) += $(obj)/eboot.o $(obj)/efi_stub_$(BITS).o \
>  	$(objtree)/drivers/firmware/efi/libstub/lib.a
>  vmlinux-objs-$(CONFIG_EFI_MIXED) += $(obj)/efi_thunk_$(BITS).o
>  
> +# The compressed kernel is built with -fPIC/-fPIE so that a boot loader
> +# can place it anywhere in memory and it will still run. However, since
> +# it is executed as-is without any ELF relocation processing performed
> +# (and has already had all relocation sections stripped from the binary),
> +# none of the code can use data relocations (e.g. static assignments of
> +# pointer values), since they will be meaningless at runtime. This check
> +# will refuse to link the vmlinux if any of these relocations are found.
> +quiet_cmd_check_data_rel = DATAREL $@
> +define cmd_check_data_rel
> +	for obj in $(filter %.o,$^); do \
> +		readelf -S $$obj | grep -qF .data.rel && { \
> +			echo "error: $$obj has data relocations!" >&2; \
> +			exit 1; \
> +		} || true; \
> +	done
> +endef

Why only data relocations?  If relocations haven't been applied yet,
wouldn't text relocations also be a problem?

-- 
Josh