Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   ebiederm@xmission.com (Eric W. Biederman)
To:     Topi Miettinen <toiwoton@gmail.com>
Cc:     Luis Chamberlain <mcgrof@kernel.org>,
        Kees Cook <keescook@chromium.org>,
        Alexey Dobriyan <adobriyan@gmail.com>,
        "linux-kernel\@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "open list\:FILESYSTEMS \(VFS and infrastructure\)" 
        <linux-fsdevel@vger.kernel.org>
References: <74a91362-247c-c749-5200-7bdce704ed9e@gmail.com>
Date:   Sun, 03 Nov 2019 12:50:55 -0600
In-Reply-To: <74a91362-247c-c749-5200-7bdce704ed9e@gmail.com> (Topi
        Miettinen's message of "Sun, 3 Nov 2019 16:55:48 +0200")
Message-ID: <87d0e8g5f4.fsf@x220.int.ebiederm.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PATCH] Allow restricting permissions in /proc/sys
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Topi Miettinen <toiwoton@gmail.com> writes:

> Several items in /proc/sys need not be accessible to unprivileged
> tasks. Let the system administrator change the permissions, but only
> to more restrictive modes than what the sysctl tables allow.

This looks quite buggy.  You neither update table->mode nor
do you ever read from table->mode to initialize the inode.
I am missing something in my quick reading of your patch?

The not updating table->mode almost certainly means that as soon as the
cached inode is invalidated the mode changes will disappear.  Not to
mention they will fail to propogate between  different instances of
proc.

Loosing all of your changes at cache invalidation seems to make this a
useless feature.

Eric


> Signed-off-by: Topi Miettinen <toiwoton@gmail.com>
> ---
>  fs/proc/proc_sysctl.c | 41 +++++++++++++++++++++++++++++++----------
>  1 file changed, 31 insertions(+), 10 deletions(-)
>
> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index d80989b6c344..88c4ca7d2782 100644
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -818,6 +818,10 @@ static int proc_sys_permission(struct inode *inode, int
> mask)
>         if ((mask & MAY_EXEC) && S_ISREG(inode->i_mode))
>                 return -EACCES;
>
> +       error = generic_permission(inode, mask);
> +       if (error)
> +               return error;
> +
>         head = grab_header(inode);
>         if (IS_ERR(head))
>                 return PTR_ERR(head);
> @@ -837,9 +841,35 @@ static int proc_sys_setattr(struct dentry *dentry, struct
> iattr *attr)
>         struct inode *inode = d_inode(dentry);
>         int error;
>
> -       if (attr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
> +       if (attr->ia_valid & (ATTR_UID | ATTR_GID))
>                 return -EPERM;
>
> +       if (attr->ia_valid & ATTR_MODE) {
> +               struct ctl_table_header *head = grab_header(inode);
> +               struct ctl_table *table = PROC_I(inode)->sysctl_entry;
> +               umode_t max_mode = 0777; /* Only these bits may change */
> +
> +               if (IS_ERR(head))
> +                       return PTR_ERR(head);
> +
> +               if (!table) /* global root - r-xr-xr-x */
> +                       max_mode &= ~0222;
> +               else /*
> +                     * Don't allow permissions to become less
> +                     * restrictive than the sysctl table entry
> +                     */
> +                       max_mode &= table->mode;
> +
> +               sysctl_head_finish(head);
> +
> +               /* Execute bits only allowed for directories */
> +               if (!S_ISDIR(inode->i_mode))
> +                       max_mode &= ~0111;
> +
> +               if (attr->ia_mode & ~S_IFMT & ~max_mode)
> +                       return -EPERM;
> +       }
> +
>         error = setattr_prepare(dentry, attr);
>         if (error)
>                 return error;
> @@ -853,17 +883,8 @@ static int proc_sys_getattr(const struct path *path, struct
> kstat *stat,
>                             u32 request_mask, unsigned int query_flags)
>  {
>         struct inode *inode = d_inode(path->dentry);
> -       struct ctl_table_header *head = grab_header(inode);
> -       struct ctl_table *table = PROC_I(inode)->sysctl_entry;
> -
> -       if (IS_ERR(head))
> -               return PTR_ERR(head);
>
>         generic_fillattr(inode, stat);
> -       if (table)
> -               stat->mode = (stat->mode & S_IFMT) | table->mode;
> -
> -       sysctl_head_finish(head);
>         return 0;
>  }