Re: [PATCH v3 1/7] kprobes: Make blacklist root user read only

[Masami Hiramatsu]

On Fri, 27 Apr 2018 09:04:12 +0200
Ingo Molnar  wrote:

> 
> * Masami Hiramatsu  wrote:
> 
> > Since the blacklist file indicates a sensitive address
> > information to reader, it should be restricted to the
> > root user.
> > 
> > Suggested-by: Thomas Richter 
> > Signed-off-by: Masami Hiramatsu 
> > ---
> >  kernel/kprobes.c |2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index ea619021d901..51096eece801 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -2621,7 +2621,7 @@ static int __init debugfs_kprobe_init(void)
> > if (!file)
> > goto error;
> >  
> > -   file = debugfs_create_file("blacklist", 0444, dir, NULL,
> > +   file = debugfs_create_file("blacklist", 0400, dir, NULL,
> > _kprobe_blacklist_ops);
> > if (!file)
> > goto error;
> 
> Note that in a typical Linux distro debugfs is already root-only:
> 
>   fomalhaut:~> ls -ld /sys/kernel/debug
>   drwx-- 28 root root 0 Apr 23 08:55 /sys/kernel/debug
> 
> but this change might make sense if debugfs is mounted in some other fashion.
> 
> But the patch looks incomplete, 'blacklist' is not the only word-readable 
> file in 
> the kprobes hierarchy. The kprobes directory itself, and the 'list' file is 
> readable as well:
> 
>   [root@fomalhaut ~]# ls -ld /sys/kernel/debug/kprobes
>   drwxr-xr-x 2 root root 0 Apr 23 08:55 /sys/kernel/debug/kprobes
> 
>   [root@fomalhaut ~]# ls -l /sys/kernel/debug/kprobes/
> 
>   -r--r--r-- 1 root root 0 Apr 23 08:55 blacklist
>   -rw--- 1 root root 0 Apr 23 08:55 enabled
>   -r--r--r-- 1 root root 0 Apr 23 08:55 list
> 
> So not just the blacklist should be 400 but 'list' as well, and the main 
> kprobes 
> directory as well.

OK, I'll mark it 0400 too.
For the kprobes directory, as Thomas pointed in the original thread,
that is currently debugfs API's limitation.

https://patchwork.kernel.org/patch/10364817/

We only have debugfs_create_dir() but it doesn't have "mode" flag.

struct dentry *debugfs_create_dir(const char *name, struct dentry *parent);

And doesn't care the parent mode bits.

Thanks,

-- 
Masami Hiramatsu 

Re: [PATCH v3 1/7] kprobes: Make blacklist root user read only

[Masami Hiramatsu]

On Fri, 27 Apr 2018 09:04:12 +0200
Ingo Molnar  wrote:

> 
> * Masami Hiramatsu  wrote:
> 
> > Since the blacklist file indicates a sensitive address
> > information to reader, it should be restricted to the
> > root user.
> > 
> > Suggested-by: Thomas Richter 
> > Signed-off-by: Masami Hiramatsu 
> > ---
> >  kernel/kprobes.c |2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index ea619021d901..51096eece801 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -2621,7 +2621,7 @@ static int __init debugfs_kprobe_init(void)
> > if (!file)
> > goto error;
> >  
> > -   file = debugfs_create_file("blacklist", 0444, dir, NULL,
> > +   file = debugfs_create_file("blacklist", 0400, dir, NULL,
> > _kprobe_blacklist_ops);
> > if (!file)
> > goto error;
> 
> Note that in a typical Linux distro debugfs is already root-only:
> 
>   fomalhaut:~> ls -ld /sys/kernel/debug
>   drwx-- 28 root root 0 Apr 23 08:55 /sys/kernel/debug
> 
> but this change might make sense if debugfs is mounted in some other fashion.
> 
> But the patch looks incomplete, 'blacklist' is not the only word-readable 
> file in 
> the kprobes hierarchy. The kprobes directory itself, and the 'list' file is 
> readable as well:
> 
>   [root@fomalhaut ~]# ls -ld /sys/kernel/debug/kprobes
>   drwxr-xr-x 2 root root 0 Apr 23 08:55 /sys/kernel/debug/kprobes
> 
>   [root@fomalhaut ~]# ls -l /sys/kernel/debug/kprobes/
> 
>   -r--r--r-- 1 root root 0 Apr 23 08:55 blacklist
>   -rw--- 1 root root 0 Apr 23 08:55 enabled
>   -r--r--r-- 1 root root 0 Apr 23 08:55 list
> 
> So not just the blacklist should be 400 but 'list' as well, and the main 
> kprobes 
> directory as well.

OK, I'll mark it 0400 too.
For the kprobes directory, as Thomas pointed in the original thread,
that is currently debugfs API's limitation.

https://patchwork.kernel.org/patch/10364817/

We only have debugfs_create_dir() but it doesn't have "mode" flag.

struct dentry *debugfs_create_dir(const char *name, struct dentry *parent);

And doesn't care the parent mode bits.

Thanks,

-- 
Masami Hiramatsu 

Re: [PATCH v3 1/7] kprobes: Make blacklist root user read only

[Masami Hiramatsu]

On Fri, 27 Apr 2018 08:56:11 +0200
Greg KH  wrote:

> On Fri, Apr 27, 2018 at 03:40:09PM +0900, Masami Hiramatsu wrote:
> > Since the blacklist file indicates a sensitive address
> > information to reader, it should be restricted to the
> > root user.
> > 
> > Suggested-by: Thomas Richter 
> > Signed-off-by: Masami Hiramatsu 
> > ---
> >  kernel/kprobes.c |2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> 
> This is not the correct way to submit patches for inclusion in the
> stable kernel tree.  Please read:
> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> for how to do this properly.
> 
> 
> 
> If you want these to go to the stable tree(s), that is great, but please
> mark them properly.  As you did it here for this series, it will not
> work.

Sorry Greg. I'll mark it properly for next version.

Thanks,

-- 
Masami Hiramatsu 

Re: [PATCH v3 1/7] kprobes: Make blacklist root user read only

[Masami Hiramatsu]

On Fri, 27 Apr 2018 08:56:11 +0200
Greg KH  wrote:

> On Fri, Apr 27, 2018 at 03:40:09PM +0900, Masami Hiramatsu wrote:
> > Since the blacklist file indicates a sensitive address
> > information to reader, it should be restricted to the
> > root user.
> > 
> > Suggested-by: Thomas Richter 
> > Signed-off-by: Masami Hiramatsu 
> > ---
> >  kernel/kprobes.c |2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> 
> This is not the correct way to submit patches for inclusion in the
> stable kernel tree.  Please read:
> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> for how to do this properly.
> 
> 
> 
> If you want these to go to the stable tree(s), that is great, but please
> mark them properly.  As you did it here for this series, it will not
> work.

Sorry Greg. I'll mark it properly for next version.

Thanks,

-- 
Masami Hiramatsu 

Re: [PATCH v3 1/7] kprobes: Make blacklist root user read only

[Ingo Molnar]

* Masami Hiramatsu  wrote:

> Since the blacklist file indicates a sensitive address
> information to reader, it should be restricted to the
> root user.
> 
> Suggested-by: Thomas Richter 
> Signed-off-by: Masami Hiramatsu 
> ---
>  kernel/kprobes.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index ea619021d901..51096eece801 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -2621,7 +2621,7 @@ static int __init debugfs_kprobe_init(void)
>   if (!file)
>   goto error;
>  
> - file = debugfs_create_file("blacklist", 0444, dir, NULL,
> + file = debugfs_create_file("blacklist", 0400, dir, NULL,
>   _kprobe_blacklist_ops);
>   if (!file)
>   goto error;

Note that in a typical Linux distro debugfs is already root-only:

  fomalhaut:~> ls -ld /sys/kernel/debug
  drwx-- 28 root root 0 Apr 23 08:55 /sys/kernel/debug

but this change might make sense if debugfs is mounted in some other fashion.

But the patch looks incomplete, 'blacklist' is not the only word-readable file 
in 
the kprobes hierarchy. The kprobes directory itself, and the 'list' file is 
readable as well:

  [root@fomalhaut ~]# ls -ld /sys/kernel/debug/kprobes
  drwxr-xr-x 2 root root 0 Apr 23 08:55 /sys/kernel/debug/kprobes

  [root@fomalhaut ~]# ls -l /sys/kernel/debug/kprobes/

  -r--r--r-- 1 root root 0 Apr 23 08:55 blacklist
  -rw--- 1 root root 0 Apr 23 08:55 enabled
  -r--r--r-- 1 root root 0 Apr 23 08:55 list

So not just the blacklist should be 400 but 'list' as well, and the main 
kprobes 
directory as well.

Thanks,

Ingo

Re: [PATCH v3 1/7] kprobes: Make blacklist root user read only

[Ingo Molnar]

* Masami Hiramatsu  wrote:

> Since the blacklist file indicates a sensitive address
> information to reader, it should be restricted to the
> root user.
> 
> Suggested-by: Thomas Richter 
> Signed-off-by: Masami Hiramatsu 
> ---
>  kernel/kprobes.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index ea619021d901..51096eece801 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -2621,7 +2621,7 @@ static int __init debugfs_kprobe_init(void)
>   if (!file)
>   goto error;
>  
> - file = debugfs_create_file("blacklist", 0444, dir, NULL,
> + file = debugfs_create_file("blacklist", 0400, dir, NULL,
>   _kprobe_blacklist_ops);
>   if (!file)
>   goto error;

Note that in a typical Linux distro debugfs is already root-only:

  fomalhaut:~> ls -ld /sys/kernel/debug
  drwx-- 28 root root 0 Apr 23 08:55 /sys/kernel/debug

but this change might make sense if debugfs is mounted in some other fashion.

But the patch looks incomplete, 'blacklist' is not the only word-readable file 
in 
the kprobes hierarchy. The kprobes directory itself, and the 'list' file is 
readable as well:

  [root@fomalhaut ~]# ls -ld /sys/kernel/debug/kprobes
  drwxr-xr-x 2 root root 0 Apr 23 08:55 /sys/kernel/debug/kprobes

  [root@fomalhaut ~]# ls -l /sys/kernel/debug/kprobes/

  -r--r--r-- 1 root root 0 Apr 23 08:55 blacklist
  -rw--- 1 root root 0 Apr 23 08:55 enabled
  -r--r--r-- 1 root root 0 Apr 23 08:55 list

So not just the blacklist should be 400 but 'list' as well, and the main 
kprobes 
directory as well.

Thanks,

Ingo

Re: [PATCH v3 1/7] kprobes: Make blacklist root user read only

[Greg KH]

On Fri, Apr 27, 2018 at 03:40:09PM +0900, Masami Hiramatsu wrote:
> Since the blacklist file indicates a sensitive address
> information to reader, it should be restricted to the
> root user.
> 
> Suggested-by: Thomas Richter 
> Signed-off-by: Masami Hiramatsu 
> ---
>  kernel/kprobes.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)



This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.



If you want these to go to the stable tree(s), that is great, but please
mark them properly.  As you did it here for this series, it will not
work.

thanks,

greg k-h

Re: [PATCH v3 1/7] kprobes: Make blacklist root user read only

[Greg KH]

On Fri, Apr 27, 2018 at 03:40:09PM +0900, Masami Hiramatsu wrote:
> Since the blacklist file indicates a sensitive address
> information to reader, it should be restricted to the
> root user.
> 
> Suggested-by: Thomas Richter 
> Signed-off-by: Masami Hiramatsu 
> ---
>  kernel/kprobes.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)



This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.



If you want these to go to the stable tree(s), that is great, but please
mark them properly.  As you did it here for this series, it will not
work.

thanks,

greg k-h

[PATCH v3 1/7] kprobes: Make blacklist root user read only

[Masami Hiramatsu]

Since the blacklist file indicates a sensitive address
information to reader, it should be restricted to the
root user.

Suggested-by: Thomas Richter 
Signed-off-by: Masami Hiramatsu 
---
 kernel/kprobes.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index ea619021d901..51096eece801 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -2621,7 +2621,7 @@ static int __init debugfs_kprobe_init(void)
if (!file)
goto error;
 
-   file = debugfs_create_file("blacklist", 0444, dir, NULL,
+   file = debugfs_create_file("blacklist", 0400, dir, NULL,
_kprobe_blacklist_ops);
if (!file)
goto error;

[PATCH v3 1/7] kprobes: Make blacklist root user read only

[Masami Hiramatsu]

Since the blacklist file indicates a sensitive address
information to reader, it should be restricted to the
root user.

Suggested-by: Thomas Richter 
Signed-off-by: Masami Hiramatsu 
---
 kernel/kprobes.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index ea619021d901..51096eece801 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -2621,7 +2621,7 @@ static int __init debugfs_kprobe_init(void)
if (!file)
goto error;
 
-   file = debugfs_create_file("blacklist", 0444, dir, NULL,
+   file = debugfs_create_file("blacklist", 0400, dir, NULL,
_kprobe_blacklist_ops);
if (!file)
goto error;