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

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

[Guixiong Wei]

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.

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.

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,
 };
-- 
2.24.3 (Apple Git-128)

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

[Greg KH]

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.

> 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,
>  };

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?

thanks,

greg k-h

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

[Kees Cook]

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

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

[Guixiong Wei]

On 2024/2/18 17:04, Kees Cook wrote:
> 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.
That's a good idea, but it depends on whether the user space tool can 
accept the remove relocation address.
>> No objection from me, but what userspace tool requires access to this
>> file today?  Will it break if permissions are changed on it?
 From the exposed content, it seems that the main users are Xen-related 
tools. I add Xen list, developers should be able to provide some 
information.
>> And what about the module notes files?  If you change one, shouldn't you
>> change all?
 From what I currently know, the module note files do not expose any 
kernel symbol address, so there is no need for modification.
> 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.
Yes, but I also not sure whether the user space tools using this file 
can accept this permission modification.

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

[Jürgen Groß]

On 18.02.24 10:04, Kees Cook wrote:
> 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...)

Right. Xen is looking into the ELF-file to find the entry point of the
kernel (PV and PVH guest types only).

> 
> 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.

Relocations applied to the kernel when it has been started don't need
to cover the notes section as far as Xen is concerned.


Juergen

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

[Jann Horn]

On Sun, Feb 18, 2024 at 8:47 AM Greg KH <gregkh@linuxfoundation.org> 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.
>
> > 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,
> >  };
>
> No objection from me, but what userspace tool requires access to this
> file today?  Will it break if permissions are changed on it?

FWIW, https://codesearch.debian.net/search?q=%2Fsys%2Fkernel%2Fnotes&literal=1
shows that (from what I can tell) this is mostly used by stuff like
perf, libdwfl and systemtap for figuring out the kernel's build-id,
probably mostly for kernel profiling?

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

[Greg KH]

On Sat, Feb 17, 2024 at 10:19:32PM -0800, Guixiong Wei wrote:
> 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.
> 
> 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.
> 
> Signed-off-by: Guixiong Wei
> ---
> kernel/ksysfs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/ksysfs.c b/kernel/ksysfs.c
> index 1d4bc493b2f4..ccef642dc4c6 100644
> --- a/kernel/ksysfs.c
> +++ b/kernel/ksysfs.c
> @@ -241,7 +241,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,
> };
> -- 
> 2.20.1

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- Your patch is malformed (tabs converted to spaces, linewrapped, etc.)
  and can not be applied.  Please read the file,
  Documentation/process/email-clients.rst in order to fix this.

- Your patch does not have a Signed-off-by: line.  Please read the
  kernel file, Documentation/process/submitting-patches.rst and resend
  it after adding that line.  Note, the line needs to be in the body of
  the email, before the patch, not at the bottom of the patch or in the
  email signature.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot