| 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
| ||
|
Message-ID: <YNA03/Y3KxHYuoLu@B-P7TQMD6M-0146.local>
Date: Mon, 21 Jun 2021 14:42:39 +0800
From: Gao Xiang <hsiangkao@...ux.alibaba.com>
To: linux-nfs@...r.kernel.org
Cc: LKML <linux-kernel@...r.kernel.org>,
Trond Myklebust <trond.myklebust@...merspace.com>,
Anna Schumaker <anna.schumaker@...app.com>,
Joseph Qi <joseph.qi@...ux.alibaba.com>,
Andreas Gruenbacher <agruenba@...hat.com>
Subject: Re: [RFC PATCH 2/1] nfs: NFSv3: fix SGID bit dropped when inheriting
ACLs
( also +cc Andreas Gruenbacher )
tested NFSv3 with -g quick in my environment:
Vanilla:
Failures: generic/258 generic/294 generic/444 generic/467 generic/477 generic/531
Patched:
Failures: generic/258 generic/294 generic/467 generic/477 generic/531
It'd be much helpful to get some hints of this, am I missing something?
Many thanks!
Thanks,
Gao Xiang
On Fri, Jun 18, 2021 at 05:12:25PM +0800, Gao Xiang wrote:
> generic/444 fails with NFSv3 as shown above, "
> QA output created by 444
> drwxrwsr-x
> -drwxrwsr-x
> +drwxrwxr-x
> ", which tests "SGID inheritance with default ACLs" fs regression
> and looks after the following commits:
>
> a3bb2d558752 ("ext4: Don't clear SGID when inheriting ACLs")
> 073931017b49 ("posix_acl: Clear SGID bit when setting file permissions")
>
> commit 055ffbea0596 ("[PATCH] NFS: Fix handling of the umask when
> an NFSv3 default acl is present.") sets acls explicitly when
> when files are created in a directory that has a default ACL.
> However, after commit a3bb2d558752 and 073931017b49, SGID can be
> dropped if user is not member of the owning group with
> set_posix_acl() called.
>
> Since underlayfs will handle ACL inheritance when creating
> files in a directory that has the default ACL and the umask is
> supposed to be ignored for such case. Therefore, I think no need
> to set acls explicitly (to avoid SGID bit cleared) but only apply
> client umask if the default ACL of the parent directory doesn't
> exist.
>
> With this patch, generic/444 can pass now.
>
> Cc: Trond Myklebust <trond.myklebust@...merspace.com>
> Cc: Anna Schumaker <anna.schumaker@...app.com>
> Cc: Joseph Qi <joseph.qi@...ux.alibaba.com>
> Signed-off-by: Gao Xiang <hsiangkao@...ux.alibaba.com>
> ---
>
> I didn't find the original discussion with Buck Huppmann <buchk@...ox.com>
> about this topic mentioned in
> https://lore.kernel.org/r/20050122203620.108564000@blunzn.suse.de/
>
> and it's just a rough thought about this issue...
>
> fs/nfs/nfs3proc.c | 43 ++++++++++++++++---------------------------
> 1 file changed, 16 insertions(+), 27 deletions(-)
>
> diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
> index 2299446b3b89..a5676be676be 100644
> --- a/fs/nfs/nfs3proc.c
> +++ b/fs/nfs/nfs3proc.c
> @@ -339,7 +339,7 @@ static void nfs3_free_createdata(struct nfs3_createdata *data)
> nfs3_proc_create(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
> int flags)
> {
> - struct posix_acl *default_acl, *acl;
> + struct posix_acl *pacl;
> struct nfs3_createdata *data;
> struct dentry *d_alias;
> int status = -ENOMEM;
> @@ -350,6 +350,10 @@ static void nfs3_free_createdata(struct nfs3_createdata *data)
> if (data == NULL)
> goto out;
>
> + pacl = get_acl(dir, ACL_TYPE_DEFAULT);
> + if (!pacl || pacl == ERR_PTR(-EOPNOTSUPP))
> + sattr->ia_mode &= ~current_umask();
> +
> data->msg.rpc_proc = &nfs3_procedures[NFS3PROC_CREATE];
> data->arg.create.fh = NFS_FH(dir);
> data->arg.create.name = dentry->d_name.name;
> @@ -363,10 +367,6 @@ static void nfs3_free_createdata(struct nfs3_createdata *data)
> data->arg.create.verifier[1] = cpu_to_be32(current->pid);
> }
>
> - status = posix_acl_create(dir, &sattr->ia_mode, &default_acl, &acl);
> - if (status)
> - goto out;
> -
> for (;;) {
> d_alias = nfs3_do_create(dir, dentry, data);
> status = PTR_ERR_OR_ZERO(d_alias);
> @@ -416,14 +416,10 @@ static void nfs3_free_createdata(struct nfs3_createdata *data)
> if (status != 0)
> goto out_dput;
> }
> -
> - status = nfs3_proc_setacls(d_inode(dentry), acl, default_acl);
> -
> out_dput:
> dput(d_alias);
> out_release_acls:
> - posix_acl_release(acl);
> - posix_acl_release(default_acl);
> + posix_acl_release(pacl);
> out:
> nfs3_free_createdata(data);
> dprintk("NFS reply create: %d\n", status);
> @@ -582,7 +578,7 @@ static void nfs3_proc_rename_rpc_prepare(struct rpc_task *task, struct nfs_renam
> static int
> nfs3_proc_mkdir(struct inode *dir, struct dentry *dentry, struct iattr *sattr)
> {
> - struct posix_acl *default_acl, *acl;
> + struct posix_acl *pacl;
> struct nfs3_createdata *data;
> struct dentry *d_alias;
> int status = -ENOMEM;
> @@ -593,10 +589,9 @@ static void nfs3_proc_rename_rpc_prepare(struct rpc_task *task, struct nfs_renam
> if (data == NULL)
> goto out;
>
> - status = posix_acl_create(dir, &sattr->ia_mode, &default_acl, &acl);
> - if (status)
> - goto out;
> -
> + pacl = get_acl(dir, ACL_TYPE_DEFAULT);
> + if (!pacl || pacl == ERR_PTR(-EOPNOTSUPP))
> + sattr->ia_mode &= ~current_umask();
> data->msg.rpc_proc = &nfs3_procedures[NFS3PROC_MKDIR];
> data->arg.mkdir.fh = NFS_FH(dir);
> data->arg.mkdir.name = dentry->d_name.name;
> @@ -612,12 +607,9 @@ static void nfs3_proc_rename_rpc_prepare(struct rpc_task *task, struct nfs_renam
> if (d_alias)
> dentry = d_alias;
>
> - status = nfs3_proc_setacls(d_inode(dentry), acl, default_acl);
> -
> dput(d_alias);
> out_release_acls:
> - posix_acl_release(acl);
> - posix_acl_release(default_acl);
> + posix_acl_release(pacl);
> out:
> nfs3_free_createdata(data);
> dprintk("NFS reply mkdir: %d\n", status);
> @@ -713,7 +705,7 @@ static int nfs3_proc_readdir(struct nfs_readdir_arg *nr_arg,
> nfs3_proc_mknod(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
> dev_t rdev)
> {
> - struct posix_acl *default_acl, *acl;
> + struct posix_acl *pacl;
> struct nfs3_createdata *data;
> struct dentry *d_alias;
> int status = -ENOMEM;
> @@ -725,9 +717,9 @@ static int nfs3_proc_readdir(struct nfs_readdir_arg *nr_arg,
> if (data == NULL)
> goto out;
>
> - status = posix_acl_create(dir, &sattr->ia_mode, &default_acl, &acl);
> - if (status)
> - goto out;
> + pacl = get_acl(dir, ACL_TYPE_DEFAULT);
> + if (!pacl || pacl == ERR_PTR(-EOPNOTSUPP))
> + sattr->ia_mode &= ~current_umask();
>
> data->msg.rpc_proc = &nfs3_procedures[NFS3PROC_MKNOD];
> data->arg.mknod.fh = NFS_FH(dir);
> @@ -762,12 +754,9 @@ static int nfs3_proc_readdir(struct nfs_readdir_arg *nr_arg,
> if (d_alias)
> dentry = d_alias;
>
> - status = nfs3_proc_setacls(d_inode(dentry), acl, default_acl);
> -
> dput(d_alias);
> out_release_acls:
> - posix_acl_release(acl);
> - posix_acl_release(default_acl);
> + posix_acl_release(pacl);
> out:
> nfs3_free_createdata(data);
> dprintk("NFS reply mknod: %d\n", status);
> --
> 1.8.3.1
Powered by blists - more mailing lists