Re: [PATCH] x86/boot: Refuse to build with data relocations
On Thu, May 19, 2016 at 11:41 PM, Ingo Molnar wrote: > > * Kees Cook wrote: > >> On Wed, May 18, 2016 at 4:29 AM, Ingo Molnar wrote: >> > >> > * 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. :) >> > >> > 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 > 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
* Kees Cook wrote: > On Wed, May 18, 2016 at 4:29 AM, Ingo Molnar wrote: > > > > * 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. :) > > > > 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 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
On Wed, May 18, 2016 at 4:29 AM, Ingo Molnar wrote: > > * 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. :) > > 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
* 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. :) 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
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
On Tue, May 17, 2016 at 12:56 PM, H. Peter Anvin 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 >> 006c7422 0007000a R_X86_64_32 006c7420 .data + 0 >> 006c74b0 00c90001 R_X86_64_64 006c4100 efi_call + 0 >> +006c74e0 00030001 R_X86_64_64 006bc9c0 .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
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 > 006c7422 0007000a R_X86_64_32 006c7420 .data + 0 > 006c74b0 00c90001 R_X86_64_64 006c4100 efi_call + 0 > +006c74e0 00030001 R_X86_64_64 006bc9c0 .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
On Tue, May 17, 2016 at 5:31 AM, H. Peter Anvin wrote: > On 05/17/16 01:13, Kees Cook wrote: >> On Mon, May 16, 2016 at 3:30 AM, Ingo Molnar wrote: >>> >>> * H. Peter Anvin 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 006c7422 0007000a R_X86_64_32 006c7420 .data + 0 006c74b0 00c90001 R_X86_64_64 006c4100 efi_call + 0 +006c74e0 00030001 R_X86_64_64 006bc9c0 .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
On 05/17/16 01:13, Kees Cook wrote: > On Mon, May 16, 2016 at 3:30 AM, Ingo Molnar wrote: >> >> * H. Peter Anvin 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
On Mon, May 16, 2016 at 3:30 AM, Ingo Molnar wrote: > > * H. Peter Anvin 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
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
Re: [PATCH] x86/boot: Refuse to build with data relocations
* H. Peter Anvin 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
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
On Thu, May 12, 2016 at 3:29 PM, H. Peter Anvin wrote: > On May 12, 2016 1:31:04 PM PDT, Kees Cook 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 >>Signed-off-by: Kees Cook >>--- >> 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
On May 12, 2016 1:31:04 PM PDT, Kees Cook 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 >Signed-off-by: Kees Cook >--- > 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.
[PATCH] x86/boot: Refuse to build with data relocations
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 Signed-off-by: Kees Cook --- 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