Received-SPF: pass (google.com: domain of linux-kernel+bounces-83577-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <0443c7c2-1c3f-4cf8-940d-88306956832a@suse.com>
Date: Tue, 27 Feb 2024 17:13:35 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] x86, relocs: Ignore relocations in .notes section
Content-Language: en-US
To: Kees Cook <keescook@chromium.org>, Borislav Petkov <bp@alien8.de>
Cc: Guixiong Wei <guixiongwei@gmail.com>, Thomas Gleixner
 <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>,
 Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
 "H. Peter Anvin" <hpa@zytor.com>,
 "Peter Zijlstra (Intel)" <peterz@infradead.org>,
 Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
 Tony Luck <tony.luck@intel.com>,
 Kristen Carlson Accardi <kristen@linux.intel.com>,
 Boris Ostrovsky <boris.ostrovsky@oracle.com>,
 Stefano Stabellini <sstabellini@kernel.org>,
 Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>,
 Guixiong Wei <weiguixiong@bytedance.com>, Jann Horn <jannh@google.com>,
 Andrew Morton <akpm@linux-foundation.org>,
 Alexey Dobriyan <adobriyan@gmail.com>, Chris Wright <chrisw@sous-sol.org>,
 Jeremy Fitzhardinge <jeremy@xensource.com>,
 Roland McGrath <roland@redhat.com>, linux-kernel@vger.kernel.org,
 linux-hardening@vger.kernel.org
References: <20240222171840.work.027-kees@kernel.org>
From: =?UTF-8?B?SsO8cmdlbiBHcm/Dnw==?= <jgross@suse.com>
In-Reply-To: <20240222171840.work.027-kees@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

On 22.02.24 18:18, Kees Cook wrote:
> When building with CONFIG_XEN_PV=y, .text symbols are emitted into the
> .notes section so that Xen can find the "startup_xen" entry point. This
> information is used prior to booting the kernel, so relocations are not
> useful. In fact, performing relocations against the .notes section means
> that the KASLR base is exposed since /sys/kernel/notes is world-readable.
> 
> To avoid leaking the KASLR base without breaking unprivileged tools that
> are expecting to read /sys/kernel/notes, skip performing relocations in
> the .notes section. The values readable in .notes are then identical to
> those found in System.map.
> 
> Reported-by: Guixiong Wei <guixiongwei@gmail.com>
> Closes: https://lore.kernel.org/all/20240218073501.54555-1-guixiongwei@gmail.com/
> Fixes: 5ead97c84fa7 ("xen: Core Xen implementation")
> Fixes: da1a679cde9b ("Add /sys/kernel/notes")
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: x86@kernel.org
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Kristen Carlson Accardi <kristen@linux.intel.com>
> Cc: "Jürgen Groß" <jgross@suse.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Cc: Guixiong Wei <weiguixiong@bytedance.com>
> Cc: Jann Horn <jannh@google.com>
> ---
>   arch/x86/tools/relocs.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/arch/x86/tools/relocs.c b/arch/x86/tools/relocs.c
> index a3bae2b24626..0811fff23b9c 100644
> --- a/arch/x86/tools/relocs.c
> +++ b/arch/x86/tools/relocs.c
> @@ -733,6 +733,16 @@ static void walk_relocs(int (*process)(struct section *sec, Elf_Rel *rel,
>   		if (sec->shdr.sh_type != SHT_REL_TYPE) {
>   			continue;
>   		}
> +
> +		/*
> +		 * Do not perform relocations in .notes section; any
> +		 * values there are meant for pre-boot consumption (e.g.
> +		 * startup_xen).
> +		 */
> +		if (strcmp(sec_name(sec->shdr.sh_info), ".notes") == 0) {

Instead of a strcmp(), wouldnt't ...

> +			continue;
> +		}
> +
>   		sec_symtab  = sec->link;
>   		sec_applies = &secs[sec->shdr.sh_info];
>   		if (!(sec_applies->shdr.sh_flags & SHF_ALLOC)) {

.. a test of "sec_applies->shdr.sh_type == SHT_NOTE" work as well?

In the end I'm fine with both variants, so:

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen