From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 34503CA9ED0 for ; Sun, 3 Nov 2019 18:51:13 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 1AB802190F for ; Sun, 3 Nov 2019 18:51:13 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728017AbfKCSvK (ORCPT ); Sun, 3 Nov 2019 13:51:10 -0500 Received: from out01.mta.xmission.com ([166.70.13.231]:42093 "EHLO out01.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727322AbfKCSvJ (ORCPT ); Sun, 3 Nov 2019 13:51:09 -0500 Received: from in02.mta.xmission.com ([166.70.13.52]) by out01.mta.xmission.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.87) (envelope-from ) id 1iRKxv-0004oZ-Jg; Sun, 03 Nov 2019 11:51:07 -0700 Received: from ip68-227-160-95.om.om.cox.net ([68.227.160.95] helo=x220.xmission.com) by in02.mta.xmission.com with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.87) (envelope-from ) id 1iRKxu-0002B7-Ob; Sun, 03 Nov 2019 11:51:07 -0700 From: ebiederm@xmission.com (Eric W. Biederman) To: Topi Miettinen Cc: Luis Chamberlain , Kees Cook , Alexey Dobriyan , "linux-kernel\@vger.kernel.org" , "open list\:FILESYSTEMS \(VFS and infrastructure\)" 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 X-XM-SPF: eid=1iRKxu-0002B7-Ob;;;mid=<87d0e8g5f4.fsf@x220.int.ebiederm.org>;;;hst=in02.mta.xmission.com;;;ip=68.227.160.95;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX18+wN/gS3wwKlC/YmuKcs2nkjhNP/60xnQ= X-SA-Exim-Connect-IP: 68.227.160.95 X-SA-Exim-Mail-From: ebiederm@xmission.com Subject: Re: [PATCH] Allow restricting permissions in /proc/sys X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -0600) X-SA-Exim-Scanned: Yes (on in02.mta.xmission.com) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Topi Miettinen 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 > --- > 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; > }