| Message ID | 20240222171840.work.027-kees@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86, relocs: Ignore relocations in .notes section | expand |
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
On February 27, 2024 8:13:35 AM PST, "Jürgen Groß" <jgross@suse.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@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 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.
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)) {
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(+)