|
Message-ID: <202006041525.CB0293F898@keescook> Date: Thu, 4 Jun 2020 15:37:58 -0700 From: Kees Cook <keescook@...omium.org> To: Kristen Carlson Accardi <kristen@...ux.intel.com> Cc: tglx@...utronix.de, mingo@...hat.com, bp@...en8.de, Jonathan Corbet <corbet@....net>, x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>, arjan@...ux.intel.com, linux-kernel@...r.kernel.org, kernel-hardening@...ts.openwall.com, rick.p.edgecombe@...el.com, Tony Luck <tony.luck@...el.com>, linux-doc@...r.kernel.org Subject: Re: [PATCH v2 7/9] x86: Add support for function granular KASLR On Thu, Jun 04, 2020 at 10:27:24AM -0700, Kristen Carlson Accardi wrote: > On Thu, 2020-05-21 at 14:08 -0700, Kees Cook wrote: > > On Thu, May 21, 2020 at 09:56:38AM -0700, Kristen Carlson Accardi > > wrote: > > > [...] > > > +/* > > > + * This is an array of pointers to sections headers for randomized > > > sections > > > + */ > > > +Elf64_Shdr **sections; > > > > Given the creation of the Elf_Shdr etc macros in the header, should > > all > > of the Elf64 things in here be updated to use the size-agnostic > > macros? > > (Is anyone actually going to run a 32-bit kernel with fgkaslr some > > day?) > > I suppose it's not impossible, just ... not useful. I will make the > update. Yeah. I figure it might just look cleaner? Or, I guess, drop the other macros? I guess I wasn't sure why those macros got touched and then didn't get used. > > > [...] > > > + /* > > > + * we are going to need to regenerate the markers table, which is a > > > + * table of offsets into the compressed stream every 256 symbols. > > > + * this code copied almost directly from scripts/kallsyms.c > > > + */ > > > > Can any of this kallsyms sorting code be reasonably reused instead > > of copy/pasting? Or is this mostly novel in that it's taking the > > output > > of that earlier sort, which isn't the input format scripts/kallsyms.c > > uses? > > No - I cut out only code blocks from scripts/kallsyms.c, but there was > no neat way to reuse entire functions without majorly refactoring a lot > of stuff in scripts/kallsyms.c Hmm. I wonder if there might be a way to carve out what you need into lib/ and then #include it as a separate compilation unit? I'm always worried about cut/pasted code getting out of sync. > > > [...] > > > +#include "../../../../lib/extable.c" > > > > Now that the earlier linking glitches have been sorted out, I wonder if > > it might be nicer to add this as a separate compilation unit, similar to > > how early_serial_console.c is done? (Or, I guess, more specifically, why > > can't this be in utils.c?) > > The problem with putting this in utils.c was because there was an > inline function (static) that I use that is defined in extable.c > (ex_to_insn). If I move this to utils.c I'm not sure how to keep re- > using this inline function without modifying it with a define like > STATIC. I thought it was cleaner to just leave it alone and do it this > way. Hm, I see. I guess if this becomes fragile in the future, the special inlines can be moved to lib/extable.h and then things can be carved out into a separate compilation unit. > > > [...] > > > + if (!strcmp(sname, ".text")) { > > > + text = s; > > > + continue; > > > + } > > > > Check text is still NULL here? > > > > Also, why the continue? This means the section isn't included in the > > sections[] array? (Obviously things still work, but I don't > > understand > > why.) > > I don't include .text in the sections[] array because sections[] is > only for sections to be randomized, and we don't randomize .text. Yeah, I got myself confused there originally. I actuallyed realized my mistake on this later when I was explaining how FGKASLR worked in the thread with tglx. :) > > > + > > > + if (!strcmp(sname, ".data..percpu")) { > > > + /* get start addr for later */ > > > + percpu = s; > > > > Similar, check percpu is still NULL here? > > > > Also, is a "continue" intended here? (This is kind of the reverse of > > the "continue" question above.) I think you get the same result > > during > > the next "if", but I was expecting this block to look like the .text > > test above. > > You are right, I could have put a continue here and saved the next > compare. Cool; yeah, it was just a "wait, is that right?" when looking at it. > > > diff --git a/arch/x86/boot/compressed/misc.c > > > b/arch/x86/boot/compressed/misc.c > > > index 9652d5c2afda..5f08922fd12a 100644 > > > --- a/arch/x86/boot/compressed/misc.c > > > +++ b/arch/x86/boot/compressed/misc.c > > > @@ -26,9 +26,6 @@ > > > * it is not safe to place pointers in static structures. > > > */ > > > > > > -/* Macros used by the included decompressor code below. */ > > > -#define STATIC static > > > > Can you split the STATIC macro rearrangement out into a separate > > patch? > > I think it deserves a dedicated commit log to explain why it's > > changing > > the way it is (i.e. we end up with "STATIC" no longer meaning > > "static" > > in misc.c now > > This change was made to fix the issue of having malloc_ptr be declared > locally rather than globally - (that weird problem I was having that > made it so I had to #include all the .c files until I figured out what > the issue was. If I separate out the change, then I feel like the > commit doesn't make sense out of this context. What if I put a big > comment in misc.h? I think it's fine to split it out with a commit log describing what it's _about_ to fix, "without this, any other code using misc.h ..." and/or "in the next patches we'll need this because ..." > > > [...] > > > + (void)adjust_address(&extended); > > > > I don't think "(void)" needed here? > > I can delete it - it's not "needed", but was done to show an > intentional discard of the return value from adjust_address. The code style in the kernel seems to be to not include these (since it's a call-site override) and instead use __must_check on the function when the results MUST be checked. This way, all call-sites suddenly light up when the attribute gets added and we don't end up with places that might get masked, etc. > The rest of your feedback incorporated into v3. Awesome! -- Kees Cook
Powered by blists - more mailing lists
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.