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@xxxxxxxxx> ä2018å12æ5æåä äå3:26åéï
>
> Anyone who can review my patch?
>
> çæ <chengyang@xxxxxxxxxx> ä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@xxxxxxxxx wrote:
> >
> > > From: Cheng Yang <mailto:chengyang@xxxxxxxxxx>
> > >
> > > 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!******/#