Message-ID: <CADd0cq2xp2_Nk2dF_3BuS7+226MBSzHsgAtAwKLO5KbXrugi7A@mail.gmail.com>
Date:   Fri, 30 Nov 2018 15:34:55 +0800
From:   程洋 <d17103513@...il.com>
To:     Andrew Morton <akpm@...ux-foundation.org>
Cc:     Alexey Dobriyan <adobriyan@...il.com>,
        David Howells <dhowells@...hat.com>,
        "Peter Zijlstra (Intel)" <peterz@...radead.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        Johannes Weiner <hannes@...xchg.org>,
        Davidlohr Bueso <dbueso@...e.de>, linux-kernel@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, panshuangquan@...omi.com
Subject: 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@...omi.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@...il.com wrote:
>
> > From: Cheng Yang <mailto:chengyang@...omi.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!******/#

Powered by blists - more mailing lists