Re: [PATCH] devpts: Add ptmx_uid and ptmx_gid options

[James Bottomley]

On Thu, 2015-03-26 at 12:29 -0700, Andy Lutomirski wrote:
> Ping?  It's been over a month.

I think we all looked at this and thought "that's not a problem".  The
reason is that we all bring up full OS containers with devpts already
mounted by the host.  Even when you run from init in the Container, the
OS always seems to be prepared to find devpts already mounted.  Can you
explain a bit more what the actual problem you're trying to solve is?  I
think also I'm a bit dubious about allowing this type of flexibility
because the more lattitude container root has, the more scope there is
for a security problem.

> On Fri, Feb 20, 2015 at 5:04 PM, Andy Lutomirski <luto@xxxxxxxxxxxxxx> wrote:
> > It's currently impossible to mount devpts in a user namespace that
> > has no root user, since ptmx can't be created.

This is where I stopped reading because it's not true ... because it is
possible, you just do it from the host as real root.

James

>   This adds options
> > ptmx_uid and ptmx_gid that override the default uid and gid of 0.
> >
> > These options are not shown in mountinfo because they have no effect
> > other than changing the initial mode of ptmx, and, in particular, it
> > wouldn't make any sense to change them on remount.  Instead, we
> > disallow them on remount.
> >
> > This could be changed, but we'd probably want to fix the userns
> > behavior of uid and gid at the same time if we did so.
> >
> > Signed-off-by: Andy Lutomirski <luto@xxxxxxxxxxxxxx>
> > ---
> >  Documentation/filesystems/devpts.txt |  4 +++
> >  fs/devpts/inode.c                    | 58 ++++++++++++++++++++++++++----------
> >  2 files changed, 46 insertions(+), 16 deletions(-)
> >
> > diff --git a/Documentation/filesystems/devpts.txt b/Documentation/filesystems/devpts.txt
> > index 68dffd87f9b7..7808e77d0d72 100644
> > --- a/Documentation/filesystems/devpts.txt
> > +++ b/Documentation/filesystems/devpts.txt
> > @@ -121,6 +121,10 @@ once), following user-space issues should be noted.
> >
> >         chmod 666 /dev/pts/ptmx
> >
> > +   The ownership for /dev/pts/ptmx can be specified using the ptmxuid
> > +   and ptmxgid options.  Both default to zero, which, in user namespaces
> > +   that have no root user, will cause mounting to fail.
> > +
> >  7. A mount of devpts without the 'newinstance' option results in binding to
> >     initial kernel mount.  This behavior while preserving legacy semantics,
> >     does not provide strict isolation in a container environment. i.e by
> > diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
> > index cfe8466f7fef..b60d1438c660 100644
> > --- a/fs/devpts/inode.c
> > +++ b/fs/devpts/inode.c
> > @@ -102,6 +102,8 @@ struct pts_mount_opts {
> >         int setgid;
> >         kuid_t   uid;
> >         kgid_t   gid;
> > +       uid_t ptmx_uid;
> > +       gid_t ptmx_gid;
> >         umode_t mode;
> >         umode_t ptmxmode;
> >         int newinstance;
> > @@ -109,8 +111,8 @@ struct pts_mount_opts {
> >  };
> >
> >  enum {
> > -       Opt_uid, Opt_gid, Opt_mode, Opt_ptmxmode, Opt_newinstance,  Opt_max,
> > -       Opt_err
> > +       Opt_uid, Opt_gid, Opt_ptmx_uid, Opt_ptmx_gid, Opt_mode, Opt_ptmxmode,
> > +       Opt_newinstance,  Opt_max, Opt_err,
> >  };
> >
> >  static const match_table_t tokens = {
> > @@ -118,6 +120,8 @@ static const match_table_t tokens = {
> >         {Opt_gid, "gid=%u"},
> >         {Opt_mode, "mode=%o"},
> >  #ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
> > +       {Opt_ptmx_uid, "ptmxuid=%u"},
> > +       {Opt_ptmx_gid, "ptmxgid=%u"},
> >         {Opt_ptmxmode, "ptmxmode=%o"},
> >         {Opt_newinstance, "newinstance"},
> >         {Opt_max, "max=%d"},
> > @@ -162,14 +166,17 @@ static int parse_mount_options(char *data, int op, struct pts_mount_opts *opts)
> >         char *p;
> >         kuid_t uid;
> >         kgid_t gid;
> > -
> > -       opts->setuid  = 0;
> > -       opts->setgid  = 0;
> > -       opts->uid     = GLOBAL_ROOT_UID;
> > -       opts->gid     = GLOBAL_ROOT_GID;
> > -       opts->mode    = DEVPTS_DEFAULT_MODE;
> > +       bool setptmxid = false;
> > +
> > +       opts->setuid   = 0;
> > +       opts->setgid   = 0;
> > +       opts->uid      = GLOBAL_ROOT_UID;
> > +       opts->gid      = GLOBAL_ROOT_GID;
> > +       opts->ptmx_uid = 0;
> > +       opts->ptmx_gid = 0;
> > +       opts->mode     = DEVPTS_DEFAULT_MODE;
> >         opts->ptmxmode = DEVPTS_DEFAULT_PTMX_MODE;
> > -       opts->max     = NR_UNIX98_PTY_MAX;
> > +       opts->max      = NR_UNIX98_PTY_MAX;
> >
> >         /* newinstance makes sense only on initial mount */
> >         if (op == PARSE_MOUNT)
> > @@ -209,6 +216,22 @@ static int parse_mount_options(char *data, int op, struct pts_mount_opts *opts)
> >                         opts->mode = option & S_IALLUGO;
> >                         break;
> >  #ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
> > +               case Opt_ptmx_uid:
> > +                       if (match_int(&args[0], &option))
> > +                               return -EINVAL;
> > +                       if (op != PARSE_MOUNT)
> > +                               return -EINVAL;
> > +                       opts->ptmx_uid = option;
> > +                       setptmxid = true;
> > +                       break;
> > +               case Opt_ptmx_gid:
> > +                       if (match_int(&args[0], &option))
> > +                               return -EINVAL;
> > +                       if (op != PARSE_MOUNT)
> > +                               return -EINVAL;
> > +                       opts->ptmx_gid = option;
> > +                       setptmxid = true;
> > +                       break;
> >                 case Opt_ptmxmode:
> >                         if (match_octal(&args[0], &option))
> >                                 return -EINVAL;
> > @@ -232,6 +255,9 @@ static int parse_mount_options(char *data, int op, struct pts_mount_opts *opts)
> >                 }
> >         }
> >
> > +       if (setptmxid && !opts->newinstance)
> > +               return -EINVAL;
> > +
> >         return 0;
> >  }
> >
> > @@ -245,12 +271,12 @@ static int mknod_ptmx(struct super_block *sb)
> >         struct dentry *root = sb->s_root;
> >         struct pts_fs_info *fsi = DEVPTS_SB(sb);
> >         struct pts_mount_opts *opts = &fsi->mount_opts;
> > -       kuid_t root_uid;
> > -       kgid_t root_gid;
> > +       kuid_t ptmx_uid;
> > +       kgid_t ptmx_gid;
> >
> > -       root_uid = make_kuid(current_user_ns(), 0);
> > -       root_gid = make_kgid(current_user_ns(), 0);
> > -       if (!uid_valid(root_uid) || !gid_valid(root_gid))
> > +       ptmx_uid = make_kuid(current_user_ns(), opts->ptmx_uid);
> > +       ptmx_gid = make_kgid(current_user_ns(), opts->ptmx_gid);
> > +       if (!uid_valid(ptmx_uid) || !gid_valid(ptmx_gid))
> >                 return -EINVAL;
> >
> >         mutex_lock(&root->d_inode->i_mutex);
> > @@ -282,8 +308,8 @@ static int mknod_ptmx(struct super_block *sb)
> >
> >         mode = S_IFCHR|opts->ptmxmode;
> >         init_special_inode(inode, mode, MKDEV(TTYAUX_MAJOR, 2));
> > -       inode->i_uid = root_uid;
> > -       inode->i_gid = root_gid;
> > +       inode->i_uid = ptmx_uid;
> > +       inode->i_gid = ptmx_gid;
> >
> >         d_add(dentry, inode);
> >
> > --
> > 2.3.0
> >
> 
> 
> 



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/