Re: [PATCH] fs: account for group membership

[Seth Forshee <sforshee@digitalocean.com>]

On Mon, Jun 13, 2022 at 01:15:17PM +0200, Christian Brauner wrote:
> When calling setattr_prepare() to determine the validity of the
> attributes the ia_{g,u}id fields contain the value that will be written
> to inode->i_{g,u}id. This is exactly the same for idmapped and
> non-idmapped mounts and allows callers to pass in the values they want
> to see written to inode->i_{g,u}id.
> 
> When group ownership is changed a caller whose fsuid owns the inode can
> change the group of the inode to any group they are a member of. When
> searching through the caller's groups we need to use the gid mapped
> according to the idmapped mount otherwise we will fail to change
> ownership for unprivileged users.
> 
> Consider a caller running with fsuid and fsgid 1000 using an idmapped
> mount that maps id 65534 to 1000 and 65535 to 1001. Consequently, a file
> owned by 65534:65535 in the filesystem will be owned by 1000:1001 in the
> idmapped mount.
> 
> The caller now requests the gid of the file to be changed to 1000 going
> through the idmapped mount. In the vfs we will immediately map the
> requested gid to the value that will need to be written to inode->i_gid
> and place it in attr->ia_gid. Since this idmapped mount maps 65534 to
> 1000 we place 65534 in attr->ia_gid.
> 
> When we check whether the caller is allowed to change group ownership we
> first validate that their fsuid matches the inode's uid. The
> inode->i_uid is 65534 which is mapped to uid 1000 in the idmapped mount.
> Since the caller's fsuid is 1000 we pass the check.
> 
> We now check whether the caller is allowed to change inode->i_gid to the
> requested gid by calling in_group_p(). This will compare the passed in
> gid to the caller's fsgid and search the caller's additional groups.
> 
> Since we're dealing with an idmapped mount we need to pass in the gid
> mapped according to the idmapped mount. This is akin to checking whether
> a caller is privileged over the future group the inode is owned by. And
> that needs to take the idmapped mount into account. Note, all helpers
> are nops without idmapped mounts.
> 
> New regression test sent to xfstests.
> 
> Link: https://github.com/lxc/lxd/issues/10537
> Fixes: 2f221d6f7b88 ("attr: handle idmapped mounts")
> Cc: Seth Forshee <seth.forshee@digitalocean.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Aleksa Sarai <cyphar@cyphar.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: stable@vger.kernel.org # 5.15+
> CC: linux-fsdevel@vger.kernel.org
> Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>

Looks correct to me.

Reviewed-by: Seth Forshee <sforshee@digitalocean.com>