LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Alexander Lobakin <alexandr.lobakin@intel.com>
To: Kees Cook <keescook@chromium.org>
Cc: Alexander Lobakin <alexandr.lobakin@intel.com>,
	linux-hardening@vger.kernel.org,
	Kristen C Accardi <kristen.c.accardi@intel.com>,
	Kristen Carlson Accardi <kristen@linux.intel.com>,
	Masahiro Yamada <masahiroy@kernel.org>,
	"H. Peter Anvin" <hpa@zytor.com>, Jessica Yu <jeyu@kernel.org>,
	Nathan Chancellor <nathan@kernel.org>,
	Nick Desaulniers <ndesaulniers@google.com>,
	Marios Pomonis <pomonis@google.com>,
	Sami Tolvanen <samitolvanen@google.com>,
	Tony Luck <tony.luck@intel.com>, Ard Biesheuvel <ardb@kernel.org>,
	Jesse Brandeburg <jesse.brandeburg@intel.com>,
	Lukasz Czapnik <lukasz.czapnik@intel.com>,
	Marta A Plantykow <marta.a.plantykow@intel.com>,
	Michal Kubiak <michal.kubiak@intel.com>,
	Michal Swiatkowski <michal.swiatkowski@intel.com>,
	linux-kbuild@vger.kernel.org, linux-arch@vger.kernel.org,
	linux-kernel@vger.kernel.org, clang-built-linux@googlegroups.com
Subject: Re: [PATCH v6 kspp-next 00/22] Function Granular KASLR
Date: Fri,  3 Sep 2021 13:19:07 +0200	[thread overview]
Message-ID: <20210903111907.136-1-alexandr.lobakin@intel.com> (raw)
In-Reply-To: <202109011815.1C439A6DA9@keescook>

> From: Kees Cook <keescook@chromium.org>
> Date: Wed, 1 Sep 2021 18:36:59 -0700
> 
> On Wed, Sep 01, 2021 at 12:36:58PM +0200, Alexander Lobakin wrote:
> > Without FG-KASLR, we have only one .text section, and the total
> > section number is relatively small.
> > With FG-KASLR enabled, we have 40K+ separate text sections (I have
> > 40K on a setup with ClangLTO and ClangCFI and about 48K on a
> > "regular" one) and each of them is described in the ELF header. Plus
> > a separate .rela.text section for every single of them. That's the
> > main reason of the size increases.
> 
> If you have the size comparisons handy, I'd love to see them. My memory
> from v5 was that none of that end up in-core. And in that case, why
> limit the entropy of the resulting layout?

My testing machine is down for now, but I could send a size
comparison later. It's something about 10 Mb of uncompressed kernel
between 1 and 4 fps or so.

> > We still have LD_ORPHAN_WARN on non-FG-KASLR builds, but we also
> > have a rather different set of sections with FG-KASLR enabled. For
> > example, I noticed the appearing of .symtab_shndx section only in
> > virtue of LD_ORPHAN_WARN. So it's kinda not the same.
> 
> Agreed: I'd rather have LD_ORPHAN_WARN always enabled.
> 
> > I don't see a problem in this extra minute. FG-KASLR is all about
> 
> But not at this cost. Maybe the x86 maintainers will disagree, but I see
> this as a prohibitive cost to doing development work under FGKASLR, and
> if we expect this to become the default in distros, no one is going to
> be happy with that change. Link time dominates the partial rebuild time,
> so my opinion is that it should not be so inflated if not absolutely
> needed. Perhaps once the link time bugs in ld.bfd and ld.lld get fixed,
> but not now.

I don't think FG-KASLR will be enabled by default in distros. Apart
from linking time, it also increases cache misses a lot, and when
it comes to performance critical usecases like high-speed servers
and datacenters, I don't believe their maintainers would consider
FG-KASLR.
Speaking about distros, almost no build systems to my knowledge use
partial building, so this is only a downside for developers.

> > security, and you often pay something for this. We already have a
> > size increase, and a small delay while booting, and we can't get
> > rid of them. With orphan sections you leave a space for potentional
> 
> There's a difference between development time costs and run time costs.
> I don't think the LD_ORPHAN_WARN coverage is worth it in this case.
> 
> Either way, we need to fix the linker.

I agree on that, I was surprised both BFD and LLD choke on big LD
scripts.

> > flaws of the code, linker and/or linker script, which is really
> > unwanted in case of a security feature.
> > After all, ClangLTO increases the linking time at lot, and
> > TRIM_UNUSED_KSYMS builds almost the entire kernel two times in a
> > row, but nobody complains about this as there's nothing we can do
> > with it and it's the price you pay for the optimizations, so again,
> > I don't see a problem here.
> 
> I get what you mean with regard to getting the perfect situation, but
> the kernel went 29 years without LD_ORPHAN_WARN. :) Anyway, we'll see
> what other folks think, I guess.

Also agree, let's wait for more opinions on that, I'm open to
everything.

> > I still don't get why you're trying to split this series into two.
> > It's been almost a year since v5 was published, I doubt you can get
> > "basic FG-KASLR" accepted quickly just because it was reviewed back
> > then.
> 
> Well, because it was blocked then by a single bug, and everything else
> you've described are distinct improvements on v5, so to me it makes
> sense to have it separated into those phases. I don't mean split the
> series, I mean rearrange the series so that a rebased v5 is at the
> start, and the improvements follow.
> 
> > I prefer to provide a full picture of what I'm trying to bring, so
> > the community could review it all and throw much more ideas and
> > stuff.
> 
> Understood. I am suggesting some ideas about how it might help with
> review. :)
> 
> > > > * It's now fully compatible with ClangLTO, ClangCFI,
> > > >   CONFIG_LD_ORPHAN_WARN and some more stuff landed since the last
> > > >   revision was published;
> > > 
> > > FWIW, v5 was was too. :) I didn't have to do anything to v5 to make it
> > > work with ClangLTO and ClangCFI.
> > 
> > Once again, repeating the thing I wrote earlier in our discussion:
> > ClangCFI, at least shadowed implementation, requires the first text
> > section of the module to be page-aligned and contain __cfi_check()
> > at the very beginning of this section. With FG-KASLR and without
> > special handling, this section gets randomized along with the
> > others, and ClangCFI either rejects almost all modules or panics
> > the kernel.
> 
> Ah-ha, thanks. I must have missed your answer to this earlier. I had
> probably done my initial v5 testing without modules.
> 
> > > Great, this is a good start. One place we saw problems in the past was
> > > with i386 build gotchas, so that'll need testing too.
> > 
> > For now, FG_KASLR for x86 depends on X86_64. We might relax this
> > dependency later after enough testing or whatsoever (like it's been
> > done for ClangLTO).
> 
> Yes, but we've had a history of making big patches that do _intend_ to
> break the i386 build, but they do anyway. Hence my question.
> 
> > > Sounds good. It might be easier to base the series on linux-next, so a
> > > smaller series. Though given the merge window just opened, it might make
> > > more sense for a v7 to be based on v5.15-rc2 in three weeks.
> > 
> > I don't usually base any series on linux-next, because it contains
> > all the changes from all "for-next" branches and repos, while the
> > series finally gets accepted to the specific repo based on just
> > v5.x-rc1 (sometimes on -rc2). This may bring additional apply/merge
> > problems.
> 
> Understood. I just find it confusing to include patches on lkml that
> already exist in a -next branch. Perhaps base on kbuild -next?

That's not a problem anymore I believe, since it doesn't hit 5.15
window, so the rebased v7 will be on top of 5.15-rc1 which will
already contain those Kbuild fixes.

> > > > Kees Cook (2):
> > > >   x86/boot: Allow a "silent" kaslr random byte fetch
> > > >   x86/boot/compressed: Avoid duplicate malloc() implementations
> > > 
> > > These two can get landed right away -- they're standalone fixes that
> > > can safely go in -tip.
> > > 
> > > > 
> > > > Kristen Carlson Accardi (9):
> > > >   x86: tools/relocs: Support >64K section headers
> > > 
> > > Same for this.
> > 
> > They make little to no sense for non-FG-KASLR systems. And none of
> > them are "pure" fixes.
> > The same could be said about e.g. ORC lookup patch, but again, it
> > makes no sense right now.
> 
> *shrug* They're trivial changes that have been reviewed before, so it
> seems like we can avoid resending them every time.
> 
> > > I suspect it'll still be easier to review this series as a rebase v5
> > > followed by the evolutionary improvements, since the "basic FGKASLR" has
> > > been reviewed in the past, and is fairly noninvasive. The changes for
> > > ASM, new .text rules, etc, make a lot more changes that I think would be
> > > nice to have separate so reasonable a/b testing can be done.
> > 
> > I don't see a point in testing it two times instead of just one, as
> > well as in delivering this feature in two halves. It sounds like
> > "let's introduce ClangLTO, but firstly only for modules, as LTO for
> > vmlinux requires changes in objtool code and a special handling for
> > the initcalls".
> > The changes you mentioned only seem invasive, in fact, they can
> > carry way less harm than the "basic FG-KASLR" itself.
> 
> Mostly it's a question of building on prior testing (v5 worked), so that
> new changes can be debugged if they cause problems. Regardless, it's
> been so long, perhaps it won't matter to other reviewers and they'll
> want to just start over from scratch.
> 
> -Kees
> 
> -- 
> Kees Cook

Thanks,
Al

      reply	other threads:[~2021-09-03 11:19 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-31 14:40 Alexander Lobakin
2021-08-31 14:40 ` [PATCH v6 kspp-next 01/22] kbuild: Fix TRIM_UNUSED_KSYMS with LTO_CLANG Alexander Lobakin
2021-08-31 14:40 ` [PATCH v6 kspp-next 02/22] kbuild: merge vmlinux_link() between the ordinary link and Clang LTO Alexander Lobakin
2021-08-31 14:40 ` [PATCH v6 kspp-next 03/22] kbuild: do not remove 'linux' link in scripts/link-vmlinux.sh Alexander Lobakin
2021-08-31 14:40 ` [PATCH v6 kspp-next 04/22] kbuild: merge vmlinux_link() between ARCH=um and other architectures Alexander Lobakin
2021-08-31 14:40 ` [PATCH v6 kspp-next 05/22] x86: tools/relocs: Support >64K section headers Alexander Lobakin
2021-08-31 14:40 ` [PATCH v6 kspp-next 06/22] x86/boot: Allow a "silent" kaslr random byte fetch Alexander Lobakin
2021-08-31 14:40 ` [PATCH v6 kspp-next 07/22] x86: Makefile: Add build and config option for CONFIG_FG_KASLR Alexander Lobakin
2021-08-31 14:41 ` [PATCH v6 kspp-next 08/22] Make sure ORC lookup covers the entire _etext - _stext Alexander Lobakin
2021-08-31 14:41 ` [PATCH v6 kspp-next 09/22] x86/tools: Add relative relocs for randomized functions Alexander Lobakin
2021-08-31 14:41 ` [PATCH v6 kspp-next 10/22] x86/boot/compressed: Avoid duplicate malloc() implementations Alexander Lobakin
2021-08-31 14:41 ` [PATCH v6 kspp-next 11/22] x86: Add support for function granular KASLR Alexander Lobakin
2021-08-31 14:41 ` [PATCH v6 kspp-next 12/22] linkage: add macros for putting ASM functions into own sections Alexander Lobakin
2021-08-31 14:41 ` [PATCH v6 kspp-next 13/22] x86: conditionally place regular ASM functions into separate sections Alexander Lobakin
2021-08-31 14:41 ` [PATCH v6 kspp-next 14/22] FG-KASLR: use a scripted approach to handle .text.* sections Alexander Lobakin
2021-08-31 14:41 ` [PATCH v6 kspp-next 15/22] kallsyms: Hide layout Alexander Lobakin
2021-08-31 14:41 ` [PATCH v6 kspp-next 16/22] livepatch: only match unique symbols when using fgkaslr Alexander Lobakin
2021-09-09 11:53   ` Miroslav Benes
2021-09-10 12:29     ` Alexander Lobakin
2021-08-31 14:41 ` [PATCH v6 kspp-next 17/22] x86/boot: allow FG-KASLR to be selected Alexander Lobakin
2021-08-31 14:41 ` [PATCH v6 kspp-next 18/22] arm64/crypto: conditionally place ASM functions into separate sections Alexander Lobakin
2021-08-31 14:41 ` [PATCH v6 kspp-next 19/22] module: Reorder functions Alexander Lobakin
2021-08-31 14:41 ` [PATCH v6 kspp-next 20/22] module: use a scripted approach for FG-KASLR Alexander Lobakin
2021-08-31 14:41 ` [PATCH v6 kspp-next 21/22] Documentation: add a documentation " Alexander Lobakin
2021-08-31 14:41 ` [PATCH v6 kspp-next 22/22] maintainers: add MAINTAINERS entry " Alexander Lobakin
2021-08-31 17:27 ` [PATCH v6 kspp-next 00/22] Function Granular KASLR Kees Cook
2021-09-01 10:36   ` Alexander Lobakin
2021-09-02  1:36     ` Kees Cook
2021-09-03 11:19       ` Alexander Lobakin [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210903111907.136-1-alexandr.lobakin@intel.com \
    --to=alexandr.lobakin@intel.com \
    --cc=ardb@kernel.org \
    --cc=clang-built-linux@googlegroups.com \
    --cc=hpa@zytor.com \
    --cc=jesse.brandeburg@intel.com \
    --cc=jeyu@kernel.org \
    --cc=keescook@chromium.org \
    --cc=kristen.c.accardi@intel.com \
    --cc=kristen@linux.intel.com \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-hardening@vger.kernel.org \
    --cc=linux-kbuild@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lukasz.czapnik@intel.com \
    --cc=marta.a.plantykow@intel.com \
    --cc=masahiroy@kernel.org \
    --cc=michal.kubiak@intel.com \
    --cc=michal.swiatkowski@intel.com \
    --cc=nathan@kernel.org \
    --cc=ndesaulniers@google.com \
    --cc=pomonis@google.com \
    --cc=samitolvanen@google.com \
    --cc=tony.luck@intel.com \
    --subject='Re: [PATCH v6 kspp-next 00/22] Function Granular KASLR' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).