linux-hardening.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Miroslav Benes <mbenes@suse.cz>
To: Alexander Lobakin <alexandr.lobakin@intel.com>
Cc: linux-hardening@vger.kernel.org,
	Kristen C Accardi <kristen.c.accardi@intel.com>,
	Kristen Carlson Accardi <kristen@linux.intel.com>,
	Kees Cook <keescook@chromium.org>,
	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,
	live-patching@vger.kernel.org
Subject: Re: [PATCH v6 kspp-next 16/22] livepatch: only match unique symbols when using fgkaslr
Date: Thu, 9 Sep 2021 13:53:35 +0200 (CEST)	[thread overview]
Message-ID: <alpine.LSU.2.21.2109091347400.20761@pobox.suse.cz> (raw)
In-Reply-To: <20210831144114.154-17-alexandr.lobakin@intel.com>

Hi,

On Tue, 31 Aug 2021, Alexander Lobakin wrote:

> From: Kristen Carlson Accardi <kristen@linux.intel.com>
> 
> If any type of function granular randomization is enabled, the sympos
> algorithm will fail, as it will be impossible to resolve symbols when
> there are duplicates using the previous symbol position.
> 
> Override the value of sympos to always be zero if fgkaslr is enabled for
> either the core kernel or modules, forcing the algorithm
> to require that only unique symbols are allowed to be patched.
> 
> Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com>
> ---
>  kernel/livepatch/core.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 335d988bd811..852bbfa9da7b 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -169,6 +169,17 @@ static int klp_find_object_symbol(const char *objname, const char *name,
>  	else
>  		kallsyms_on_each_symbol(klp_find_callback, &args);
>  
> +	/*
> +	 * If any type of function granular randomization is enabled, it
> +	 * will be impossible to resolve symbols when there are duplicates
> +	 * using the previous symbol position (i.e. sympos != 0). Override
> +	 * the value of sympos to always be zero in this case. This will
> +	 * force the algorithm to require that only unique symbols are
> +	 * allowed to be patched.
> +	 */
> +	if (IS_ENABLED(CONFIG_FG_KASLR))
> +		sympos = 0;
> +

I ran the live patching tests and no problem occurred, which is great. We 
do not have a test for old_sympos, which makes the testing less telling, 
but at least nothing blows up with the section randomization itself.

However, I want to reiterate what I wrote for the same patch in v5 
series.

The above hunk should work, but I wonder if we should make it more 
explicit. With the change the user will get the error with "unresolvable 
ambiguity for symbol..." if they specify sympos and the symbol is not 
unique. It could confuse them.

So, how about it making it something like

if (IS_ENABLED(CONFIG_FG_KASLR) || IS_ENABLED(CONFIG_MODULE_FG_KASLR))
        if (sympos) {
                pr_err("fgkaslr is enabled, specifying sympos for symbol '%s' in object '%s' does not work.\n",
                        name, objname);
                *addr = 0;
                return -EINVAL;
        }

? (there could be goto to the error out at the end of the function to 
save copy-pasting).

In that case, if sympos is not specified, the user will get the message 
which matches the reality. If the user specifies it, they will get the 
error in case of fgkaslr (no matter if the symbol is found or not).

What do you think?

Miroslav

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

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-31 14:40 [PATCH v6 kspp-next 00/22] Function Granular KASLR 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 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 [this message]
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
2021-09-01 17:16 ` Accardi, Kristen C

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=alpine.LSU.2.21.2109091347400.20761@pobox.suse.cz \
    --to=mbenes@suse.cz \
    --cc=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=live-patching@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 \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).