[PATCH] Security: Handle hidepid option correctly

[PATCH] Security: Handle hidepid option correctly

[d17103513]

From: Cheng Yang <chengyang@xiaomi.com>

The proc_parse_options() call from proc_mount() runs only once at boot
time.  So on any later mount attempt, any mount options are ignored
because ->s_root is already initialized.
As a consequence, "mount -o <options>" will ignore the options.  The
only way to change mount options is "mount -o remount,<options>".
To fix this, parse the mount options unconditionally.

Signed-off-by: Cheng Yang <chengyang@xiaomi.com>
---
 fs/proc/inode.c | 4 ----
 fs/proc/root.c  | 3 +++
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/proc/inode.c b/fs/proc/inode.c
index 5792f9e..ec509d1 100644
--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -493,13 +493,9 @@ 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 pid_namespace *ns = get_pid_ns(s->s_fs_info);
 	struct inode *root_inode;
 	int ret;
 
-	if (!proc_parse_options(data, ns))
-		return -EINVAL;
-
 	/* User space would break if executables or devices appear on proc */
 	s->s_iflags |= SB_I_USERNS_VISIBLE | SB_I_NOEXEC | SB_I_NODEV;
 	s->s_flags |= SB_NODIRATIME | SB_NOSUID | SB_NOEXEC;
diff --git a/fs/proc/root.c b/fs/proc/root.c
index f4b1a9d..f5f3bf3 100644
--- a/fs/proc/root.c
+++ b/fs/proc/root.c
@@ -98,6 +98,9 @@ static struct dentry *proc_mount(struct file_system_type *fs_type,
 		ns = task_active_pid_ns(current);
 	}
 
+	if (!proc_parse_options(data, ns))
+		return ERR_PTR(-EINVAL);
+
 	return mount_ns(fs_type, flags, data, ns, ns->user_ns, proc_fill_super);
 }
 
-- 
1.9.1

Re: [PATCH] Security: Handle hidepid option correctly

[Andrew Morton]

> [PATCH] Security: Handle hidepid option correctly

Why is this considered to be security sensitive?  I can guess, but I'd
like to know your reasoning.

On Thu, 29 Nov 2018 19:08:21 +0800 d17103513@gmail.com wrote:

> From: Cheng Yang <chengyang@xiaomi.com>
> 
> The proc_parse_options() call from proc_mount() runs only once at boot
> time.  So on any later mount attempt, any mount options are ignored
> because ->s_root is already initialized.
> As a consequence, "mount -o <options>" will ignore the options.  The
> only way to change mount options is "mount -o remount,<options>".
> To fix this, parse the mount options unconditionally.
> 
> --- a/fs/proc/inode.c
> +++ b/fs/proc/inode.c
> @@ -493,13 +493,9 @@ 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 pid_namespace *ns = get_pid_ns(s->s_fs_info);
>  	struct inode *root_inode;
>  	int ret;
>  
> -	if (!proc_parse_options(data, ns))
> -		return -EINVAL;
> -
>  	/* User space would break if executables or devices appear on proc */
>  	s->s_iflags |= SB_I_USERNS_VISIBLE | SB_I_NOEXEC | SB_I_NODEV;
>  	s->s_flags |= SB_NODIRATIME | SB_NOSUID | SB_NOEXEC;
> diff --git a/fs/proc/root.c b/fs/proc/root.c
> index f4b1a9d..f5f3bf3 100644
> --- a/fs/proc/root.c
> +++ b/fs/proc/root.c
> @@ -98,6 +98,9 @@ static struct dentry *proc_mount(struct file_system_type *fs_type,
>  		ns = task_active_pid_ns(current);
>  	}
>  
> +	if (!proc_parse_options(data, ns))
> +		return ERR_PTR(-EINVAL);
> +
>  	return mount_ns(fs_type, flags, data, ns, ns->user_ns, proc_fill_super);
>  }

Other filesystems parse the options from fill_super().  Is proc special
in some fashion?

> [PATCH] Security: Handle hidepid option correctly

[程洋]

Here is an article illustrates the details.
https://medium.com/@topjohnwu/from-anime-game-to-android-system-security-vulnerability-9b955a182f20

And There is a similar fix on kernel-4.4:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=99663be772c827b8f5f594fe87eb4807be1994e5

Q: Other filesystems parse the options from fill_super().  Is proc special in some fashion?
A: According to my research, start_kernel will call proc_mount first, and initialize sb->s_root before any userspace process runs. If others want to mount it, all options will be ignored.
     AOSP change here: https://android-review.googlesource.com/c/platform/system/core/+/181345/4/init/init.cpp
     At first I though we should mount it with MS_REMOUNT flag. But kernel will crash if we did this.

Q:  Why is this considered to be security sensitive?  I can guess, but I'd like to know your reasoning.
A: See the article above. It's part of Android sanbox.


> [PATCH] Security: Handle hidepid option correctly

Why is this considered to be security sensitive?  I can guess, but I'd like to know your reasoning.

On Thu, 29 Nov 2018 19:08:21 +0800 mailto:d17103513@gmail.com wrote:

> From: Cheng Yang <mailto:chengyang@xiaomi.com>
>
> The proc_parse_options() call from proc_mount() runs only once at boot
> time.  So on any later mount attempt, any mount options are ignored
> because ->s_root is already initialized.
> As a consequence, "mount -o <options>" will ignore the options.  The
> only way to change mount options is "mount -o remount,<options>".
> To fix this, parse the mount options unconditionally.
>
> --- a/fs/proc/inode.c
> +++ b/fs/proc/inode.c
> @@ -493,13 +493,9 @@ 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 pid_namespace *ns = get_pid_ns(s->s_fs_info);
>  struct inode *root_inode;
>  int ret;
>
> -if (!proc_parse_options(data, ns))
> -return -EINVAL;
> -
>  /* User space would break if executables or devices appear on proc */
>  s->s_iflags |= SB_I_USERNS_VISIBLE | SB_I_NOEXEC | SB_I_NODEV;
>  s->s_flags |= SB_NODIRATIME | SB_NOSUID | SB_NOEXEC; diff --git
> a/fs/proc/root.c b/fs/proc/root.c index f4b1a9d..f5f3bf3 100644
> --- a/fs/proc/root.c
> +++ b/fs/proc/root.c
> @@ -98,6 +98,9 @@ static struct dentry *proc_mount(struct file_system_type *fs_type,
>  ns = task_active_pid_ns(current);
>  }
>
> +if (!proc_parse_options(data, ns))
> +return ERR_PTR(-EINVAL);
> +
>  return mount_ns(fs_type, flags, data, ns, ns->user_ns,
> proc_fill_super);  }

Other filesystems parse the options from fill_super().  Is proc special in some fashion?

#/******本邮件及其附件含有小米公司的保密信息，仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本邮件！ This e-mail and its attachments contain confidential information from XIAOMI, which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction, or dissemination) by persons other than the intended recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender by phone or email immediately and delete it!******/#

Re: > [PATCH] Security: Handle hidepid option correctly

[程洋]

Here is an article illustrates the details.
https://medium.com/@topjohnwu/from-anime-game-to-android-system-security-vulnerability-9b955a182f20

And There is a similar fix on kernel-4.4:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=99663be772c827b8f5f594fe87eb4807be1994e5

Q: Other filesystems parse the options from fill_super().  Is proc
special in some fashion?
A: According to my research, start_kernel will call proc_mount first,
and initialize sb->s_root before any userspace process runs. If others
want to mount it, all options will be ignored.
     AOSP change here:
https://android-review.googlesource.com/c/platform/system/core/+/181345/4/init/init.cpp
     At first I though we should mount it with MS_REMOUNT flag. But
mount will failed. Though i can mount it first, and then mount second
time with MS_REMOUNT, which is tricky

Q:  Why is this considered to be security sensitive?  I can guess, but
I'd like to know your reasoning.
A: See the article above. It's part of Android sanbox.

程洋 <chengyang@xiaomi.com> 于2018年11月30日周五 上午10:34写道：
>
> Here is an article illustrates the details.
> https://medium.com/@topjohnwu/from-anime-game-to-android-system-security-vulnerability-9b955a182f20
>
> And There is a similar fix on kernel-4.4:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=99663be772c827b8f5f594fe87eb4807be1994e5
>
> Q: Other filesystems parse the options from fill_super().  Is proc special in some fashion?
> A: According to my research, start_kernel will call proc_mount first, and initialize sb->s_root before any userspace process runs. If others want to mount it, all options will be ignored.
>      AOSP change here: https://android-review.googlesource.com/c/platform/system/core/+/181345/4/init/init.cpp
>      At first I though we should mount it with MS_REMOUNT flag. But kernel will crash if we did this.
>
> Q:  Why is this considered to be security sensitive?  I can guess, but I'd like to know your reasoning.
> A: See the article above. It's part of Android sanbox.
>
>
> > [PATCH] Security: Handle hidepid option correctly
>
> Why is this considered to be security sensitive?  I can guess, but I'd like to know your reasoning.
>
> On Thu, 29 Nov 2018 19:08:21 +0800 mailto:d17103513@gmail.com wrote:
>
> > From: Cheng Yang <mailto:chengyang@xiaomi.com>
> >
> > The proc_parse_options() call from proc_mount() runs only once at boot
> > time.  So on any later mount attempt, any mount options are ignored
> > because ->s_root is already initialized.
> > As a consequence, "mount -o <options>" will ignore the options.  The
> > only way to change mount options is "mount -o remount,<options>".
> > To fix this, parse the mount options unconditionally.
> >
> > --- a/fs/proc/inode.c
> > +++ b/fs/proc/inode.c
> > @@ -493,13 +493,9 @@ 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 pid_namespace *ns = get_pid_ns(s->s_fs_info);
> >  struct inode *root_inode;
> >  int ret;
> >
> > -if (!proc_parse_options(data, ns))
> > -return -EINVAL;
> > -
> >  /* User space would break if executables or devices appear on proc */
> >  s->s_iflags |= SB_I_USERNS_VISIBLE | SB_I_NOEXEC | SB_I_NODEV;
> >  s->s_flags |= SB_NODIRATIME | SB_NOSUID | SB_NOEXEC; diff --git
> > a/fs/proc/root.c b/fs/proc/root.c index f4b1a9d..f5f3bf3 100644
> > --- a/fs/proc/root.c
> > +++ b/fs/proc/root.c
> > @@ -98,6 +98,9 @@ static struct dentry *proc_mount(struct file_system_type *fs_type,
> >  ns = task_active_pid_ns(current);
> >  }
> >
> > +if (!proc_parse_options(data, ns))
> > +return ERR_PTR(-EINVAL);
> > +
> >  return mount_ns(fs_type, flags, data, ns, ns->user_ns,
> > proc_fill_super);  }
>
> Other filesystems parse the options from fill_super().  Is proc special in some fashion?
>
> #/******本邮件及其附件含有小米公司的保密信息，仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本邮件！ This e-mail and its attachments contain confidential information from XIAOMI, which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction, or dissemination) by persons other than the intended recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender by phone or email immediately and delete it!******/#

Re: > [PATCH] Security: Handle hidepid option correctly

[程洋]

Andrew's question makes me think if this fix is superficial. Actually
i have had same question. But when i saw a smilar patch in kernel-4.4
was already merged in 2012, i decided to submit this patch first.

Here is the call stack i got:
[    0.003450] [<ffffff8bef2a0190>] proc_mount+0x2c/0x98
[    0.003459] [<ffffff8bef22e560>] mount_fs+0x164/0x190
[    0.003465] [<ffffff8bef24c138>] vfs_kern_mount+0x74/0x168
[    0.003469] [<ffffff8bef24c244>] kern_mount_data+0x18/0x30
[    0.003474] [<ffffff8bef2a0258>] pid_ns_prepare_proc+0x24/0x40
[    0.003484] [<ffffff8bef0cd5ec>] alloc_pid+0x498/0x4b4
[    0.003492] [<ffffff8bef0a9b94>] copy_process.isra.73.part.74+0xed0/0x1708
[    0.003496] [<ffffff8bef0aa560>] _do_fork+0xdc/0x3f8
[    0.003501] [<ffffff8bef0aa8c8>] kernel_thread+0x34/0x3c
[    0.003511] [<ffffff8bf00cd498>] rest_init+0x20/0x80
[    0.003522] [<ffffff8bf0c00c7c>] start_kernel+0x3e4/0x43c
[    0.003527] [<ffffff8bf0c001e8>] __primary_switched+0x64/0x90
I notice only proc filesystem has function "pid_ns_prepare_proc".
there is no other "pid_ns_prepare_xxx" function in other filesystem.
Take the position of proc filesystem of kernel into consideration, the
answer of question "Other filesystems parse the options from
fill_super().  Is proc special in some fashion" could be "Yes, it is.
Because proc filesystem is special indeed. It's a filesystem kernel
will mount when it's booting".

But is it enough? Is anyone responsible for deinitialize sb->sroot?
Well actually i'm not an expert of filesystem, and don't unserstand
what does sb->s_root represent for. But i'm sure no one call
"pid_ns_release_proc" in the runtime(by add some logs). And even it is
called, it doesn't clean sb->s_root. Until now, i didn't see any
deeper issue. Maybe it's true that we should handle proc filesystem
specially.

If anyone who is sure about the functionality of sb->s_root and think
it should be handled in another way, feel free to correct me.


程洋 <chengyang@xiaomi.com> 于2018年11月30日周五 上午10:34写道：
>> Here is an article illustrates the details.
> https://medium.com/@topjohnwu/from-anime-game-to-android-system-security-vulnerability-9b955a182f20
>
> And There is a similar fix on kernel-4.4:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=99663be772c827b8f5f594fe87eb4807be1994e5
>
> Q: Other filesystems parse the options from fill_super().  Is proc special in some fashion?
> A: According to my research, start_kernel will call proc_mount first, and initialize sb->s_root before any userspace process runs. If others want to mount it, all options will be ignored.
>      AOSP change here: https://android-review.googlesource.com/c/platform/system/core/+/181345/4/init/init.cpp
>      At first I though we should mount it with MS_REMOUNT flag. But kernel will crash if we did this.
>
> Q:  Why is this considered to be security sensitive?  I can guess, but I'd like to know your reasoning.
> A: See the article above. It's part of Android sanbox.
>
>
> > [PATCH] Security: Handle hidepid option correctly
>
> Why is this considered to be security sensitive?  I can guess, but I'd like to know your reasoning.
>
> On Thu, 29 Nov 2018 19:08:21 +0800 mailto:d17103513@gmail.com wrote:
>
> > From: Cheng Yang <mailto:chengyang@xiaomi.com>
> >
> > The proc_parse_options() call from proc_mount() runs only once at boot
> > time.  So on any later mount attempt, any mount options are ignored
> > because ->s_root is already initialized.
> > As a consequence, "mount -o <options>" will ignore the options.  The
> > only way to change mount options is "mount -o remount,<options>".
> > To fix this, parse the mount options unconditionally.
> >
> > --- a/fs/proc/inode.c
> > +++ b/fs/proc/inode.c
> > @@ -493,13 +493,9 @@ 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 pid_namespace *ns = get_pid_ns(s->s_fs_info);
> >  struct inode *root_inode;
> >  int ret;
> >
> > -if (!proc_parse_options(data, ns))
> > -return -EINVAL;
> > -
> >  /* User space would break if executables or devices appear on proc */
> >  s->s_iflags |= SB_I_USERNS_VISIBLE | SB_I_NOEXEC | SB_I_NODEV;
> >  s->s_flags |= SB_NODIRATIME | SB_NOSUID | SB_NOEXEC; diff --git
> > a/fs/proc/root.c b/fs/proc/root.c index f4b1a9d..f5f3bf3 100644
> > --- a/fs/proc/root.c
> > +++ b/fs/proc/root.c
> > @@ -98,6 +98,9 @@ static struct dentry *proc_mount(struct file_system_type *fs_type,
> >  ns = task_active_pid_ns(current);
> >  }
> >
> > +if (!proc_parse_options(data, ns))
> > +return ERR_PTR(-EINVAL);
> > +
> >  return mount_ns(fs_type, flags, data, ns, ns->user_ns,
> > proc_fill_super);  }
>
> Other filesystems parse the options from fill_super().  Is proc special in some fashion?
>
> #/******本邮件及其附件含有小米公司的保密信息，仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本邮件！ This e-mail and its attachments contain confidential information from XIAOMI, which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction, or dissemination) by persons other than the intended recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender by phone or email immediately and delete it!******/#

Re: > [PATCH] Security: Handle hidepid option correctly

[程洋]

Anyone who can review my patch?

程洋 <chengyang@xiaomi.com> 于2018年11月30日周五 上午10:34写道：
>
> Here is an article illustrates the details.
> https://medium.com/@topjohnwu/from-anime-game-to-android-system-security-vulnerability-9b955a182f20
>
> And There is a similar fix on kernel-4.4:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=99663be772c827b8f5f594fe87eb4807be1994e5
>
> Q: Other filesystems parse the options from fill_super().  Is proc special in some fashion?
> A: According to my research, start_kernel will call proc_mount first, and initialize sb->s_root before any userspace process runs. If others want to mount it, all options will be ignored.
>      AOSP change here: https://android-review.googlesource.com/c/platform/system/core/+/181345/4/init/init.cpp
>      At first I though we should mount it with MS_REMOUNT flag. But kernel will crash if we did this.
>
> Q:  Why is this considered to be security sensitive?  I can guess, but I'd like to know your reasoning.
> A: See the article above. It's part of Android sanbox.
>
>
> > [PATCH] Security: Handle hidepid option correctly
>
> Why is this considered to be security sensitive?  I can guess, but I'd like to know your reasoning.
>
> On Thu, 29 Nov 2018 19:08:21 +0800 mailto:d17103513@gmail.com wrote:
>
> > From: Cheng Yang <mailto:chengyang@xiaomi.com>
> >
> > The proc_parse_options() call from proc_mount() runs only once at boot
> > time.  So on any later mount attempt, any mount options are ignored
> > because ->s_root is already initialized.
> > As a consequence, "mount -o <options>" will ignore the options.  The
> > only way to change mount options is "mount -o remount,<options>".
> > To fix this, parse the mount options unconditionally.
> >
> > --- a/fs/proc/inode.c
> > +++ b/fs/proc/inode.c
> > @@ -493,13 +493,9 @@ 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 pid_namespace *ns = get_pid_ns(s->s_fs_info);
> >  struct inode *root_inode;
> >  int ret;
> >
> > -if (!proc_parse_options(data, ns))
> > -return -EINVAL;
> > -
> >  /* User space would break if executables or devices appear on proc */
> >  s->s_iflags |= SB_I_USERNS_VISIBLE | SB_I_NOEXEC | SB_I_NODEV;
> >  s->s_flags |= SB_NODIRATIME | SB_NOSUID | SB_NOEXEC; diff --git
> > a/fs/proc/root.c b/fs/proc/root.c index f4b1a9d..f5f3bf3 100644
> > --- a/fs/proc/root.c
> > +++ b/fs/proc/root.c
> > @@ -98,6 +98,9 @@ static struct dentry *proc_mount(struct file_system_type *fs_type,
> >  ns = task_active_pid_ns(current);
> >  }
> >
> > +if (!proc_parse_options(data, ns))
> > +return ERR_PTR(-EINVAL);
> > +
> >  return mount_ns(fs_type, flags, data, ns, ns->user_ns,
> > proc_fill_super);  }
>
> Other filesystems parse the options from fill_super().  Is proc special in some fashion?
>
> #/******本邮件及其附件含有小米公司的保密信息，仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本邮件！ This e-mail and its attachments contain confidential information from XIAOMI, which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction, or dissemination) by persons other than the intended recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender by phone or email immediately and delete it!******/#

Re: > [PATCH] Security: Handle hidepid option correctly

[程洋]

@Nick. Would mind giving this patch an "Acked-by"?
This issue causes any Android who uses latest kernel cannot mount proc
with "hidepid=2" option. Which causes problems
程洋 <d17103513@gmail.com> 于2018年12月5日周三 下午3:26写道：
>
> Anyone who can review my patch?
>
> 程洋 <chengyang@xiaomi.com> 于2018年11月30日周五 上午10:34写道：
> >
> > Here is an article illustrates the details.
> > https://medium.com/@topjohnwu/from-anime-game-to-android-system-security-vulnerability-9b955a182f20
> >
> > And There is a similar fix on kernel-4.4:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=99663be772c827b8f5f594fe87eb4807be1994e5
> >
> > Q: Other filesystems parse the options from fill_super().  Is proc special in some fashion?
> > A: According to my research, start_kernel will call proc_mount first, and initialize sb->s_root before any userspace process runs. If others want to mount it, all options will be ignored.
> >      AOSP change here: https://android-review.googlesource.com/c/platform/system/core/+/181345/4/init/init.cpp
> >      At first I though we should mount it with MS_REMOUNT flag. But kernel will crash if we did this.
> >
> > Q:  Why is this considered to be security sensitive?  I can guess, but I'd like to know your reasoning.
> > A: See the article above. It's part of Android sanbox.
> >
> >
> > > [PATCH] Security: Handle hidepid option correctly
> >
> > Why is this considered to be security sensitive?  I can guess, but I'd like to know your reasoning.
> >
> > On Thu, 29 Nov 2018 19:08:21 +0800 mailto:d17103513@gmail.com wrote:
> >
> > > From: Cheng Yang <mailto:chengyang@xiaomi.com>
> > >
> > > The proc_parse_options() call from proc_mount() runs only once at boot
> > > time.  So on any later mount attempt, any mount options are ignored
> > > because ->s_root is already initialized.
> > > As a consequence, "mount -o <options>" will ignore the options.  The
> > > only way to change mount options is "mount -o remount,<options>".
> > > To fix this, parse the mount options unconditionally.
> > >
> > > --- a/fs/proc/inode.c
> > > +++ b/fs/proc/inode.c
> > > @@ -493,13 +493,9 @@ 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 pid_namespace *ns = get_pid_ns(s->s_fs_info);
> > >  struct inode *root_inode;
> > >  int ret;
> > >
> > > -if (!proc_parse_options(data, ns))
> > > -return -EINVAL;
> > > -
> > >  /* User space would break if executables or devices appear on proc */
> > >  s->s_iflags |= SB_I_USERNS_VISIBLE | SB_I_NOEXEC | SB_I_NODEV;
> > >  s->s_flags |= SB_NODIRATIME | SB_NOSUID | SB_NOEXEC; diff --git
> > > a/fs/proc/root.c b/fs/proc/root.c index f4b1a9d..f5f3bf3 100644
> > > --- a/fs/proc/root.c
> > > +++ b/fs/proc/root.c
> > > @@ -98,6 +98,9 @@ static struct dentry *proc_mount(struct file_system_type *fs_type,
> > >  ns = task_active_pid_ns(current);
> > >  }
> > >
> > > +if (!proc_parse_options(data, ns))
> > > +return ERR_PTR(-EINVAL);
> > > +
> > >  return mount_ns(fs_type, flags, data, ns, ns->user_ns,
> > > proc_fill_super);  }
> >
> > Other filesystems parse the options from fill_super().  Is proc special in some fashion?
> >
> > #/******本邮件及其附件含有小米公司的保密信息，仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本邮件！ This e-mail and its attachments contain confidential information from XIAOMI, which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction, or dissemination) by persons other than the intended recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender by phone or email immediately and delete it!******/#

Re: > [PATCH] Security: Handle hidepid option correctly

[Alexey Dobriyan]

On Wed, Dec 05, 2018 at 03:26:04PM +0800, 程洋 wrote:
> Anyone who can review my patch?
> 
> 程洋 <chengyang@xiaomi.com> 于2018年11月30日周五 上午10:34写道：
> >
> > Here is an article illustrates the details.
> > https://medium.com/@topjohnwu/from-anime-game-to-android-system-security-vulnerability-9b955a182f20
> >
> > And There is a similar fix on kernel-4.4:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=99663be772c827b8f5f594fe87eb4807be1994e5
> >
> > Q: Other filesystems parse the options from fill_super().  Is proc special in some fashion?
> > A: According to my research, start_kernel will call proc_mount first, and initialize sb->s_root before any userspace process runs. If others want to mount it, all options will be ignored.
> >      AOSP change here: https://android-review.googlesource.com/c/platform/system/core/+/181345/4/init/init.cpp
> >      At first I though we should mount it with MS_REMOUNT flag. But kernel will crash if we did this.

This is not true: /proc is mounted by userspace (and it is easy to see
from the fact that proc_mount() is not called from kernel anywhere).

hidepid= in its current form is misdesigned, so might as well not bother
changing anything. IIRC there were(?) patches to make it per-mount.

Re: > [PATCH] Security: Handle hidepid option correctly

[程洋]

Actually I'm pretty sure kernel calls proc_mount()
Here is the call stack
[    0.003450] [<ffffff8bef2a0190>] proc_mount+0x2c/0x98
[    0.003459] [<ffffff8bef22e560>] mount_fs+0x164/0x190
[    0.003465] [<ffffff8bef24c138>] vfs_kern_mount+0x74/0x168
[    0.003469] [<ffffff8bef24c244>] kern_mount_data+0x18/0x30
[    0.003474] [<ffffff8bef2a0258>] pid_ns_prepare_proc+0x24/0x40
[    0.003484] [<ffffff8bef0cd5ec>] alloc_pid+0x498/0x4b4
[    0.003492] [<ffffff8bef0a9b94>] copy_process.isra.73.part.74+0xed0/0x1708
[    0.003496] [<ffffff8bef0aa560>] _do_fork+0xdc/0x3f8
[    0.003501] [<ffffff8bef0aa8c8>] kernel_thread+0x34/0x3c
[    0.003511] [<ffffff8bf00cd498>] rest_init+0x20/0x80
[    0.003522] [<ffffff8bf0c00c7c>] start_kernel+0x3e4/0x43c
[    0.003527] [<ffffff8bf0c001e8>] __primary_switched+0x64/0x90
Alexey Dobriyan <adobriyan@gmail.com> 于2018年12月14日周五 下午11:44写道：
>
> On Wed, Dec 05, 2018 at 03:26:04PM +0800, 程洋 wrote:
> > Anyone who can review my patch?
> >
> > 程洋 <chengyang@xiaomi.com> 于2018年11月30日周五 上午10:34写道：
> > >
> > > Here is an article illustrates the details.
> > > https://medium.com/@topjohnwu/from-anime-game-to-android-system-security-vulnerability-9b955a182f20
> > >
> > > And There is a similar fix on kernel-4.4:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=99663be772c827b8f5f594fe87eb4807be1994e5
> > >
> > > Q: Other filesystems parse the options from fill_super().  Is proc special in some fashion?
> > > A: According to my research, start_kernel will call proc_mount first, and initialize sb->s_root before any userspace process runs. If others want to mount it, all options will be ignored.
> > >      AOSP change here: https://android-review.googlesource.com/c/platform/system/core/+/181345/4/init/init.cpp
> > >      At first I though we should mount it with MS_REMOUNT flag. But kernel will crash if we did this.
>
> This is not true: /proc is mounted by userspace (and it is easy to see
> from the fact that proc_mount() is not called from kernel anywhere).
>
> hidepid= in its current form is misdesigned, so might as well not bother
> changing anything. IIRC there were(?) patches to make it per-mount.

Re: > [PATCH] Security: Handle hidepid option correctly

[Alexey Dobriyan]

On Mon, Dec 17, 2018 at 12:21:40PM +0800, 程洋 wrote:
> Actually I'm pretty sure kernel calls proc_mount()
> Here is the call stack

OK, hidepid= is still misdesigned. :-(