Re: [RESEND RFC] kernel/ksysfs.c: restrict /sys/kernel/notes to root access

[Kees Cook <keescook@chromium.org>]

On Sun, Feb 18, 2024 at 08:47:03AM +0100, Greg KH wrote:
> On Sun, Feb 18, 2024 at 03:35:01PM +0800, Guixiong Wei wrote:
> > From: Guixiong Wei <weiguixiong@bytedance.com>
> > 
> > Restrict non-privileged user access to /sys/kernel/notes to
> > avoid security attack.
> > 
> > The non-privileged users have read access to notes. The notes
> > expose the load address of startup_xen. This address could be
> > used to bypass KASLR.
> 
> How can it be used to bypass it?
> 
> KASLR is, for local users, pretty much not an issue, as that's not what
> it protects from, only remote ones.
> 
> > For example, the startup_xen is built at 0xffffffff82465180 and
> > commit_creds is built at 0xffffffff810ad570 which could read from
> > the /boot/System.map. And the loaded address of startup_xen is
> > 0xffffffffbc265180 which read from /sys/kernel/notes. So the loaded
> > address of commit_creds is 0xffffffffbc265180 - (0xffffffff82465180
> >  - 0xffffffff810ad570) = 0xffffffffbaead570.
> 
> I've cc: the hardening list on this, I'm sure the developers there have
> opinions about this.

Oh eww, why is Xen spewing addresses into the notes section? (This must
be how it finds its entry point? But that would be before relocations
happen...)

But yes, I can confirm that relocations are done against the .notes
section at boot, so the addresses exposed in .notes is an immediate
KASLR offset exposure.

In /sys/kernel/notes (are there any tools to read this? I wrote my own...)

	type: 1
	name: Xen
	desc: 0xb4a711c0 0xffffffff

which matches a privileged read of /proc/kallsysms:

	ffffffffb4a711c0 T startup_xen

(and the hypercall_page too)

There are all coming from arch/x86/xen/xen-head.S:

        ELFNOTE(Xen, XEN_ELFNOTE_GUEST_OS,       .asciz "linux")
        ELFNOTE(Xen, XEN_ELFNOTE_GUEST_VERSION,  .asciz "2.6")
        ELFNOTE(Xen, XEN_ELFNOTE_XEN_VERSION,    .asciz "xen-3.0")
#ifdef CONFIG_XEN_PV
        ELFNOTE(Xen, XEN_ELFNOTE_VIRT_BASE,      _ASM_PTR __START_KERNEL_map)
        /* Map the p2m table to a 512GB-aligned user address. */
        ELFNOTE(Xen, XEN_ELFNOTE_INIT_P2M,       .quad (PUD_SIZE * PTRS_PER_PUD))
        ELFNOTE(Xen, XEN_ELFNOTE_ENTRY,          _ASM_PTR startup_xen)
...

Introduced in commit 5ead97c84fa7 ("xen: Core Xen implementation")

Exposed in commit da1a679cde9b ("Add /sys/kernel/notes")

Amazingly these both went in on the same release (v2.6.23, 2007). This
has been exposed for longer than KASLR has been upstream. :P

> 
> > Signed-off-by: Guixiong Wei <weiguixiong@bytedance.com>
> > ---
> >  kernel/ksysfs.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/ksysfs.c b/kernel/ksysfs.c
> > index b1292a57c2a5..09bc0730239b 100644
> > --- a/kernel/ksysfs.c
> > +++ b/kernel/ksysfs.c
> > @@ -199,7 +199,7 @@ static ssize_t notes_read(struct file *filp, struct kobject *kobj,
> >  static struct bin_attribute notes_attr __ro_after_init  = {
> >  	.attr = {
> >  		.name = "notes",
> > -		.mode = S_IRUGO,
> > +		.mode = S_IRUSR,
> >  	},
> >  	.read = &notes_read,
> >  };

Yes please.

Reviewed-by: Kees Cook <keescook@chromium.org>

I wonder if we should also remove relocations that are aimed at the
.notes section for good measure? If that had already been true, this
would have just given the same info as System.map.

> 
> No objection from me, but what userspace tool requires access to this
> file today?  Will it break if permissions are changed on it?
> 
> And what about the module notes files?  If you change one, shouldn't you
> change all?

Luckily all of _those_ contain what I'd expect: the Linux and
GNU.build-id notes, which are harmless. But if we're going to suddenly
have things appearing in here, let's make those root-only too.

-Kees

-- 
Kees Cook