Re: [PATCH RFC v3 3/7] proc: add helpers to set and get proc hidepid and gid mount options

2017-11-10 Thread Djalal Harouni
On Fri, Nov 10, 2017 at 11:36 AM, Alexey Dobriyan  wrote:
> On 11/9/17, Djalal Harouni  wrote:
>> --- a/fs/proc/base.c
>> +++ b/fs/proc/base.c
>
>> -static bool has_pid_permissions(struct pid_namespace *pid,
>> +static bool has_pid_permissions(struct proc_fs_info *fs_info,
>
> More "const".
>
>> diff --git a/fs/proc/inode.c b/fs/proc/inode.c
>> index 9abc370..bdd808d 100644
>> --- a/fs/proc/inode.c
>> +++ b/fs/proc/inode.c
>> @@ -476,11 +476,12 @@ struct inode *proc_get_inode(struct super_block *sb,
>> struct proc_dir_entry *de)
>>  int proc_fill_super(struct super_block *s, void *data, int silent)
>>  {
>>   struct proc_fs_info *fs_info = proc_sb(s);
>> - struct pid_namespace *ns = get_pid_ns(fs_info->pid_ns);
>>   struct inode *root_inode;
>>   int ret;
>>
>> - if (!proc_parse_options(data, ns))
>> + get_pid_ns(fs_info->pid_ns);
>> +
>> + if (!proc_parse_options(data, fs_info))
>>   return -EINVAL;
>>
>>   /* User space would break if executables or devices appear on proc */
>> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
>> index 4a67188..10bc7be 100644
>> --- a/fs/proc/internal.h
>> +++ b/fs/proc/internal.h
>> @@ -240,7 +240,7 @@ static inline void proc_tty_init(void) {}
>>   * root.c
>>   */
>>  extern struct proc_dir_entry proc_root;
>> -extern int proc_parse_options(char *options, struct pid_namespace *pid);
>> +extern int proc_parse_options(char *options, struct proc_fs_info
>> *fs_info);
>
> "extern" can be dropped if you're touching prototype anyway.
>
>
>
>> +static inline int proc_fs_hide_pid(struct proc_fs_info *fs_info)
>> +{
>> + return fs_info->pid_ns->hide_pid;
>> +}
>> +
>> +static inline kgid_t proc_fs_pid_gid(struct proc_fs_info *fs_info)
>> +{
>> + return fs_info->pid_ns->pid_gid;
>> +}
>
> More "const".
>
>> @@ -59,6 +81,24 @@ static inline void proc_flush_task(struct task_struct
>> *task)
>>  {
>>  }
>>
>> +static inline void proc_fs_set_hide_pid(struct proc_fs_info *fs_info, int
>> hide_pid)
>> +{
>> +}
>> +
>> +static inline void proc_fs_set_pid_gid(struct proc_info_fs *fs_info, kgid_t
>> gid)
>> +{
>> +}
>> +
>> +static inline int proc_fs_hide_pid(struct proc_fs_info *fs_info)
>> +{
>> + return 0;
>> +}
>> +
>> +extern kgid_t proc_fs_pid_gid(struct proc_fs_info *fs_info)
>
> ehh?

Ouch copy/past, will compile it without proc support.

Will fix "const" and other comments too, thank you!


-- 
tixxdz


Re: [PATCH RFC v3 3/7] proc: add helpers to set and get proc hidepid and gid mount options

2017-11-10 Thread Djalal Harouni
On Fri, Nov 10, 2017 at 11:36 AM, Alexey Dobriyan  wrote:
> On 11/9/17, Djalal Harouni  wrote:
>> --- a/fs/proc/base.c
>> +++ b/fs/proc/base.c
>
>> -static bool has_pid_permissions(struct pid_namespace *pid,
>> +static bool has_pid_permissions(struct proc_fs_info *fs_info,
>
> More "const".
>
>> diff --git a/fs/proc/inode.c b/fs/proc/inode.c
>> index 9abc370..bdd808d 100644
>> --- a/fs/proc/inode.c
>> +++ b/fs/proc/inode.c
>> @@ -476,11 +476,12 @@ struct inode *proc_get_inode(struct super_block *sb,
>> struct proc_dir_entry *de)
>>  int proc_fill_super(struct super_block *s, void *data, int silent)
>>  {
>>   struct proc_fs_info *fs_info = proc_sb(s);
>> - struct pid_namespace *ns = get_pid_ns(fs_info->pid_ns);
>>   struct inode *root_inode;
>>   int ret;
>>
>> - if (!proc_parse_options(data, ns))
>> + get_pid_ns(fs_info->pid_ns);
>> +
>> + if (!proc_parse_options(data, fs_info))
>>   return -EINVAL;
>>
>>   /* User space would break if executables or devices appear on proc */
>> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
>> index 4a67188..10bc7be 100644
>> --- a/fs/proc/internal.h
>> +++ b/fs/proc/internal.h
>> @@ -240,7 +240,7 @@ static inline void proc_tty_init(void) {}
>>   * root.c
>>   */
>>  extern struct proc_dir_entry proc_root;
>> -extern int proc_parse_options(char *options, struct pid_namespace *pid);
>> +extern int proc_parse_options(char *options, struct proc_fs_info
>> *fs_info);
>
> "extern" can be dropped if you're touching prototype anyway.
>
>
>
>> +static inline int proc_fs_hide_pid(struct proc_fs_info *fs_info)
>> +{
>> + return fs_info->pid_ns->hide_pid;
>> +}
>> +
>> +static inline kgid_t proc_fs_pid_gid(struct proc_fs_info *fs_info)
>> +{
>> + return fs_info->pid_ns->pid_gid;
>> +}
>
> More "const".
>
>> @@ -59,6 +81,24 @@ static inline void proc_flush_task(struct task_struct
>> *task)
>>  {
>>  }
>>
>> +static inline void proc_fs_set_hide_pid(struct proc_fs_info *fs_info, int
>> hide_pid)
>> +{
>> +}
>> +
>> +static inline void proc_fs_set_pid_gid(struct proc_info_fs *fs_info, kgid_t
>> gid)
>> +{
>> +}
>> +
>> +static inline int proc_fs_hide_pid(struct proc_fs_info *fs_info)
>> +{
>> + return 0;
>> +}
>> +
>> +extern kgid_t proc_fs_pid_gid(struct proc_fs_info *fs_info)
>
> ehh?

Ouch copy/past, will compile it without proc support.

Will fix "const" and other comments too, thank you!


-- 
tixxdz


Re: [PATCH RFC v3 3/7] proc: add helpers to set and get proc hidepid and gid mount options

2017-11-10 Thread Alexey Dobriyan
On 11/9/17, Djalal Harouni  wrote:
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c

> -static bool has_pid_permissions(struct pid_namespace *pid,
> +static bool has_pid_permissions(struct proc_fs_info *fs_info,

More "const".

> diff --git a/fs/proc/inode.c b/fs/proc/inode.c
> index 9abc370..bdd808d 100644
> --- a/fs/proc/inode.c
> +++ b/fs/proc/inode.c
> @@ -476,11 +476,12 @@ struct inode *proc_get_inode(struct super_block *sb,
> struct proc_dir_entry *de)
>  int proc_fill_super(struct super_block *s, void *data, int silent)
>  {
>   struct proc_fs_info *fs_info = proc_sb(s);
> - struct pid_namespace *ns = get_pid_ns(fs_info->pid_ns);
>   struct inode *root_inode;
>   int ret;
>
> - if (!proc_parse_options(data, ns))
> + get_pid_ns(fs_info->pid_ns);
> +
> + if (!proc_parse_options(data, fs_info))
>   return -EINVAL;
>
>   /* User space would break if executables or devices appear on proc */
> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> index 4a67188..10bc7be 100644
> --- a/fs/proc/internal.h
> +++ b/fs/proc/internal.h
> @@ -240,7 +240,7 @@ static inline void proc_tty_init(void) {}
>   * root.c
>   */
>  extern struct proc_dir_entry proc_root;
> -extern int proc_parse_options(char *options, struct pid_namespace *pid);
> +extern int proc_parse_options(char *options, struct proc_fs_info
> *fs_info);

"extern" can be dropped if you're touching prototype anyway.



> +static inline int proc_fs_hide_pid(struct proc_fs_info *fs_info)
> +{
> + return fs_info->pid_ns->hide_pid;
> +}
> +
> +static inline kgid_t proc_fs_pid_gid(struct proc_fs_info *fs_info)
> +{
> + return fs_info->pid_ns->pid_gid;
> +}

More "const".

> @@ -59,6 +81,24 @@ static inline void proc_flush_task(struct task_struct
> *task)
>  {
>  }
>
> +static inline void proc_fs_set_hide_pid(struct proc_fs_info *fs_info, int
> hide_pid)
> +{
> +}
> +
> +static inline void proc_fs_set_pid_gid(struct proc_info_fs *fs_info, kgid_t
> gid)
> +{
> +}
> +
> +static inline int proc_fs_hide_pid(struct proc_fs_info *fs_info)
> +{
> + return 0;
> +}
> +
> +extern kgid_t proc_fs_pid_gid(struct proc_fs_info *fs_info)

ehh?

> +{
> + return GLOBAL_ROOT_GID;
> +}


Re: [PATCH RFC v3 3/7] proc: add helpers to set and get proc hidepid and gid mount options

2017-11-10 Thread Alexey Dobriyan
On 11/9/17, Djalal Harouni  wrote:
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c

> -static bool has_pid_permissions(struct pid_namespace *pid,
> +static bool has_pid_permissions(struct proc_fs_info *fs_info,

More "const".

> diff --git a/fs/proc/inode.c b/fs/proc/inode.c
> index 9abc370..bdd808d 100644
> --- a/fs/proc/inode.c
> +++ b/fs/proc/inode.c
> @@ -476,11 +476,12 @@ struct inode *proc_get_inode(struct super_block *sb,
> struct proc_dir_entry *de)
>  int proc_fill_super(struct super_block *s, void *data, int silent)
>  {
>   struct proc_fs_info *fs_info = proc_sb(s);
> - struct pid_namespace *ns = get_pid_ns(fs_info->pid_ns);
>   struct inode *root_inode;
>   int ret;
>
> - if (!proc_parse_options(data, ns))
> + get_pid_ns(fs_info->pid_ns);
> +
> + if (!proc_parse_options(data, fs_info))
>   return -EINVAL;
>
>   /* User space would break if executables or devices appear on proc */
> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> index 4a67188..10bc7be 100644
> --- a/fs/proc/internal.h
> +++ b/fs/proc/internal.h
> @@ -240,7 +240,7 @@ static inline void proc_tty_init(void) {}
>   * root.c
>   */
>  extern struct proc_dir_entry proc_root;
> -extern int proc_parse_options(char *options, struct pid_namespace *pid);
> +extern int proc_parse_options(char *options, struct proc_fs_info
> *fs_info);

"extern" can be dropped if you're touching prototype anyway.



> +static inline int proc_fs_hide_pid(struct proc_fs_info *fs_info)
> +{
> + return fs_info->pid_ns->hide_pid;
> +}
> +
> +static inline kgid_t proc_fs_pid_gid(struct proc_fs_info *fs_info)
> +{
> + return fs_info->pid_ns->pid_gid;
> +}

More "const".

> @@ -59,6 +81,24 @@ static inline void proc_flush_task(struct task_struct
> *task)
>  {
>  }
>
> +static inline void proc_fs_set_hide_pid(struct proc_fs_info *fs_info, int
> hide_pid)
> +{
> +}
> +
> +static inline void proc_fs_set_pid_gid(struct proc_info_fs *fs_info, kgid_t
> gid)
> +{
> +}
> +
> +static inline int proc_fs_hide_pid(struct proc_fs_info *fs_info)
> +{
> + return 0;
> +}
> +
> +extern kgid_t proc_fs_pid_gid(struct proc_fs_info *fs_info)

ehh?

> +{
> + return GLOBAL_ROOT_GID;
> +}


[PATCH RFC v3 3/7] proc: add helpers to set and get proc hidepid and gid mount options

2017-11-09 Thread Djalal Harouni
This is a cleaning patch to add helpers to set and get proc mount
options instead of a direct access. This allows later to easily track
what's happening, how these fields are accessed, and in case we need
to update them in the future.

Later we will move these mount options to proc_fs_info struct. First
lets fix the access.

Cc: Kees Cook 
Cc: Greg Kroah-Hartman 
Suggested-by: Andy Lutomirski 
Signed-off-by: Alexey Gladkov 
Signed-off-by: Djalal Harouni 
---
 fs/proc/base.c  | 16 +---
 fs/proc/inode.c |  5 +++--
 fs/proc/internal.h  |  2 +-
 fs/proc/root.c  | 15 ++-
 include/linux/proc_fs.h | 44 ++--
 5 files changed, 65 insertions(+), 17 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 0d9b4214..f324c49 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -682,13 +682,16 @@ int proc_setattr(struct dentry *dentry, struct iattr 
*attr)
  * May current process learn task's sched/cmdline info (for hide_pid_min=1)
  * or euid/egid (for hide_pid_min=2)?
  */
-static bool has_pid_permissions(struct pid_namespace *pid,
+static bool has_pid_permissions(struct proc_fs_info *fs_info,
 struct task_struct *task,
 int hide_pid_min)
 {
-   if (pid->hide_pid < hide_pid_min)
+   int hide_pid = proc_fs_hide_pid(fs_info);
+   kgid_t gid = proc_fs_pid_gid(fs_info);
+
+   if (hide_pid < hide_pid_min)
return true;
-   if (in_group_p(pid->pid_gid))
+   if (in_group_p(gid))
return true;
return ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS);
 }
@@ -704,7 +707,7 @@ static int proc_pid_permission(struct inode *inode, int 
mask)
task = get_proc_task(inode);
if (!task)
return -ESRCH;
-   has_perms = has_pid_permissions(pid, task, HIDEPID_NO_ACCESS);
+   has_perms = has_pid_permissions(fs_info, task, HIDEPID_NO_ACCESS);
put_task_struct(task);
 
if (!has_perms) {
@@ -1778,7 +1781,6 @@ int pid_getattr(const struct path *path, struct kstat 
*stat,
struct task_struct *task;
struct inode *inode = d_inode(path->dentry);
struct proc_fs_info *fs_info = proc_sb(inode->i_sb);
-   struct pid_namespace *pid = fs_info->pid_ns;
 
generic_fillattr(inode, stat);
 
@@ -1787,7 +1789,7 @@ int pid_getattr(const struct path *path, struct kstat 
*stat,
stat->gid = GLOBAL_ROOT_GID;
task = pid_task(proc_pid(inode), PIDTYPE_PID);
if (task) {
-   if (!has_pid_permissions(pid, task, HIDEPID_INVISIBLE)) {
+   if (!has_pid_permissions(fs_info, task, HIDEPID_INVISIBLE)) {
rcu_read_unlock();
/*
 * This doesn't prevent learning whether PID exists,
@@ -3234,7 +3236,7 @@ int proc_pid_readdir(struct file *file, struct 
dir_context *ctx)
int len;
 
cond_resched();
-   if (!has_pid_permissions(ns, iter.task, HIDEPID_INVISIBLE))
+   if (!has_pid_permissions(fs_info, iter.task, HIDEPID_INVISIBLE))
continue;
 
len = snprintf(name, sizeof(name), "%d", iter.tgid);
diff --git a/fs/proc/inode.c b/fs/proc/inode.c
index 9abc370..bdd808d 100644
--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -476,11 +476,12 @@ struct inode *proc_get_inode(struct super_block *sb, 
struct proc_dir_entry *de)
 int proc_fill_super(struct super_block *s, void *data, int silent)
 {
struct proc_fs_info *fs_info = proc_sb(s);
-   struct pid_namespace *ns = get_pid_ns(fs_info->pid_ns);
struct inode *root_inode;
int ret;
 
-   if (!proc_parse_options(data, ns))
+   get_pid_ns(fs_info->pid_ns);
+
+   if (!proc_parse_options(data, fs_info))
return -EINVAL;
 
/* User space would break if executables or devices appear on proc */
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index 4a67188..10bc7be 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -240,7 +240,7 @@ static inline void proc_tty_init(void) {}
  * root.c
  */
 extern struct proc_dir_entry proc_root;
-extern int proc_parse_options(char *options, struct pid_namespace *pid);
+extern int proc_parse_options(char *options, struct proc_fs_info *fs_info);
 
 extern void proc_self_init(void);
 extern int proc_remount(struct super_block *, int *, char *);
diff --git a/fs/proc/root.c b/fs/proc/root.c
index b225ae5..48cc481 100644
--- a/fs/proc/root.c
+++ b/fs/proc/root.c
@@ -37,11 +37,12 @@ static const match_table_t tokens = {
{Opt_err, NULL},
 };
 
-int proc_parse_options(char *options, struct pid_namespace *pid)
+int proc_parse_options(char *options, struct proc_fs_info *fs_info)
 {
char *p;
substring_t 

[PATCH RFC v3 3/7] proc: add helpers to set and get proc hidepid and gid mount options

2017-11-09 Thread Djalal Harouni
This is a cleaning patch to add helpers to set and get proc mount
options instead of a direct access. This allows later to easily track
what's happening, how these fields are accessed, and in case we need
to update them in the future.

Later we will move these mount options to proc_fs_info struct. First
lets fix the access.

Cc: Kees Cook 
Cc: Greg Kroah-Hartman 
Suggested-by: Andy Lutomirski 
Signed-off-by: Alexey Gladkov 
Signed-off-by: Djalal Harouni 
---
 fs/proc/base.c  | 16 +---
 fs/proc/inode.c |  5 +++--
 fs/proc/internal.h  |  2 +-
 fs/proc/root.c  | 15 ++-
 include/linux/proc_fs.h | 44 ++--
 5 files changed, 65 insertions(+), 17 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 0d9b4214..f324c49 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -682,13 +682,16 @@ int proc_setattr(struct dentry *dentry, struct iattr 
*attr)
  * May current process learn task's sched/cmdline info (for hide_pid_min=1)
  * or euid/egid (for hide_pid_min=2)?
  */
-static bool has_pid_permissions(struct pid_namespace *pid,
+static bool has_pid_permissions(struct proc_fs_info *fs_info,
 struct task_struct *task,
 int hide_pid_min)
 {
-   if (pid->hide_pid < hide_pid_min)
+   int hide_pid = proc_fs_hide_pid(fs_info);
+   kgid_t gid = proc_fs_pid_gid(fs_info);
+
+   if (hide_pid < hide_pid_min)
return true;
-   if (in_group_p(pid->pid_gid))
+   if (in_group_p(gid))
return true;
return ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS);
 }
@@ -704,7 +707,7 @@ static int proc_pid_permission(struct inode *inode, int 
mask)
task = get_proc_task(inode);
if (!task)
return -ESRCH;
-   has_perms = has_pid_permissions(pid, task, HIDEPID_NO_ACCESS);
+   has_perms = has_pid_permissions(fs_info, task, HIDEPID_NO_ACCESS);
put_task_struct(task);
 
if (!has_perms) {
@@ -1778,7 +1781,6 @@ int pid_getattr(const struct path *path, struct kstat 
*stat,
struct task_struct *task;
struct inode *inode = d_inode(path->dentry);
struct proc_fs_info *fs_info = proc_sb(inode->i_sb);
-   struct pid_namespace *pid = fs_info->pid_ns;
 
generic_fillattr(inode, stat);
 
@@ -1787,7 +1789,7 @@ int pid_getattr(const struct path *path, struct kstat 
*stat,
stat->gid = GLOBAL_ROOT_GID;
task = pid_task(proc_pid(inode), PIDTYPE_PID);
if (task) {
-   if (!has_pid_permissions(pid, task, HIDEPID_INVISIBLE)) {
+   if (!has_pid_permissions(fs_info, task, HIDEPID_INVISIBLE)) {
rcu_read_unlock();
/*
 * This doesn't prevent learning whether PID exists,
@@ -3234,7 +3236,7 @@ int proc_pid_readdir(struct file *file, struct 
dir_context *ctx)
int len;
 
cond_resched();
-   if (!has_pid_permissions(ns, iter.task, HIDEPID_INVISIBLE))
+   if (!has_pid_permissions(fs_info, iter.task, HIDEPID_INVISIBLE))
continue;
 
len = snprintf(name, sizeof(name), "%d", iter.tgid);
diff --git a/fs/proc/inode.c b/fs/proc/inode.c
index 9abc370..bdd808d 100644
--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -476,11 +476,12 @@ struct inode *proc_get_inode(struct super_block *sb, 
struct proc_dir_entry *de)
 int proc_fill_super(struct super_block *s, void *data, int silent)
 {
struct proc_fs_info *fs_info = proc_sb(s);
-   struct pid_namespace *ns = get_pid_ns(fs_info->pid_ns);
struct inode *root_inode;
int ret;
 
-   if (!proc_parse_options(data, ns))
+   get_pid_ns(fs_info->pid_ns);
+
+   if (!proc_parse_options(data, fs_info))
return -EINVAL;
 
/* User space would break if executables or devices appear on proc */
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index 4a67188..10bc7be 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -240,7 +240,7 @@ static inline void proc_tty_init(void) {}
  * root.c
  */
 extern struct proc_dir_entry proc_root;
-extern int proc_parse_options(char *options, struct pid_namespace *pid);
+extern int proc_parse_options(char *options, struct proc_fs_info *fs_info);
 
 extern void proc_self_init(void);
 extern int proc_remount(struct super_block *, int *, char *);
diff --git a/fs/proc/root.c b/fs/proc/root.c
index b225ae5..48cc481 100644
--- a/fs/proc/root.c
+++ b/fs/proc/root.c
@@ -37,11 +37,12 @@ static const match_table_t tokens = {
{Opt_err, NULL},
 };
 
-int proc_parse_options(char *options, struct pid_namespace *pid)
+int proc_parse_options(char *options, struct proc_fs_info *fs_info)
 {
char *p;
substring_t args[MAX_OPT_ARGS];
int option;
+   kgid_t gid;
 
if (!options)
return 1;
@@ -57,7