[PATCH 6/7] Add /sys/kernel/notes

[PATCH 6/7] Add /sys/kernel/notes

[Roland McGrath]

This patch adds the /sys/kernel/notes magic file.  Reading this delivers
the contents of the kernel's .notes section.  This lets userland easily
glean any detailed information about the running kernel's build that was
stored there at compile time.

Signed-off-by: Roland McGrath <roland@redhat.com>
---
 kernel/ksysfs.c |   30 +++++++++++++++++++++++++++++-
 1 files changed, 29 insertions(+), 1 deletions(-)

diff --git a/kernel/ksysfs.c b/kernel/ksysfs.c
index 559deca..873fc17 100644
--- a/kernel/ksysfs.c
+++ b/kernel/ksysfs.c
@@ -62,6 +62,28 @@ static ssize_t kexec_crash_loaded_show(struct kset *kset, char *page)
 KERNEL_ATTR_RO(kexec_crash_loaded);
 #endif /* CONFIG_KEXEC */
 
+/*
+ * Make /sys/kernel/notes give the raw contents of our kernel .notes section.
+ */
+extern const char __start_notes __attribute__((weak));
+extern const char __stop_notes __attribute__((weak));
+#define	notes_size (&__stop_notes - &__start_notes)
+
+static ssize_t notes_read(struct kobject *kobj,
+			  char *buf, loff_t off, size_t count)
+{
+	memcpy(buf, &__start_notes + off, count);
+	return count;
+}
+
+static struct bin_attribute notes_attr = {
+	.attr = {
+		.name = "notes",
+		.mode = S_IRUGO,
+	},
+	.read = &notes_read,
+};
+
 decl_subsys(kernel, NULL, NULL);
 EXPORT_SYMBOL_GPL(kernel_subsys);
 
@@ -88,6 +110,12 @@ static int __init ksysfs_init(void)
 		error = sysfs_create_group(&kernel_subsys.kobj,
 					   &kernel_attr_group);
 
+	if (!error && notes_size > 0) {
+		notes_attr.size = notes_size;
+		error = sysfs_create_bin_file(&kernel_subsys.kobj,
+					      &notes_attr);
+	}
+
 	return error;
 }
 

Re: [PATCH 6/7] Add /sys/kernel/notes

[Linus Torvalds]

On Wed, 11 Jul 2007, Roland McGrath wrote:
> 
> This patch adds the /sys/kernel/notes magic file.  Reading this delivers
> the contents of the kernel's .notes section.  This lets userland easily
> glean any detailed information about the running kernel's build that was
> stored there at compile time.

Umm. You seem to make it readable by everybody. That's a mistake, I think. 
I don't know if there is anything security-conscious there, but just on 
general principles, I don't think we really would want normal users 
reading kernel configuration info, no?

		Linus

Re: [PATCH 6/7] Add /sys/kernel/notes

[Roland McGrath]

[I'd meant to send that from roland@redhat.com, so please correct any
followups.]

> Umm. You seem to make it readable by everybody. That's a mistake, I think. 
> I don't know if there is anything security-conscious there, but just on 
> general principles, I don't think we really would want normal users 
> reading kernel configuration info, no?

What I expect to find in notes I'd call kernel version and identification
info, not configuration info.  I don't think it's likely to be any more
revealing than "uname -v".  The main use I have in mind is to check
exactly which kernel binary you have, though indeed that is only of any
use to someone who can do something with kernel addresses and such.  It
is probably a lot less revealing on its own than /proc/config.gz or
/proc/kallsyms, which are world-readable.

It hadn't really occurred to me that the kernel binary would be
deliberately hidden from the user.  If you are doing that, indeed
/sys/kernel/noes is of no use to the user and you probably want to hide
it too.  Still, I think it is more useful that the default be to let an
unprivileged user see this as they can see /proc/kallsyms.  Both are
useful for the same sorts of things, i.e. making sense of kernel
addresses from oops logs or whatnot.  /sys/kernel/notes will be a part of
"eu-addr2line -k 0x12345" being reliable and automatic, for example (it
already works now with kernel-debuginfo installed, but this will help it
reliably figure out if you botched the install or something).


Thanks,
Roland

Re: [PATCH 6/7] Add /sys/kernel/notes

[Linus Torvalds]

On Wed, 11 Jul 2007, Roland McGrath wrote:
> 
> It hadn't really occurred to me that the kernel binary would be
> deliberately hidden from the user.

We don't want to have things like config info, or really preferably 
anything that can be used to generate kernel addresses.

Yeah, we've made that mistake before, and I'm not saying we are perfect 
here, but if we make this world-readable, I think it needs to guarantee it 
doesn't really give any rootkit people any new information.

And yes, I do think normal people shouldn't be able to read the vmlinuz 
binary, the same way they are generally not allowed to read /etc/grub.conf 
etc.

			Linus

Re: [PATCH 6/7] Add /sys/kernel/notes

[Roland McGrath]

> Yeah, we've made that mistake before, and I'm not saying we are perfect 
> here, but if we make this world-readable, I think it needs to guarantee it 
> doesn't really give any rootkit people any new information.

It will give them at least as reliable an identification of the kernel
binary as the "uname -rv" info if they have access to a set of candidate
known binaries to select from.  It's of no direct help at all in getting
anything like kernel addresses.  In practice, it is probably no easier for
a rootkit to use than "uname -rv" to pick an exploit for a particular known
distro kernel binary, just easier for legitimate debugging tools.  I think
it's not only more harmless than what you might call "past mistakes", but
is actually just plain harmless.  But I'm not really one to talk a man out
of his paranoia.

> And yes, I do think normal people shouldn't be able to read the vmlinuz 
> binary, the same way they are generally not allowed to read /etc/grub.conf 
> etc.

I have no special opinions about that, but I haven't seen an install where
the /boot files were not readable anyway.  But certainly in a chroot or
such, you won't have /boot at all but might have /proc and /sys.

All in all, this seems like a question of local policy.  Ideally the modes
would be flexibly chosen by admins, or else constrained more precisely by
SELinux policy or suchlike.  But I have no axe to grind on the subject with
this particular change.  I care more that the feature gets in and at least
root can use it, than about the permissions question.


Thanks,
Roland

Re: [PATCH 6/7] Add /sys/kernel/notes

[Linus Torvalds]

On Wed, 11 Jul 2007, Roland McGrath wrote:
> 
> It will give them at least as reliable an identification of the kernel
> binary as the "uname -rv" info if they have access to a set of candidate
> known binaries to select from.

Ok, that doesn't sound bad. Is the "note" something global for the whole 
thing, or are there per-object file notes that you can parse (ie can you 
figure out which modules are linked in some way?)

If it's basically just a set of hashes, I won't worry.

> I have no special opinions about that, but I haven't seen an install where
> the /boot files were not readable anyway.  But certainly in a chroot or
> such, you won't have /boot at all but might have /proc and /sys.

Yah. Not that we generally do perfectly here, but that's the kind of thing 
I'd prefer.

> All in all, this seems like a question of local policy.

Is there any reason for it to be readable by anybody but the sysadmin? 
It's not like you're likely to do things like kernel debugging without 
root privileges?

So if the *common* thing is that normally people have root who really 
care, and the distro could (if it wants to) just chmod the /sysfs file 
appropriately, maybe that would allow the best of both worlds? Just 
default to it being root-readable, but let the distro override it at 
boot-time?

		Linus

Re: [PATCH 6/7] Add /sys/kernel/notes

[Roland McGrath]

> Ok, that doesn't sound bad. Is the "note" something global for the whole 
> thing, or are there per-object file notes that you can parse (ie can you 
> figure out which modules are linked in some way?)

Usually there is one per final link, so one in vmlinux and one in each .ko.
There is nothing you can figure out from the bitstring except to match it
with known binaries.

> If it's basically just a set of hashes, I won't worry.

It's just one hash.  (Each .ko has one too, but I didn't bother with the
/sys/module/name/notes hack because it was too much trouble.)

> Is there any reason for it to be readable by anybody but the sysadmin? 
> It's not like you're likely to do things like kernel debugging without 
> root privileges?

I do kernel debugging all the time, but I am not usually root.  It's not
that I can't be root any time, of course--I'm usually using a machine where
I can sudo with no password.  But when I was young they taught me not to be
root whenever you don't have to, because I know I'm bound to break
something with a slip of the finger even when I surely ought to know
better.  So I don't run analysis tools as root, just use sudo when I need
to actually change something, or fetch a protected log to look at.

> So if the *common* thing is that normally people have root who really 
> care, and the distro could (if it wants to) just chmod the /sysfs file 
> appropriately, maybe that would allow the best of both worlds? Just 
> default to it being root-readable, but let the distro override it at 
> boot-time?

If distros are prepared for the notion of chmod's in /sys, that sounds
fine.  I presume you would want to uniformly change most of /sys, where
many things in /sys/module and /sys/kernel can be pretty revealing.  Like I
said, I have no special opinions about the permissions.  On all the systems
I use myself, everyone has ready access to the complete source and binaries
anyway (and can run dmesg).


Thanks,
Roland

Re: [PATCH 6/7] Add /sys/kernel/notes

[Linus Torvalds]

On Wed, 11 Jul 2007, Roland McGrath wrote:
> 
> > If it's basically just a set of hashes, I won't worry.
> 
> It's just one hash.  (Each .ko has one too, but I didn't bother with the
> /sys/module/name/notes hack because it was too much trouble.)

Ok, then I withdraw any objection. The only way that will give you any 
more information is if you already have practically the information to 
begin with, and you can just choose a particular case from a limited set 
of other cases.

No worries then,

		Linus

Re: [PATCH 6/7] Add /sys/kernel/notes

[Andrew Morton]

On Wed, 11 Jul 2007 12:04:34 -0700 (PDT)
Roland McGrath <roland@frob.com> wrote:

> +/*
> + * Make /sys/kernel/notes give the raw contents of our kernel .notes section.
> + */
> +extern const char __start_notes __attribute__((weak));
> +extern const char __stop_notes __attribute__((weak));
> +#define	notes_size (&__stop_notes - &__start_notes)
> +
> +static ssize_t notes_read(struct kobject *kobj,
> +			  char *buf, loff_t off, size_t count)
> +{
> +	memcpy(buf, &__start_notes + off, count);
> +	return count;
> +}
> +
> +static struct bin_attribute notes_attr = {
> +	.attr = {
> +		.name = "notes",
> +		.mode = S_IRUGO,
> +	},
> +	.read = &notes_read,
> +};
> +
>  decl_subsys(kernel, NULL, NULL);
>  EXPORT_SYMBOL_GPL(kernel_subsys);
>  
> @@ -88,6 +110,12 @@ static int __init ksysfs_init(void)
>  		error = sysfs_create_group(&kernel_subsys.kobj,
>  					   &kernel_attr_group);
>  
> +	if (!error && notes_size > 0) {
> +		notes_attr.size = notes_size;
> +		error = sysfs_create_bin_file(&kernel_subsys.kobj,
> +					      &notes_attr);
> +	}

I'm curios to know what happens if nobody defines __start_notes and
__end_notes.  We'll use the extern-attribute-weak thing, but those two
locations won't even get instantiated in vmlinux, I think.

And the code relies upon the difference between two non-existent
attribute-weak locations being zero.



akpm:/home/akpm> cat t.c

#include <stdio.h>

extern const char __start_notes __attribute__((weak));
extern const char __stop_notes __attribute__((weak));

main()
{
	int a = &__stop_notes - &__start_notes;
	
	printf("%d\n", a);
}

akpm:/home/akpm> gcc -g t.c
akpm:/home/akpm> ./a.out
0
akpm:/home/akpm> nm a.out|grep notes
         w __start_notes
         w __stop_notes

So it all works OK on this toolchain.  But is it _supposed_ to work?  Are
we venturing into unexplored binutils territory here?

Re: [PATCH 6/7] Add /sys/kernel/notes

[Roland McGrath]

> I'm curios to know what happens if nobody defines __start_notes and
> __end_notes.  We'll use the extern-attribute-weak thing, but those two
> locations won't even get instantiated in vmlinux, I think.
> 
> And the code relies upon the difference between two non-existent
> attribute-weak locations being zero.

The well-defined meaning of weak externs is that they resolve to zero if
undefined.  It relies on zero-zero being zero.  It relies on noone defining
one of __start_notes and __stop_notes but not both.


Thanks,
Roland

Re: [PATCH 6/7] Add /sys/kernel/notes

[Jeremy Fitzhardinge]

Roland McGrath wrote:
> This patch adds the /sys/kernel/notes magic file.  Reading this delivers
> the contents of the kernel's .notes section.  This lets userland easily
> glean any detailed information about the running kernel's build that was
> stored there at compile time.
>   

Is .notes an allocated section?  I didn't think it necessarily appeared 
in any of the PT_LOAD segments, because the linux/elfnote.h macros don't 
necessarily set "a" on the section.

I have a patch to always do this, and also convert all the note 
generation to C code rather than asm.  If your patch works for you, 
however, it should be completely orthogonal.

    J

Re: [PATCH 6/7] Add /sys/kernel/notes

[Roland McGrath]

> Is .notes an allocated section?  I didn't think it necessarily appeared 
> in any of the PT_LOAD segments, because the linux/elfnote.h macros don't 
> necessarily set "a" on the section.

The earlier patches in this series change the linker script to place it
appropriately and set the phdr.  It's allocated when its input sections are
allocated.  Current builds I've seen don't have any input note sections at
all.  My motivation is for the ld --build-id support, which generates an
allocated note section as if it were input.

> I have a patch to always do this, and also convert all the note 
> generation to C code rather than asm.  If your patch works for you, 
> however, it should be completely orthogonal.

What note generation do you mean?  The only explicit notes in asm that I
know of are in the vDSO images, not in vmlinux.  This patch has nothing to
do with any vDSO image's generation.


Thanks,
Roland

Re: [PATCH 6/7] Add /sys/kernel/notes

[Jeremy Fitzhardinge]

Roland McGrath wrote:
> The earlier patches in this series change the linker script to place it
> appropriately and set the phdr.  It's allocated when its input sections are
> allocated.  Current builds I've seen don't have any input note sections at
> all.  My motivation is for the ld --build-id support, which generates an
> allocated note section as if it were input.
>   

OK, I see.

>> I have a patch to always do this, and also convert all the note 
>> generation to C code rather than asm.  If your patch works for you, 
>> however, it should be completely orthogonal.
>>     
>
> What note generation do you mean?  The only explicit notes in asm that I
> know of are in the vDSO images, not in vmlinux.  This patch has nothing to
> do with any vDSO image's generation.
>   

I'm adding other notes for Xen, so I have some in my tree.  But the vdso 
ones could also be generated in C rather than asm.   I'm not sure what 
stage the patch to convert vsyscall-notes to using linux/elfnote.h is 
at; I think its in Andi's tree.

    J