On Tue, May 17, 2016 at 12:56 PM, H. Peter Anvin <h...@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