Message-ID: <CADd0cq3UZPfzdUXb_XtL23L-xHNjF-xV=wNVNDiF-NYPreioZw@mail.gmail.com>
Date:   Fri, 30 Nov 2018 13:58:14 +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
Subject: 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@...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