linux-kernel - Re: [PATCH v1] devpts: resolve devpts bind-mounts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+55aFyMHKSA4V7Y7qN5_F4R=hpD+W41FFYCE8b+FrFSeXQMtQ@mail.gmail.com>
Date:   Fri, 9 Mar 2018 10:37:34 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Christian Brauner <christian.brauner@...ntu.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "Eric W. Biederman" <ebiederm@...ssion.com>
Subject: Re: [PATCH v1] devpts: resolve devpts bind-mounts

Hmm. This hunk annoys me and makes me go "Whaa?":

On Fri, Mar 9, 2018 at 2:57 AM, Christian Brauner
<christian.brauner@...ntu.com> wrote:
> @@ -163,6 +159,26 @@ struct vfsmount *devpts_mntget(struct file *filp, struct pts_fs_info *fsi)
>
>         path = filp->f_path;
>         path_get(&path);
> +       if ((DEVPTS_SB(path.mnt->mnt_sb) == fsi) &&
> +           (path.mnt->mnt_root == fsi->ptmx_dentry)) {
> +               /* Walk upward while the start point is a bind mount of a single
> +                * file.
> +                */
> +               while (path.mnt->mnt_root == path.dentry)
> +                       if (follow_up(&path) == 0)
> +                               break;
> +
> +               /* Is this path a valid devpts filesystem? */
> +               err = devpts_ptmx_path(&path);
> +               dput(path.dentry);
> +               if (err == 0)
> +                       goto check_devpts_sb;
> +
> +               path_put(&path);
> +               path = filp->f_path;
> +               path_get(&path);
> +               goto check_devpts_sb;
> +       }
>
>         err = devpts_ptmx_path(&path);
>         dput(path.dentry);

why did you duplicate the devpts_ptmx_path() and then do that odd
error handling?

We only go into that "if()" statement if
DEVPTS_SB(filp->f_path.mnt->mnt_sb) == fsi, so then when you do that
"put path and re-get it, and go to check_devpts_sb", the
check_devpts_sb won't actually _do_ anything, because it has

> +check_devpts_sb:
>         if (DEVPTS_SB(path.mnt->mnt_sb) != fsi) {

and we know that "if()" there cannot trigger, since we just checked it earlier.

So abou two thirds of the above seems unnecessary.

Why isn't the code just doing


       if ((DEVPTS_SB(path.mnt->mnt_sb) == fsi) &&
           (path.mnt->mnt_root == fsi->ptmx_dentry)) {
               /* Walk upward while the start point is a bind mount of a single
                * file.
                */
               while (path.mnt->mnt_root == path.dentry)
                       if (follow_up(&path) == 0)
                               break;
        }

and then just falling through to the existing "devpts_ptmx_path()" etc
code? Duplicating it seems wrong, and the error handling in the
duplicated path seems wrong too.

Am I missing something?


> @@ -187,10 +206,16 @@ struct pts_fs_info *devpts_acquire(struct file *filp)
>         path = filp->f_path;
>         path_get(&path);
>
> -       err = devpts_ptmx_path(&path);
> -       if (err) {
> -               result = ERR_PTR(err);
> -               goto out;
> +       /* Has the devpts filesystem already been found? */
> +       if (path.mnt->mnt_sb->s_magic != DEVPTS_SUPER_MAGIC) {
> +               /* Is there an appropriate devpts filesystem in the parent
> +                * directory?
> +                */
> +               err = devpts_ptmx_path(&path);
> +               if (err) {
> +                       result = ERR_PTR(err);
> +                       goto out;
> +               }
>         }

This part (and the accompanying removal from devpts_ptmx_path() should
just have been a separate preparatory patch that doesn't change
semantics, no? Also, the scope of 'err' is now entirely inside that
if(), so I think it should just be declared there too.

I didn't actually test the patch, this is just from reading it, so I
might have missed something.

                Linus