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 <[email protected]>
Closes: https://lore.kernel.org/all/[email protected]/
Fixes: 5ead97c84fa7 ("xen: Core Xen implementation")
Fixes: da1a679cde9b ("Add /sys/kernel/notes")
Signed-off-by: Kees Cook <[email protected]>
---
Cc: Borislav Petkov <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: [email protected]
Cc: "H. Peter Anvin" <[email protected]>
Cc: "Peter Zijlstra (Intel)" <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: Kristen Carlson Accardi <[email protected]>
Cc: "Jürgen Groß" <[email protected]>
Cc: Boris Ostrovsky <[email protected]>
Cc: Stefano Stabellini <[email protected]>
Cc: Oleksandr Tyshchenko <[email protected]>
Cc: Guixiong Wei <[email protected]>
Cc: Jann Horn <[email protected]>
---
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) {
+ continue;
+ }
+
sec_symtab = sec->link;
sec_applies = &secs[sec->shdr.sh_info];
if (!(sec_applies->shdr.sh_flags & SHF_ALLOC)) {
--
2.34.1
On February 27, 2024 8:13:35 AM PST, "Jürgen Groß" <[email protected]> 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 <[email protected]>
>> Closes: https://lore.kernel.org/all/[email protected]/
>> Fixes: 5ead97c84fa7 ("xen: Core Xen implementation")
>> Fixes: da1a679cde9b ("Add /sys/kernel/notes")
>> Signed-off-by: Kees Cook <[email protected]>
>> ---
>> Cc: Borislav Petkov <[email protected]>
>> Cc: Thomas Gleixner <[email protected]>
>> Cc: Ingo Molnar <[email protected]>
>> Cc: Dave Hansen <[email protected]>
>> Cc: [email protected]
>> Cc: "H. Peter Anvin" <[email protected]>
>> Cc: "Peter Zijlstra (Intel)" <[email protected]>
>> Cc: Greg Kroah-Hartman <[email protected]>
>> Cc: Tony Luck <[email protected]>
>> Cc: Kristen Carlson Accardi <[email protected]>
>> Cc: "Jürgen Groß" <[email protected]>
>> Cc: Boris Ostrovsky <[email protected]>
>> Cc: Stefano Stabellini <[email protected]>
>> Cc: Oleksandr Tyshchenko <[email protected]>
>> Cc: Guixiong Wei <[email protected]>
>> Cc: Jann Horn <[email protected]>
>> ---
>> 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 <[email protected]>
>
>
>Juergen
A type check would probably be better...
On Tue, Feb 27, 2024 at 05:13:35PM +0100, J?rgen Gro? wrote:
> ... a test of "sec_applies->shdr.sh_type == SHT_NOTE" work as well?
Oh yeah! Thanks. I'll send a v2.
--
Kees Cook
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 <[email protected]>
> Closes: https://lore.kernel.org/all/[email protected]/
> Fixes: 5ead97c84fa7 ("xen: Core Xen implementation")
> Fixes: da1a679cde9b ("Add /sys/kernel/notes")
> Signed-off-by: Kees Cook <[email protected]>
> ---
> Cc: Borislav Petkov <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Dave Hansen <[email protected]>
> Cc: [email protected]
> Cc: "H. Peter Anvin" <[email protected]>
> Cc: "Peter Zijlstra (Intel)" <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Tony Luck <[email protected]>
> Cc: Kristen Carlson Accardi <[email protected]>
> Cc: "Jürgen Groß" <[email protected]>
> Cc: Boris Ostrovsky <[email protected]>
> Cc: Stefano Stabellini <[email protected]>
> Cc: Oleksandr Tyshchenko <[email protected]>
> Cc: Guixiong Wei <[email protected]>
> Cc: Jann Horn <[email protected]>
> ---
> 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 <[email protected]>
Juergen