From: Christian Brauner <brauner@kernel.org>
To: Amir Goldstein <amir73il@gmail.com>,
Miklos Szeredi <miklos@szeredi.hu>,
"Darrick J . Wong" <djwong@kernel.org>
Cc: Christian Brauner <brauner@kernel.org>,
Al Viro <viro@zeniv.linux.org.uk>, Christoph Hellwig <hch@lst.de>,
Seth Forshee <sforshee@kernel.org>,
Yang Xu <xuyang2018.jy@fujitsu.com>,
Filipe Manana <fdmanana@kernel.org>,
linux-unionfs@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: [PATCH v2 2/5] attr: add should_remove_sgid()
Date: Fri, 7 Oct 2022 16:05:40 +0200 [thread overview]
Message-ID: <20221007140543.1039983-3-brauner@kernel.org> (raw)
In-Reply-To: <20221007140543.1039983-1-brauner@kernel.org>
The current setgid stripping logic during write and ownership change
operations is inconsistent and strewn over multiple places. In order to
consolidate it and make more consistent we'll add a new helper
should_remove_sgid(). The function retains the old behavior where we
remove the S_ISGID bit unconditionally when S_IXGRP is set but also when
it isn't set and the caller is neither in the group of the inode nor
privileged over the inode.
We will use this helper both in write operation permission removal such
as file_remove_privs() as well as in ownership change operations.
Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
Notes:
/* v2 */
Dave Chinner <dchinner@redhat.com>:
- Use easier to follow logic in the new helper.
fs/attr.c | 27 +++++++++++++++++++++++++++
fs/internal.h | 2 ++
2 files changed, 29 insertions(+)
diff --git a/fs/attr.c b/fs/attr.c
index b1cff6f5b715..d0bb1dae425e 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -39,6 +39,33 @@ static bool setattr_drop_sgid(struct user_namespace *mnt_userns,
return true;
}
+/**
+ * should_remove_sgid - determine whether the setgid bit needs to be removed
+ * @mnt_userns: User namespace of the mount the inode was created from
+ * @inode: inode to check
+ *
+ * This function determines whether the setgid bit needs to be removed.
+ * We retain backwards compatibility and require setgid bit to be removed
+ * unconditionally if S_IXGRP is set. Otherwise we have the exact same
+ * requirements as setattr_prepare() and setattr_copy().
+ *
+ * Return: ATTR_KILL_SGID if setgid bit needs to be removed, 0 otherwise.
+ */
+int should_remove_sgid(struct user_namespace *mnt_userns,
+ const struct inode *inode)
+{
+ umode_t mode = inode->i_mode;
+
+ if (!(mode & S_ISGID))
+ return 0;
+ if (mode & S_IXGRP)
+ return ATTR_KILL_SGID;
+ if (setattr_drop_sgid(mnt_userns, inode,
+ i_gid_into_vfsgid(mnt_userns, inode)))
+ return ATTR_KILL_SGID;
+ return 0;
+}
+
/**
* chown_ok - verify permissions to chown inode
* @mnt_userns: user namespace of the mount @inode was found from
diff --git a/fs/internal.h b/fs/internal.h
index 87e96b9024ce..9d165ab65a2a 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -221,3 +221,5 @@ ssize_t do_getxattr(struct user_namespace *mnt_userns,
int setxattr_copy(const char __user *name, struct xattr_ctx *ctx);
int do_setxattr(struct user_namespace *mnt_userns, struct dentry *dentry,
struct xattr_ctx *ctx);
+int should_remove_sgid(struct user_namespace *mnt_userns,
+ const struct inode *inode);
--
2.34.1
next prev parent reply other threads:[~2022-10-07 14:06 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-07 14:05 [PATCH v2 0/5] fs: improve setgid stripping consistency even more Christian Brauner
2022-10-07 14:05 ` [PATCH v2 1/5] attr: add setattr_drop_sgid() Christian Brauner
2022-10-11 8:11 ` Amir Goldstein
2022-10-11 8:57 ` Christian Brauner
2022-10-07 14:05 ` Christian Brauner [this message]
2022-10-07 19:16 ` [PATCH v2 2/5] attr: add should_remove_sgid() kernel test robot
2022-10-08 5:56 ` Christian Brauner
2022-10-11 8:18 ` Amir Goldstein
2022-10-11 8:46 ` Christian Brauner
2022-10-07 14:05 ` [PATCH v2 3/5] attr: use consistent sgid stripping checks Christian Brauner
2022-10-11 8:43 ` Amir Goldstein
2022-10-11 8:56 ` Christian Brauner
2022-10-11 11:07 ` Amir Goldstein
2022-10-11 13:48 ` Christian Brauner
2022-10-07 14:05 ` [PATCH v2 4/5] ovl: remove privs in ovl_copyfile() Christian Brauner
2022-10-07 14:05 ` [PATCH v2 5/5] ovl: remove privs in ovl_fallocate() Christian Brauner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20221007140543.1039983-3-brauner@kernel.org \
--to=brauner@kernel.org \
--cc=amir73il@gmail.com \
--cc=djwong@kernel.org \
--cc=fdmanana@kernel.org \
--cc=hch@lst.de \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-unionfs@vger.kernel.org \
--cc=miklos@szeredi.hu \
--cc=sforshee@kernel.org \
--cc=viro@zeniv.linux.org.uk \
--cc=xuyang2018.jy@fujitsu.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).