On Tue, May 14, 2019 at 11:11 AM Kees Cook <keesc...@chromium.org> wrote: > > On Mon, May 13, 2019 at 04:50:05PM -0700, Nick Desaulniers wrote: > > On Mon, May 13, 2019 at 4:29 PM Nathan Chancellor > > <natechancel...@gmail.com> wrote: > > > > > > On Mon, May 13, 2019 at 03:21:09PM -0700, 'Nick Desaulniers' via Clang > > > Built Linux wrote: > > > > With CONFIG_LKDTM=y and make OBJCOPY=llvm-objcopy, llvm-objcopy errors: > > > > llvm-objcopy: error: --set-section-flags=.text conflicts with > > > > --rename-section=.text=.rodata > > > > > > > > Rather than support setting flags then renaming sections vs renaming > > > > then setting flags, it's simpler to just change both at the same time > > > > via --rename-section. > > > > > > > > This can be verified with: > > > > $ readelf -S drivers/misc/lkdtm/rodata_objcopy.o > > > > ... > > > > Section Headers: > > > > [Nr] Name Type Address Offset > > > > Size EntSize Flags Link Info Align > > > > ... > > > > [ 1] .rodata PROGBITS 0000000000000000 00000040 > > > > 0000000000000004 0000000000000000 A 0 0 4 > > > > ... > > > > > > > > Which shows in the Flags field that .text is now renamed .rodata, the > > > > append flag A is set, and the section is not flagged as writeable W. > > > > > > > > Link: https://github.com/ClangBuiltLinux/linux/issues/448 > > > > Reported-by: Nathan Chancellor <nathancha...@gmail.com> > > > > > > This should be natechancel...@gmail.com (although I think I do own that > > > email, just haven't been into it for 10+ years...) > > > > Sorry, I should have looked it up. I'll just fix this, my earlier > > mistake, and collect Kees's reviewed by tag in a v2 sent directly to > > GKH. > > Sounds good. > > > > I ran this script to see if there was any change for GNU objcopy and it > > > looks like .rodata's type gets changed, is this intentional? Otherwise, > > > this works for llvm-objcopy like you show. > > > > > > ----------- > > > > > > 1c1 > > > < There are 11 section headers, starting at offset 0x240: > > > --- > > > > There are 11 section headers, starting at offset 0x230: > > > 8c8 > > > < [ 1] .rodata PROGBITS 0000000000000000 00000040 > > > --- > > > > [ 1] .rodata NOBITS 0000000000000000 00000040 > > > 10c10 > > Oh, very good catch; thank you! > > > > > Interesting find. the .rodata of vmlinux itself is marked PROGBITS, > > so its curious that GNU binutils changes the "Type" after the rename. > > I doubt the code in question relies on NOBITS for this section. Kees, > > can you clarify? Jordan, do you know what the differences are between > > PROGBITS vs NOBITS? > > https://people.redhat.com/mpolacek/src/devconf2012.pdf seems to > > suggest NOBITS zero initializes data but I'm not sure that's what this > > code wants. > > Yes, the linker treats this as a zeroed section. My testing only checked that > the NX protection check kicked, but in looking at the memory, the failure > mode wouldn't have returned, because it got zeroed instead of seeing a "ret": > > Before the patch (with a two-byte target dump added): > > lkdtm: attempting bad execution at ffffffff986db2b0 > lkdtm: f3 c3 > > After the patch: > > lkdtm: attempting bad execution at ffffffff986db2b0 > lkdtm: 00 00 > > So, yes, this breaks the fall-back case and should not be used. It seems > that objcopy BFD breaks the PROGBITS in this case, but llvm-objcopy > does not...
I'm not sure I want to call it a bug in the initial implementation. I've filed: https://sourceware.org/bugzilla/show_bug.cgi?id=24554 for clarification. Jordan, I hope you can participate in any discussion there? > > $ objcopy --set-section-flags .text=alloc,readonly --rename-section > .text=.rodata rodata.o rodata_objcopy.o > $ readelf -S rodata_objcopy.o | grep -A1 \.rodata > [ 1] .rodata PROGBITS 0000000000000000 00000040 > 0000000000000002 0000000000000000 A 0 0 16 > > $ objcopy --rename-section .text=.rodata,alloc,readonly rodata.o > rodata_objcopy.o > $ readelf -S rodata_objcopy.o | grep -A1 \.rodata > [ 1] .rodata NOBITS 0000000000000000 00000040 > 0000000000000002 0000000000000000 A 0 0 16 > > $ llvm-objcopy --rename-section .text=.rodata,alloc,readonly rodata.o > rodata_objcopy-llvm.o > $ readelf -S rodata_objcopy-llvm.o | grep -A1 \.rodata > [ 1] .rodata PROGBITS 0000000000000000 00000040 > 0000000000000002 0000000000000000 A 0 0 16 > > > llvm-objcopy doesn't like doing both arguments at the same time, > and BFD gets it wrong when using the appended flags. How about just a > two-stage copy, like this? Yeah, I think this should work. What you wrote above + the link the bug I just filed would go well in a commit message, along with: Reviewed-by: Nick Desaulniers <ndesaulni...@google.com> > > diff --git a/drivers/misc/lkdtm/Makefile b/drivers/misc/lkdtm/Makefile > index 951c984de61a..715832c844c8 100644 > --- a/drivers/misc/lkdtm/Makefile > +++ b/drivers/misc/lkdtm/Makefile > @@ -14,9 +14,12 @@ KASAN_SANITIZE_stackleak.o := n > KCOV_INSTRUMENT_rodata.o := n > > OBJCOPYFLAGS := > +OBJCOPYFLAGS_rodata_flags.o := \ > + --set-section-flags .text=alloc,readonly > OBJCOPYFLAGS_rodata_objcopy.o := \ > - --set-section-flags .text=alloc,readonly \ > --rename-section .text=.rodata > -targets += rodata.o rodata_objcopy.o > -$(obj)/rodata_objcopy.o: $(obj)/rodata.o FORCE > +targets += rodata.o rodata_flags.o rodata_objcopy.o > +$(obj)/rodata_flags.o: $(obj)/rodata.o FORCE > + $(call if_changed,objcopy) > +$(obj)/rodata_objcopy.o: $(obj)/rodata_flags.o FORCE > $(call if_changed,objcopy) > > > -- > Kees Cook > > -- > You received this message because you are subscribed to the Google Groups > "Clang Built Linux" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to clang-built-linux+unsubscr...@googlegroups.com. > To post to this group, send email to clang-built-li...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/clang-built-linux/201905141041.C38DA1B305%40keescook. > For more options, visit https://groups.google.com/d/optout. -- Thanks, ~Nick Desaulniers
