linux-hardening - Re: [PATCH] x86, relocs: Ignore relocations in .notes section

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 27 Feb 2024 08:32:51 -0800
From: "H. Peter Anvin" <hpa@...or.com>
To: Jürgen Groß <jgross@...e.com>,
        Kees Cook <keescook@...omium.org>, Borislav Petkov <bp@...en8.de>
CC: Guixiong Wei <guixiongwei@...il.com>, Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
        "Peter Zijlstra (Intel)" <peterz@...radead.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Tony Luck <tony.luck@...el.com>,
        Kristen Carlson Accardi <kristen@...ux.intel.com>,
        Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        Stefano Stabellini <sstabellini@...nel.org>,
        Oleksandr Tyshchenko <oleksandr_tyshchenko@...m.com>,
        Guixiong Wei <weiguixiong@...edance.com>, Jann Horn <jannh@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Alexey Dobriyan <adobriyan@...il.com>,
        Chris Wright <chrisw@...s-sol.org>,
        Jeremy Fitzhardinge <jeremy@...source.com>,
        Roland McGrath <roland@...hat.com>, linux-kernel@...r.kernel.org,
        linux-hardening@...r.kernel.org
Subject: Re: [PATCH] x86, relocs: Ignore relocations in .notes section

On February 27, 2024 8:13:35 AM PST, "Jürgen Groß" <jgross@...e.com> wrote:
>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@...il.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@...omium.org>
>> ---
>> Cc: Borislav Petkov <bp@...en8.de>
>> Cc: Thomas Gleixner <tglx@...utronix.de>
>> Cc: Ingo Molnar <mingo@...hat.com>
>> Cc: Dave Hansen <dave.hansen@...ux.intel.com>
>> Cc: x86@...nel.org
>> Cc: "H. Peter Anvin" <hpa@...or.com>
>> Cc: "Peter Zijlstra (Intel)" <peterz@...radead.org>
>> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
>> Cc: Tony Luck <tony.luck@...el.com>
>> Cc: Kristen Carlson Accardi <kristen@...ux.intel.com>
>> Cc: "Jürgen Groß" <jgross@...e.com>
>> Cc: Boris Ostrovsky <boris.ostrovsky@...cle.com>
>> Cc: Stefano Stabellini <sstabellini@...nel.org>
>> Cc: Oleksandr Tyshchenko <oleksandr_tyshchenko@...m.com>
>> Cc: Guixiong Wei <weiguixiong@...edance.com>
>> Cc: Jann Horn <jannh@...gle.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@...e.com>
>
>
>Juergen

A type check would probably be better...