In cases where group_info is cached (e.g. sunrpc), multiplpe
threads may call set_groups with a freshly created group_info
cache (e.g. nfsd), and attempt to sort them simultaneously,
which configures a race condition that can overwrite some
groups in the cache and lead to errors. In the case of nfsd,
the client was receiving EPERM if the group used to provide
authorization was overwritten by this race condition.
In an email exchange with bfields, we agreed that it seems
unintuitive that the groups are sorted on set_groups, and that
it would be better to move the responsibility of sorting to
the caller of set_groups.
These patches:
- Export groups_sort in include/linux/cred.h
- Add a call to groups_sort after the groups are inserted in
group_info
- Remove the call to sort_groups from set_groups
Thiago Rafael Becker (3):
kernel: make groups_sort globally visible
kernel: Move groups_sort to the caller of set_groups.
kernel: set_groups doesn't call groups_sort anymore.
include/linux/cred.h | 1 +
kernel/groups.c | 6 ++++--
kernel/uid16.c | 1 +
net/sunrpc/svcauth_unix.c | 7 +++++++
4 files changed, 13 insertions(+), 2 deletions(-)
--
2.9.5
When the group_info is cached (e.g. sunrpc) there's the possibility
that threads calling set_groups will attempt to do so simultaneously.
Moving the responsibility of sorting to the caller of set_groups, or
in the case of nfsd, to the point where it is received from rpc.mountd
avoids this issue.
Moving it to the caller has the added benifit of a slight improvement on
the nfsd performance.
Signed-off-by: Thiago Rafael Becker <[email protected]>
---
kernel/groups.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/kernel/groups.c b/kernel/groups.c
index 17073a9..8620ad3 100644
--- a/kernel/groups.c
+++ b/kernel/groups.c
@@ -124,7 +124,6 @@ int groups_search(const struct group_info *group_info, kgid_t grp)
void set_groups(struct cred *new, struct group_info *group_info)
{
put_group_info(new->group_info);
- groups_sort(group_info);
get_group_info(group_info);
new->group_info = group_info;
}
--
2.9.5
The responsibility for calling groups_sort is now on the caller
of set_groups.
Signed-off-by: Thiago Rafael Becker <[email protected]>
---
fs/nfsd/auth.c | 3 +++
kernel/groups.c | 1 +
kernel/uid16.c | 1 +
net/sunrpc/auth_gss/gss_rpc_xdr.c | 1 +
net/sunrpc/auth_gss/svcauth_gss.c | 1 +
net/sunrpc/svcauth_unix.c | 8 ++++++++
6 files changed, 15 insertions(+)
diff --git a/fs/nfsd/auth.c b/fs/nfsd/auth.c
index 697f8ae..7b5099b 100644
--- a/fs/nfsd/auth.c
+++ b/fs/nfsd/auth.c
@@ -60,6 +60,9 @@ int nfsd_setuser(struct svc_rqst *rqstp, struct svc_export *exp)
gi->gid[i] = exp->ex_anon_gid;
else
gi->gid[i] = rqgi->gid[i];
+
+ /* Should be race free as long as each thread allocates a new gi */
+ groups_sort(gi);
}
} else {
gi = get_group_info(rqgi);
diff --git a/kernel/groups.c b/kernel/groups.c
index 4c9c9ed..17073a9 100644
--- a/kernel/groups.c
+++ b/kernel/groups.c
@@ -208,6 +208,7 @@ SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist)
return retval;
}
+ groups_sort(group_info);
retval = set_current_groups(group_info);
put_group_info(group_info);
diff --git a/kernel/uid16.c b/kernel/uid16.c
index ce74a49..ef1da2a 100644
--- a/kernel/uid16.c
+++ b/kernel/uid16.c
@@ -192,6 +192,7 @@ SYSCALL_DEFINE2(setgroups16, int, gidsetsize, old_gid_t __user *, grouplist)
return retval;
}
+ groups_sort(group_info);
retval = set_current_groups(group_info);
put_group_info(group_info);
diff --git a/net/sunrpc/auth_gss/gss_rpc_xdr.c b/net/sunrpc/auth_gss/gss_rpc_xdr.c
index c4778ca..444380f 100644
--- a/net/sunrpc/auth_gss/gss_rpc_xdr.c
+++ b/net/sunrpc/auth_gss/gss_rpc_xdr.c
@@ -231,6 +231,7 @@ static int gssx_dec_linux_creds(struct xdr_stream *xdr,
goto out_free_groups;
creds->cr_group_info->gid[i] = kgid;
}
+ groups_sort(creds->cr_group_info);
return 0;
out_free_groups:
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 5dd4e6c..2653119 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -481,6 +481,7 @@ static int rsc_parse(struct cache_detail *cd,
goto out;
rsci.cred.cr_group_info->gid[i] = kgid;
}
+ groups_sort(rsci.cred.cr_group_info);
/* mech name */
len = qword_get(&mesg, buf, mlen);
diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
index 740b67d..99841e1 100644
--- a/net/sunrpc/svcauth_unix.c
+++ b/net/sunrpc/svcauth_unix.c
@@ -20,6 +20,7 @@
#include "netns.h"
+void groups_sort(struct group_info *group_info);
/*
* AUTHUNIX and AUTHNULL credentials are both handled here.
@@ -520,6 +521,12 @@ static int unix_gid_parse(struct cache_detail *cd,
ug.gi->gid[i] = kgid;
}
+ /* Sort the groups before inserting this entry
+ * into the cache to avoid future corrutpions
+ * by multiple simultaneous attempts to sort this
+ * entry.
+ */
+ groups_sort(ug.gi);
ugp = unix_gid_lookup(cd, uid);
if (ugp) {
struct cache_head *ch;
@@ -819,6 +826,7 @@ svcauth_unix_accept(struct svc_rqst *rqstp, __be32 *authp)
kgid_t kgid = make_kgid(&init_user_ns, svc_getnl(argv));
cred->cr_group_info->gid[i] = kgid;
}
+ groups_sort(cred->cr_group_info);
if (svc_getu32(argv) != htonl(RPC_AUTH_NULL) || svc_getu32(argv) != 0) {
*authp = rpc_autherr_badverf;
return SVC_DENIED;
--
2.9.5
In preparation to move group_info sorting to the caller,
make group_sort globally visible.
Signed-off-by: Thiago Rafael Becker <[email protected]>
---
include/linux/cred.h | 1 +
kernel/groups.c | 4 +++-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/linux/cred.h b/include/linux/cred.h
index 099058e..6312865 100644
--- a/include/linux/cred.h
+++ b/include/linux/cred.h
@@ -83,6 +83,7 @@ extern int set_current_groups(struct group_info *);
extern void set_groups(struct cred *, struct group_info *);
extern int groups_search(const struct group_info *, kgid_t);
extern bool may_setgroups(void);
+extern void groups_sort(struct group_info *);
/*
* The security context of a task
diff --git a/kernel/groups.c b/kernel/groups.c
index e357bc8..4c9c9ed 100644
--- a/kernel/groups.c
+++ b/kernel/groups.c
@@ -86,11 +86,13 @@ static int gid_cmp(const void *_a, const void *_b)
return gid_gt(a, b) - gid_lt(a, b);
}
-static void groups_sort(struct group_info *group_info)
+void groups_sort(struct group_info *group_info)
{
sort(group_info->gid, group_info->ngroups, sizeof(*group_info->gid),
gid_cmp, NULL);
}
+EXPORT_SYMBOL(groups_sort);
+
/* a simple bsearch */
int groups_search(const struct group_info *group_info, kgid_t grp)
--
2.9.5
The responsibility for calling groups_sort is now on the caller
of set_groups.
Signed-off-by: Thiago Rafael Becker <[email protected]>
---
arch/s390/kernel/compat_linux.c | 1 +
fs/nfsd/auth.c | 3 +++
kernel/groups.c | 1 +
kernel/uid16.c | 1 +
net/sunrpc/auth_gss/gss_rpc_xdr.c | 1 +
net/sunrpc/auth_gss/svcauth_gss.c | 1 +
net/sunrpc/svcauth_unix.c | 8 ++++++++
7 files changed, 16 insertions(+)
diff --git a/arch/s390/kernel/compat_linux.c b/arch/s390/kernel/compat_linux.c
index f04db37..59eea9c 100644
--- a/arch/s390/kernel/compat_linux.c
+++ b/arch/s390/kernel/compat_linux.c
@@ -263,6 +263,7 @@ COMPAT_SYSCALL_DEFINE2(s390_setgroups16, int, gidsetsize, u16 __user *, grouplis
return retval;
}
+ groups_sort(group_info);
retval = set_current_groups(group_info);
put_group_info(group_info);
diff --git a/fs/nfsd/auth.c b/fs/nfsd/auth.c
index 697f8ae..7b5099b 100644
--- a/fs/nfsd/auth.c
+++ b/fs/nfsd/auth.c
@@ -60,6 +60,9 @@ int nfsd_setuser(struct svc_rqst *rqstp, struct svc_export *exp)
gi->gid[i] = exp->ex_anon_gid;
else
gi->gid[i] = rqgi->gid[i];
+
+ /* Should be race free as long as each thread allocates a new gi */
+ groups_sort(gi);
}
} else {
gi = get_group_info(rqgi);
diff --git a/kernel/groups.c b/kernel/groups.c
index 4c9c9ed..17073a9 100644
--- a/kernel/groups.c
+++ b/kernel/groups.c
@@ -208,6 +208,7 @@ SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist)
return retval;
}
+ groups_sort(group_info);
retval = set_current_groups(group_info);
put_group_info(group_info);
diff --git a/kernel/uid16.c b/kernel/uid16.c
index ce74a49..ef1da2a 100644
--- a/kernel/uid16.c
+++ b/kernel/uid16.c
@@ -192,6 +192,7 @@ SYSCALL_DEFINE2(setgroups16, int, gidsetsize, old_gid_t __user *, grouplist)
return retval;
}
+ groups_sort(group_info);
retval = set_current_groups(group_info);
put_group_info(group_info);
diff --git a/net/sunrpc/auth_gss/gss_rpc_xdr.c b/net/sunrpc/auth_gss/gss_rpc_xdr.c
index c4778ca..444380f 100644
--- a/net/sunrpc/auth_gss/gss_rpc_xdr.c
+++ b/net/sunrpc/auth_gss/gss_rpc_xdr.c
@@ -231,6 +231,7 @@ static int gssx_dec_linux_creds(struct xdr_stream *xdr,
goto out_free_groups;
creds->cr_group_info->gid[i] = kgid;
}
+ groups_sort(creds->cr_group_info);
return 0;
out_free_groups:
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 5dd4e6c..2653119 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -481,6 +481,7 @@ static int rsc_parse(struct cache_detail *cd,
goto out;
rsci.cred.cr_group_info->gid[i] = kgid;
}
+ groups_sort(rsci.cred.cr_group_info);
/* mech name */
len = qword_get(&mesg, buf, mlen);
diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
index 740b67d..99841e1 100644
--- a/net/sunrpc/svcauth_unix.c
+++ b/net/sunrpc/svcauth_unix.c
@@ -20,6 +20,7 @@
#include "netns.h"
+void groups_sort(struct group_info *group_info);
/*
* AUTHUNIX and AUTHNULL credentials are both handled here.
@@ -520,6 +521,12 @@ static int unix_gid_parse(struct cache_detail *cd,
ug.gi->gid[i] = kgid;
}
+ /* Sort the groups before inserting this entry
+ * into the cache to avoid future corrutpions
+ * by multiple simultaneous attempts to sort this
+ * entry.
+ */
+ groups_sort(ug.gi);
ugp = unix_gid_lookup(cd, uid);
if (ugp) {
struct cache_head *ch;
@@ -819,6 +826,7 @@ svcauth_unix_accept(struct svc_rqst *rqstp, __be32 *authp)
kgid_t kgid = make_kgid(&init_user_ns, svc_getnl(argv));
cred->cr_group_info->gid[i] = kgid;
}
+ groups_sort(cred->cr_group_info);
if (svc_getu32(argv) != htonl(RPC_AUTH_NULL) || svc_getu32(argv) != 0) {
*authp = rpc_autherr_badverf;
return SVC_DENIED;
--
2.9.5
It seems I missed another one in arch/s390, uploaded an update.
On Tue, 5 Dec 2017, Thiago Rafael Becker wrote:
> The responsibility for calling groups_sort is now on the caller
> of set_groups.
On Tue, Dec 05 2017, Thiago Rafael Becker wrote:
> The responsibility for calling groups_sort is now on the caller
> of set_groups.
>
> Signed-off-by: Thiago Rafael Becker <[email protected]>
> ---
> arch/s390/kernel/compat_linux.c | 1 +
> fs/nfsd/auth.c | 3 +++
> kernel/groups.c | 1 +
> kernel/uid16.c | 1 +
> net/sunrpc/auth_gss/gss_rpc_xdr.c | 1 +
> net/sunrpc/auth_gss/svcauth_gss.c | 1 +
> net/sunrpc/svcauth_unix.c | 8 ++++++++
> 7 files changed, 16 insertions(+)
>
> diff --git a/arch/s390/kernel/compat_linux.c b/arch/s390/kernel/compat_linux.c
> index f04db37..59eea9c 100644
> --- a/arch/s390/kernel/compat_linux.c
> +++ b/arch/s390/kernel/compat_linux.c
> @@ -263,6 +263,7 @@ COMPAT_SYSCALL_DEFINE2(s390_setgroups16, int, gidsetsize, u16 __user *, grouplis
> return retval;
> }
>
> + groups_sort(group_info);
> retval = set_current_groups(group_info);
> put_group_info(group_info);
>
> diff --git a/fs/nfsd/auth.c b/fs/nfsd/auth.c
> index 697f8ae..7b5099b 100644
> --- a/fs/nfsd/auth.c
> +++ b/fs/nfsd/auth.c
> @@ -60,6 +60,9 @@ int nfsd_setuser(struct svc_rqst *rqstp, struct svc_export *exp)
> gi->gid[i] = exp->ex_anon_gid;
> else
> gi->gid[i] = rqgi->gid[i];
> +
> + /* Should be race free as long as each thread allocates a new gi */
> + groups_sort(gi);
> }
> } else {
> gi = get_group_info(rqgi);
> diff --git a/kernel/groups.c b/kernel/groups.c
> index 4c9c9ed..17073a9 100644
> --- a/kernel/groups.c
> +++ b/kernel/groups.c
> @@ -208,6 +208,7 @@ SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist)
> return retval;
> }
>
> + groups_sort(group_info);
> retval = set_current_groups(group_info);
> put_group_info(group_info);
>
> diff --git a/kernel/uid16.c b/kernel/uid16.c
> index ce74a49..ef1da2a 100644
> --- a/kernel/uid16.c
> +++ b/kernel/uid16.c
> @@ -192,6 +192,7 @@ SYSCALL_DEFINE2(setgroups16, int, gidsetsize, old_gid_t __user *, grouplist)
> return retval;
> }
>
> + groups_sort(group_info);
> retval = set_current_groups(group_info);
> put_group_info(group_info);
>
> diff --git a/net/sunrpc/auth_gss/gss_rpc_xdr.c b/net/sunrpc/auth_gss/gss_rpc_xdr.c
> index c4778ca..444380f 100644
> --- a/net/sunrpc/auth_gss/gss_rpc_xdr.c
> +++ b/net/sunrpc/auth_gss/gss_rpc_xdr.c
> @@ -231,6 +231,7 @@ static int gssx_dec_linux_creds(struct xdr_stream *xdr,
> goto out_free_groups;
> creds->cr_group_info->gid[i] = kgid;
> }
> + groups_sort(creds->cr_group_info);
>
> return 0;
> out_free_groups:
> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> index 5dd4e6c..2653119 100644
> --- a/net/sunrpc/auth_gss/svcauth_gss.c
> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> @@ -481,6 +481,7 @@ static int rsc_parse(struct cache_detail *cd,
> goto out;
> rsci.cred.cr_group_info->gid[i] = kgid;
> }
> + groups_sort(rsci.cred.cr_group_info);
>
> /* mech name */
> len = qword_get(&mesg, buf, mlen);
> diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
> index 740b67d..99841e1 100644
> --- a/net/sunrpc/svcauth_unix.c
> +++ b/net/sunrpc/svcauth_unix.c
> @@ -20,6 +20,7 @@
>
>
> #include "netns.h"
> +void groups_sort(struct group_info *group_info);
>
> /*
> * AUTHUNIX and AUTHNULL credentials are both handled here.
> @@ -520,6 +521,12 @@ static int unix_gid_parse(struct cache_detail *cd,
> ug.gi->gid[i] = kgid;
> }
>
> + /* Sort the groups before inserting this entry
> + * into the cache to avoid future corrutpions
> + * by multiple simultaneous attempts to sort this
> + * entry.
> + */
> + groups_sort(ug.gi);
> ugp = unix_gid_lookup(cd, uid);
> if (ugp) {
> struct cache_head *ch;
> @@ -819,6 +826,7 @@ svcauth_unix_accept(struct svc_rqst *rqstp, __be32 *authp)
> kgid_t kgid = make_kgid(&init_user_ns, svc_getnl(argv));
> cred->cr_group_info->gid[i] = kgid;
> }
> + groups_sort(cred->cr_group_info);
> if (svc_getu32(argv) != htonl(RPC_AUTH_NULL) || svc_getu32(argv) != 0) {
> *authp = rpc_autherr_badverf;
> return SVC_DENIED;
> --
> 2.9.5
Reviewed-by: NeilBrown <[email protected]>
Looks good to me, thanks.
NeilBrown
ACK to these patches from me. I'm not sure who should pick them up....
--b.
On Tue, Dec 05, 2017 at 12:05:09PM -0200, Thiago Rafael Becker wrote:
> In cases where group_info is cached (e.g. sunrpc), multiplpe
> threads may call set_groups with a freshly created group_info
> cache (e.g. nfsd), and attempt to sort them simultaneously,
> which configures a race condition that can overwrite some
> groups in the cache and lead to errors. In the case of nfsd,
> the client was receiving EPERM if the group used to provide
> authorization was overwritten by this race condition.
>
> In an email exchange with bfields, we agreed that it seems
> unintuitive that the groups are sorted on set_groups, and that
> it would be better to move the responsibility of sorting to
> the caller of set_groups.
>
> These patches:
> - Export groups_sort in include/linux/cred.h
> - Add a call to groups_sort after the groups are inserted in
> group_info
> - Remove the call to sort_groups from set_groups
>
> Thiago Rafael Becker (3):
> kernel: make groups_sort globally visible
> kernel: Move groups_sort to the caller of set_groups.
> kernel: set_groups doesn't call groups_sort anymore.
>
> include/linux/cred.h | 1 +
> kernel/groups.c | 6 ++++--
> kernel/uid16.c | 1 +
> net/sunrpc/svcauth_unix.c | 7 +++++++
> 4 files changed, 13 insertions(+), 2 deletions(-)
>
> --
> 2.9.5
In testing, we found that nfsd threads may call set_groups in parallel for
the same entry cached in auth.unix.gid, racing in the call of groups_sort,
corrupting the groups for that entry and leading to permission denials for
the client.
This patch:
- Make groups_sort globally visible.
- Move the call to groups_sort to the modifiers of group_info
- Remove the call to groups_sort from set_groups
Signed-off-by: Thiago Rafael Becker <[email protected]>
---
arch/s390/kernel/compat_linux.c | 1 +
fs/nfsd/auth.c | 3 +++
include/linux/cred.h | 1 +
kernel/groups.c | 6 ++++--
kernel/uid16.c | 1 +
net/sunrpc/auth_gss/gss_rpc_xdr.c | 1 +
net/sunrpc/auth_gss/svcauth_gss.c | 1 +
net/sunrpc/svcauth_unix.c | 7 +++++++
8 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/arch/s390/kernel/compat_linux.c b/arch/s390/kernel/compat_linux.c
index f04db37..59eea9c 100644
--- a/arch/s390/kernel/compat_linux.c
+++ b/arch/s390/kernel/compat_linux.c
@@ -263,6 +263,7 @@ COMPAT_SYSCALL_DEFINE2(s390_setgroups16, int, gidsetsize, u16 __user *, grouplis
return retval;
}
+ groups_sort(group_info);
retval = set_current_groups(group_info);
put_group_info(group_info);
diff --git a/fs/nfsd/auth.c b/fs/nfsd/auth.c
index 697f8ae..7b5099b 100644
--- a/fs/nfsd/auth.c
+++ b/fs/nfsd/auth.c
@@ -60,6 +60,9 @@ int nfsd_setuser(struct svc_rqst *rqstp, struct svc_export *exp)
gi->gid[i] = exp->ex_anon_gid;
else
gi->gid[i] = rqgi->gid[i];
+
+ /* Should be race free as long as each thread allocates a new gi */
+ groups_sort(gi);
}
} else {
gi = get_group_info(rqgi);
diff --git a/include/linux/cred.h b/include/linux/cred.h
index 099058e..6312865 100644
--- a/include/linux/cred.h
+++ b/include/linux/cred.h
@@ -83,6 +83,7 @@ extern int set_current_groups(struct group_info *);
extern void set_groups(struct cred *, struct group_info *);
extern int groups_search(const struct group_info *, kgid_t);
extern bool may_setgroups(void);
+extern void groups_sort(struct group_info *);
/*
* The security context of a task
diff --git a/kernel/groups.c b/kernel/groups.c
index e357bc8..8620ad3 100644
--- a/kernel/groups.c
+++ b/kernel/groups.c
@@ -86,11 +86,13 @@ static int gid_cmp(const void *_a, const void *_b)
return gid_gt(a, b) - gid_lt(a, b);
}
-static void groups_sort(struct group_info *group_info)
+void groups_sort(struct group_info *group_info)
{
sort(group_info->gid, group_info->ngroups, sizeof(*group_info->gid),
gid_cmp, NULL);
}
+EXPORT_SYMBOL(groups_sort);
+
/* a simple bsearch */
int groups_search(const struct group_info *group_info, kgid_t grp)
@@ -122,7 +124,6 @@ int groups_search(const struct group_info *group_info, kgid_t grp)
void set_groups(struct cred *new, struct group_info *group_info)
{
put_group_info(new->group_info);
- groups_sort(group_info);
get_group_info(group_info);
new->group_info = group_info;
}
@@ -206,6 +207,7 @@ SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist)
return retval;
}
+ groups_sort(group_info);
retval = set_current_groups(group_info);
put_group_info(group_info);
diff --git a/kernel/uid16.c b/kernel/uid16.c
index ce74a49..ef1da2a 100644
--- a/kernel/uid16.c
+++ b/kernel/uid16.c
@@ -192,6 +192,7 @@ SYSCALL_DEFINE2(setgroups16, int, gidsetsize, old_gid_t __user *, grouplist)
return retval;
}
+ groups_sort(group_info);
retval = set_current_groups(group_info);
put_group_info(group_info);
diff --git a/net/sunrpc/auth_gss/gss_rpc_xdr.c b/net/sunrpc/auth_gss/gss_rpc_xdr.c
index c4778ca..444380f 100644
--- a/net/sunrpc/auth_gss/gss_rpc_xdr.c
+++ b/net/sunrpc/auth_gss/gss_rpc_xdr.c
@@ -231,6 +231,7 @@ static int gssx_dec_linux_creds(struct xdr_stream *xdr,
goto out_free_groups;
creds->cr_group_info->gid[i] = kgid;
}
+ groups_sort(creds->cr_group_info);
return 0;
out_free_groups:
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 5dd4e6c..2653119 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -481,6 +481,7 @@ static int rsc_parse(struct cache_detail *cd,
goto out;
rsci.cred.cr_group_info->gid[i] = kgid;
}
+ groups_sort(rsci.cred.cr_group_info);
/* mech name */
len = qword_get(&mesg, buf, mlen);
diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
index 740b67d..7154dab 100644
--- a/net/sunrpc/svcauth_unix.c
+++ b/net/sunrpc/svcauth_unix.c
@@ -520,6 +520,12 @@ static int unix_gid_parse(struct cache_detail *cd,
ug.gi->gid[i] = kgid;
}
+ /* Sort the groups before inserting this entry
+ * into the cache to avoid future corrutpions
+ * by multiple simultaneous attempts to sort this
+ * entry.
+ */
+ groups_sort(ug.gi);
ugp = unix_gid_lookup(cd, uid);
if (ugp) {
struct cache_head *ch;
@@ -819,6 +825,7 @@ svcauth_unix_accept(struct svc_rqst *rqstp, __be32 *authp)
kgid_t kgid = make_kgid(&init_user_ns, svc_getnl(argv));
cred->cr_group_info->gid[i] = kgid;
}
+ groups_sort(cred->cr_group_info);
if (svc_getu32(argv) != htonl(RPC_AUTH_NULL) || svc_getu32(argv) != 0) {
*authp = rpc_autherr_badverf;
return SVC_DENIED;
--
2.9.5
On Mon, Dec 11, 2017 at 11:28:06AM -0200, Thiago Rafael Becker wrote:
> +++ b/fs/nfsd/auth.c
> @@ -60,6 +60,9 @@ int nfsd_setuser(struct svc_rqst *rqstp, struct svc_export *exp)
> gi->gid[i] = exp->ex_anon_gid;
> else
> gi->gid[i] = rqgi->gid[i];
> +
> + /* Should be race free as long as each thread allocates a new gi */
> + groups_sort(gi);
> }
Overlong line. Would recommend:
/* Each thread has its own gi, so no race */
> +++ b/kernel/groups.c
> @@ -86,11 +86,13 @@ static int gid_cmp(const void *_a, const void *_b)
> return gid_gt(a, b) - gid_lt(a, b);
> }
>
> -static void groups_sort(struct group_info *group_info)
> +void groups_sort(struct group_info *group_info)
> {
> sort(group_info->gid, group_info->ngroups, sizeof(*group_info->gid),
> gid_cmp, NULL);
> }
> +EXPORT_SYMBOL(groups_sort);
> +
>
Spurious extra line
> diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
> index 740b67d..7154dab 100644
> --- a/net/sunrpc/svcauth_unix.c
> +++ b/net/sunrpc/svcauth_unix.c
> @@ -520,6 +520,12 @@ static int unix_gid_parse(struct cache_detail *cd,
> ug.gi->gid[i] = kgid;
> }
>
> + /* Sort the groups before inserting this entry
> + * into the cache to avoid future corrutpions
> + * by multiple simultaneous attempts to sort this
> + * entry.
> + */
> + groups_sort(ug.gi);
> ugp = unix_gid_lookup(cd, uid);
> if (ugp) {
> struct cache_head *ch;
Why comment this call and not the other ones? I appreciate this is the
call-site where you discovered the bug, but that's not going to make it
special to someone who's reading this code in ten years time. I would
leave this comment out entirely; it's just the new way we do things.
I can't find anything else to critique; nice job.
In testing, we found that nfsd threads may call set_groups in parallel for
the same entry cached in auth.unix.gid, racing in the call of groups_sort,
corrupting the groups for that entry and leading to permission denials for
the client.
This patch:
- Make groups_sort globally visible.
- Move the call to groups_sort to the modifiers of group_info
- Remove the call to groups_sort from set_groups
Signed-off-by: Thiago Rafael Becker <[email protected]>
---
arch/s390/kernel/compat_linux.c | 1 +
fs/nfsd/auth.c | 3 +++
include/linux/cred.h | 1 +
kernel/groups.c | 5 +++--
kernel/uid16.c | 1 +
net/sunrpc/auth_gss/gss_rpc_xdr.c | 1 +
net/sunrpc/auth_gss/svcauth_gss.c | 1 +
net/sunrpc/svcauth_unix.c | 2 ++
8 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/arch/s390/kernel/compat_linux.c b/arch/s390/kernel/compat_linux.c
index f04db37..59eea9c 100644
--- a/arch/s390/kernel/compat_linux.c
+++ b/arch/s390/kernel/compat_linux.c
@@ -263,6 +263,7 @@ COMPAT_SYSCALL_DEFINE2(s390_setgroups16, int, gidsetsize, u16 __user *, grouplis
return retval;
}
+ groups_sort(group_info);
retval = set_current_groups(group_info);
put_group_info(group_info);
diff --git a/fs/nfsd/auth.c b/fs/nfsd/auth.c
index 697f8ae..f650e47 100644
--- a/fs/nfsd/auth.c
+++ b/fs/nfsd/auth.c
@@ -60,6 +60,9 @@ int nfsd_setuser(struct svc_rqst *rqstp, struct svc_export *exp)
gi->gid[i] = exp->ex_anon_gid;
else
gi->gid[i] = rqgi->gid[i];
+
+ /* Each thread allocates its own gi, no race */
+ groups_sort(gi);
}
} else {
gi = get_group_info(rqgi);
diff --git a/include/linux/cred.h b/include/linux/cred.h
index 099058e..6312865 100644
--- a/include/linux/cred.h
+++ b/include/linux/cred.h
@@ -83,6 +83,7 @@ extern int set_current_groups(struct group_info *);
extern void set_groups(struct cred *, struct group_info *);
extern int groups_search(const struct group_info *, kgid_t);
extern bool may_setgroups(void);
+extern void groups_sort(struct group_info *);
/*
* The security context of a task
diff --git a/kernel/groups.c b/kernel/groups.c
index e357bc8..daae2f2 100644
--- a/kernel/groups.c
+++ b/kernel/groups.c
@@ -86,11 +86,12 @@ static int gid_cmp(const void *_a, const void *_b)
return gid_gt(a, b) - gid_lt(a, b);
}
-static void groups_sort(struct group_info *group_info)
+void groups_sort(struct group_info *group_info)
{
sort(group_info->gid, group_info->ngroups, sizeof(*group_info->gid),
gid_cmp, NULL);
}
+EXPORT_SYMBOL(groups_sort);
/* a simple bsearch */
int groups_search(const struct group_info *group_info, kgid_t grp)
@@ -122,7 +123,6 @@ int groups_search(const struct group_info *group_info, kgid_t grp)
void set_groups(struct cred *new, struct group_info *group_info)
{
put_group_info(new->group_info);
- groups_sort(group_info);
get_group_info(group_info);
new->group_info = group_info;
}
@@ -206,6 +206,7 @@ SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist)
return retval;
}
+ groups_sort(group_info);
retval = set_current_groups(group_info);
put_group_info(group_info);
diff --git a/kernel/uid16.c b/kernel/uid16.c
index ce74a49..ef1da2a 100644
--- a/kernel/uid16.c
+++ b/kernel/uid16.c
@@ -192,6 +192,7 @@ SYSCALL_DEFINE2(setgroups16, int, gidsetsize, old_gid_t __user *, grouplist)
return retval;
}
+ groups_sort(group_info);
retval = set_current_groups(group_info);
put_group_info(group_info);
diff --git a/net/sunrpc/auth_gss/gss_rpc_xdr.c b/net/sunrpc/auth_gss/gss_rpc_xdr.c
index c4778ca..444380f 100644
--- a/net/sunrpc/auth_gss/gss_rpc_xdr.c
+++ b/net/sunrpc/auth_gss/gss_rpc_xdr.c
@@ -231,6 +231,7 @@ static int gssx_dec_linux_creds(struct xdr_stream *xdr,
goto out_free_groups;
creds->cr_group_info->gid[i] = kgid;
}
+ groups_sort(creds->cr_group_info);
return 0;
out_free_groups:
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 5dd4e6c..2653119 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -481,6 +481,7 @@ static int rsc_parse(struct cache_detail *cd,
goto out;
rsci.cred.cr_group_info->gid[i] = kgid;
}
+ groups_sort(rsci.cred.cr_group_info);
/* mech name */
len = qword_get(&mesg, buf, mlen);
diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
index 740b67d..af7f28f 100644
--- a/net/sunrpc/svcauth_unix.c
+++ b/net/sunrpc/svcauth_unix.c
@@ -520,6 +520,7 @@ static int unix_gid_parse(struct cache_detail *cd,
ug.gi->gid[i] = kgid;
}
+ groups_sort(ug.gi);
ugp = unix_gid_lookup(cd, uid);
if (ugp) {
struct cache_head *ch;
@@ -819,6 +820,7 @@ svcauth_unix_accept(struct svc_rqst *rqstp, __be32 *authp)
kgid_t kgid = make_kgid(&init_user_ns, svc_getnl(argv));
cred->cr_group_info->gid[i] = kgid;
}
+ groups_sort(cred->cr_group_info);
if (svc_getu32(argv) != htonl(RPC_AUTH_NULL) || svc_getu32(argv) != 0) {
*authp = rpc_autherr_badverf;
return SVC_DENIED;
--
2.9.5
On Mon, Dec 11, 2017 at 01:14:20PM -0200, Thiago Rafael Becker wrote:
> In testing, we found that nfsd threads may call set_groups in parallel for
> the same entry cached in auth.unix.gid, racing in the call of groups_sort,
> corrupting the groups for that entry and leading to permission denials for
> the client.
>
> This patch:
> - Make groups_sort globally visible.
> - Move the call to groups_sort to the modifiers of group_info
> - Remove the call to groups_sort from set_groups
>
> Signed-off-by: Thiago Rafael Becker <[email protected]>
Reviewed-by: Matthew Wilcox <[email protected]>
ACK. (Assuming somebody else takes it--Andrew? Al? Or I can take it
through the nfsd tree. I'm not sure who owns the stuff under kernel/.)
--b.
On Mon, Dec 11, 2017 at 01:14:20PM -0200, Thiago Rafael Becker wrote:
> In testing, we found that nfsd threads may call set_groups in parallel for
> the same entry cached in auth.unix.gid, racing in the call of groups_sort,
> corrupting the groups for that entry and leading to permission denials for
> the client.
>
> This patch:
> - Make groups_sort globally visible.
> - Move the call to groups_sort to the modifiers of group_info
> - Remove the call to groups_sort from set_groups
>
> Signed-off-by: Thiago Rafael Becker <[email protected]>
> ---
> arch/s390/kernel/compat_linux.c | 1 +
> fs/nfsd/auth.c | 3 +++
> include/linux/cred.h | 1 +
> kernel/groups.c | 5 +++--
> kernel/uid16.c | 1 +
> net/sunrpc/auth_gss/gss_rpc_xdr.c | 1 +
> net/sunrpc/auth_gss/svcauth_gss.c | 1 +
> net/sunrpc/svcauth_unix.c | 2 ++
> 8 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/arch/s390/kernel/compat_linux.c b/arch/s390/kernel/compat_linux.c
> index f04db37..59eea9c 100644
> --- a/arch/s390/kernel/compat_linux.c
> +++ b/arch/s390/kernel/compat_linux.c
> @@ -263,6 +263,7 @@ COMPAT_SYSCALL_DEFINE2(s390_setgroups16, int, gidsetsize, u16 __user *, grouplis
> return retval;
> }
>
> + groups_sort(group_info);
> retval = set_current_groups(group_info);
> put_group_info(group_info);
>
> diff --git a/fs/nfsd/auth.c b/fs/nfsd/auth.c
> index 697f8ae..f650e47 100644
> --- a/fs/nfsd/auth.c
> +++ b/fs/nfsd/auth.c
> @@ -60,6 +60,9 @@ int nfsd_setuser(struct svc_rqst *rqstp, struct svc_export *exp)
> gi->gid[i] = exp->ex_anon_gid;
> else
> gi->gid[i] = rqgi->gid[i];
> +
> + /* Each thread allocates its own gi, no race */
> + groups_sort(gi);
> }
> } else {
> gi = get_group_info(rqgi);
> diff --git a/include/linux/cred.h b/include/linux/cred.h
> index 099058e..6312865 100644
> --- a/include/linux/cred.h
> +++ b/include/linux/cred.h
> @@ -83,6 +83,7 @@ extern int set_current_groups(struct group_info *);
> extern void set_groups(struct cred *, struct group_info *);
> extern int groups_search(const struct group_info *, kgid_t);
> extern bool may_setgroups(void);
> +extern void groups_sort(struct group_info *);
>
> /*
> * The security context of a task
> diff --git a/kernel/groups.c b/kernel/groups.c
> index e357bc8..daae2f2 100644
> --- a/kernel/groups.c
> +++ b/kernel/groups.c
> @@ -86,11 +86,12 @@ static int gid_cmp(const void *_a, const void *_b)
> return gid_gt(a, b) - gid_lt(a, b);
> }
>
> -static void groups_sort(struct group_info *group_info)
> +void groups_sort(struct group_info *group_info)
> {
> sort(group_info->gid, group_info->ngroups, sizeof(*group_info->gid),
> gid_cmp, NULL);
> }
> +EXPORT_SYMBOL(groups_sort);
>
> /* a simple bsearch */
> int groups_search(const struct group_info *group_info, kgid_t grp)
> @@ -122,7 +123,6 @@ int groups_search(const struct group_info *group_info, kgid_t grp)
> void set_groups(struct cred *new, struct group_info *group_info)
> {
> put_group_info(new->group_info);
> - groups_sort(group_info);
> get_group_info(group_info);
> new->group_info = group_info;
> }
> @@ -206,6 +206,7 @@ SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist)
> return retval;
> }
>
> + groups_sort(group_info);
> retval = set_current_groups(group_info);
> put_group_info(group_info);
>
> diff --git a/kernel/uid16.c b/kernel/uid16.c
> index ce74a49..ef1da2a 100644
> --- a/kernel/uid16.c
> +++ b/kernel/uid16.c
> @@ -192,6 +192,7 @@ SYSCALL_DEFINE2(setgroups16, int, gidsetsize, old_gid_t __user *, grouplist)
> return retval;
> }
>
> + groups_sort(group_info);
> retval = set_current_groups(group_info);
> put_group_info(group_info);
>
> diff --git a/net/sunrpc/auth_gss/gss_rpc_xdr.c b/net/sunrpc/auth_gss/gss_rpc_xdr.c
> index c4778ca..444380f 100644
> --- a/net/sunrpc/auth_gss/gss_rpc_xdr.c
> +++ b/net/sunrpc/auth_gss/gss_rpc_xdr.c
> @@ -231,6 +231,7 @@ static int gssx_dec_linux_creds(struct xdr_stream *xdr,
> goto out_free_groups;
> creds->cr_group_info->gid[i] = kgid;
> }
> + groups_sort(creds->cr_group_info);
>
> return 0;
> out_free_groups:
> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> index 5dd4e6c..2653119 100644
> --- a/net/sunrpc/auth_gss/svcauth_gss.c
> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> @@ -481,6 +481,7 @@ static int rsc_parse(struct cache_detail *cd,
> goto out;
> rsci.cred.cr_group_info->gid[i] = kgid;
> }
> + groups_sort(rsci.cred.cr_group_info);
>
> /* mech name */
> len = qword_get(&mesg, buf, mlen);
> diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
> index 740b67d..af7f28f 100644
> --- a/net/sunrpc/svcauth_unix.c
> +++ b/net/sunrpc/svcauth_unix.c
> @@ -520,6 +520,7 @@ static int unix_gid_parse(struct cache_detail *cd,
> ug.gi->gid[i] = kgid;
> }
>
> + groups_sort(ug.gi);
> ugp = unix_gid_lookup(cd, uid);
> if (ugp) {
> struct cache_head *ch;
> @@ -819,6 +820,7 @@ svcauth_unix_accept(struct svc_rqst *rqstp, __be32 *authp)
> kgid_t kgid = make_kgid(&init_user_ns, svc_getnl(argv));
> cred->cr_group_info->gid[i] = kgid;
> }
> + groups_sort(cred->cr_group_info);
> if (svc_getu32(argv) != htonl(RPC_AUTH_NULL) || svc_getu32(argv) != 0) {
> *authp = rpc_autherr_badverf;
> return SVC_DENIED;
> --
> 2.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 11 2017, Thiago Rafael Becker wrote:
> In testing, we found that nfsd threads may call set_groups in parallel for
> the same entry cached in auth.unix.gid, racing in the call of groups_sort,
> corrupting the groups for that entry and leading to permission denials for
> the client.
>
> This patch:
> - Make groups_sort globally visible.
> - Move the call to groups_sort to the modifiers of group_info
> - Remove the call to groups_sort from set_groups
>
> Signed-off-by: Thiago Rafael Becker <[email protected]>
Reviewed-by: NeilBrown <[email protected]>
Thanks. I like this better as a single patch.
I'd probably suggest "Cc: [email protected]".
Less important bugfixes have gone to stable...
It might be nice to enhance
Documentation/security/credentials.rst
to explain that the groups list is always sorted, and must be sorted
before set_groups() or set_current_groups() is called, but
as the document doesn't mention any of this at all, this is purely
an extra enhancement and doesn't need to be included in the same patch.
David: do you agree that this sort of content would be appropriate in
that file (which you apparently authored). Would you like to update it,
or shall I?
Thanks,
NeilBrown
> ---
> arch/s390/kernel/compat_linux.c | 1 +
> fs/nfsd/auth.c | 3 +++
> include/linux/cred.h | 1 +
> kernel/groups.c | 5 +++--
> kernel/uid16.c | 1 +
> net/sunrpc/auth_gss/gss_rpc_xdr.c | 1 +
> net/sunrpc/auth_gss/svcauth_gss.c | 1 +
> net/sunrpc/svcauth_unix.c | 2 ++
> 8 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/arch/s390/kernel/compat_linux.c b/arch/s390/kernel/compat_linux.c
> index f04db37..59eea9c 100644
> --- a/arch/s390/kernel/compat_linux.c
> +++ b/arch/s390/kernel/compat_linux.c
> @@ -263,6 +263,7 @@ COMPAT_SYSCALL_DEFINE2(s390_setgroups16, int, gidsetsize, u16 __user *, grouplis
> return retval;
> }
>
> + groups_sort(group_info);
> retval = set_current_groups(group_info);
> put_group_info(group_info);
>
> diff --git a/fs/nfsd/auth.c b/fs/nfsd/auth.c
> index 697f8ae..f650e47 100644
> --- a/fs/nfsd/auth.c
> +++ b/fs/nfsd/auth.c
> @@ -60,6 +60,9 @@ int nfsd_setuser(struct svc_rqst *rqstp, struct svc_export *exp)
> gi->gid[i] = exp->ex_anon_gid;
> else
> gi->gid[i] = rqgi->gid[i];
> +
> + /* Each thread allocates its own gi, no race */
> + groups_sort(gi);
> }
> } else {
> gi = get_group_info(rqgi);
> diff --git a/include/linux/cred.h b/include/linux/cred.h
> index 099058e..6312865 100644
> --- a/include/linux/cred.h
> +++ b/include/linux/cred.h
> @@ -83,6 +83,7 @@ extern int set_current_groups(struct group_info *);
> extern void set_groups(struct cred *, struct group_info *);
> extern int groups_search(const struct group_info *, kgid_t);
> extern bool may_setgroups(void);
> +extern void groups_sort(struct group_info *);
>
> /*
> * The security context of a task
> diff --git a/kernel/groups.c b/kernel/groups.c
> index e357bc8..daae2f2 100644
> --- a/kernel/groups.c
> +++ b/kernel/groups.c
> @@ -86,11 +86,12 @@ static int gid_cmp(const void *_a, const void *_b)
> return gid_gt(a, b) - gid_lt(a, b);
> }
>
> -static void groups_sort(struct group_info *group_info)
> +void groups_sort(struct group_info *group_info)
> {
> sort(group_info->gid, group_info->ngroups, sizeof(*group_info->gid),
> gid_cmp, NULL);
> }
> +EXPORT_SYMBOL(groups_sort);
>
> /* a simple bsearch */
> int groups_search(const struct group_info *group_info, kgid_t grp)
> @@ -122,7 +123,6 @@ int groups_search(const struct group_info *group_info, kgid_t grp)
> void set_groups(struct cred *new, struct group_info *group_info)
> {
> put_group_info(new->group_info);
> - groups_sort(group_info);
> get_group_info(group_info);
> new->group_info = group_info;
> }
> @@ -206,6 +206,7 @@ SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist)
> return retval;
> }
>
> + groups_sort(group_info);
> retval = set_current_groups(group_info);
> put_group_info(group_info);
>
> diff --git a/kernel/uid16.c b/kernel/uid16.c
> index ce74a49..ef1da2a 100644
> --- a/kernel/uid16.c
> +++ b/kernel/uid16.c
> @@ -192,6 +192,7 @@ SYSCALL_DEFINE2(setgroups16, int, gidsetsize, old_gid_t __user *, grouplist)
> return retval;
> }
>
> + groups_sort(group_info);
> retval = set_current_groups(group_info);
> put_group_info(group_info);
>
> diff --git a/net/sunrpc/auth_gss/gss_rpc_xdr.c b/net/sunrpc/auth_gss/gss_rpc_xdr.c
> index c4778ca..444380f 100644
> --- a/net/sunrpc/auth_gss/gss_rpc_xdr.c
> +++ b/net/sunrpc/auth_gss/gss_rpc_xdr.c
> @@ -231,6 +231,7 @@ static int gssx_dec_linux_creds(struct xdr_stream *xdr,
> goto out_free_groups;
> creds->cr_group_info->gid[i] = kgid;
> }
> + groups_sort(creds->cr_group_info);
>
> return 0;
> out_free_groups:
> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> index 5dd4e6c..2653119 100644
> --- a/net/sunrpc/auth_gss/svcauth_gss.c
> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> @@ -481,6 +481,7 @@ static int rsc_parse(struct cache_detail *cd,
> goto out;
> rsci.cred.cr_group_info->gid[i] = kgid;
> }
> + groups_sort(rsci.cred.cr_group_info);
>
> /* mech name */
> len = qword_get(&mesg, buf, mlen);
> diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
> index 740b67d..af7f28f 100644
> --- a/net/sunrpc/svcauth_unix.c
> +++ b/net/sunrpc/svcauth_unix.c
> @@ -520,6 +520,7 @@ static int unix_gid_parse(struct cache_detail *cd,
> ug.gi->gid[i] = kgid;
> }
>
> + groups_sort(ug.gi);
> ugp = unix_gid_lookup(cd, uid);
> if (ugp) {
> struct cache_head *ch;
> @@ -819,6 +820,7 @@ svcauth_unix_accept(struct svc_rqst *rqstp, __be32 *authp)
> kgid_t kgid = make_kgid(&init_user_ns, svc_getnl(argv));
> cred->cr_group_info->gid[i] = kgid;
> }
> + groups_sort(cred->cr_group_info);
> if (svc_getu32(argv) != htonl(RPC_AUTH_NULL) || svc_getu32(argv) != 0) {
> *authp = rpc_autherr_badverf;
> return SVC_DENIED;
> --
> 2.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
NeilBrown <[email protected]> wrote:
> David: do you agree that this sort of content would be appropriate in
> that file (which you apparently authored). Would you like to update it,
> or shall I?
Please update it.
Thanks,
David
This patch updates the documentation with the observations that led
to commit bdcf0a423ea1 ("kernel: make groups_sort calling a
responsibility group_info allocators") and the new behaviour required.
Specifically that groups_sort() should be called on a new group_list
before set_groups() or set_current_groups() is called.
Signed-off-by: NeilBrown <[email protected]>
---
Documentation/security/credentials.rst | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/Documentation/security/credentials.rst b/Documentation/security/credentials.rst
index 66a2e24939d8..5d659e3e52ad 100644
--- a/Documentation/security/credentials.rst
+++ b/Documentation/security/credentials.rst
@@ -451,6 +451,13 @@ checks and hooks done. Both the current and the proposed sets of credentials
are available for this purpose as current_cred() will return the current set
still at this point.
+When replacing the group list, the new list must be sorted before it
+is added to the credential, as a binary search is used to test for
+membership. In practice, this means ``groups_sort()`` should be
+called before ``set_groups()`` or ``set_current_groups()``.
+``groups_sort()`` must not be called on a ``struct group_list`` which
+is shared as it may permute elements as part of the sorting process
+even if the array is already sorted.
When the credential set is ready, it should be committed to the current process
by calling::
--
2.14.0.rc0.dirty
On Wed, Jan 03, 2018 at 08:01:15AM +1100, NeilBrown wrote:
>
> +When replacing the group list, the new list must be sorted before it
> +is added to the credential, as a binary search is used to test for
> +membership. In practice, this means ``groups_sort()`` should be
For a .rst file, shouldn't we be using :c:func:`groups_sort` instead of
``groups_sort()``?
> +called before ``set_groups()`` or ``set_current_groups()``.
> +``groups_sort()`` must not be called on a ``struct group_list`` which
> +is shared as it may permute elements as part of the sorting process
> +even if the array is already sorted.
>
> When the credential set is ready, it should be committed to the current process
> by calling::
> --
> 2.14.0.rc0.dirty
>
On Tue, 2 Jan 2018 13:04:31 -0800
Matthew Wilcox <[email protected]> wrote:
> > +When replacing the group list, the new list must be sorted before it
> > +is added to the credential, as a binary search is used to test for
> > +membership. In practice, this means ``groups_sort()`` should be
>
> For a .rst file, shouldn't we be using :c:func:`groups_sort` instead of
> ``groups_sort()``?
There is value in using the c:func syntax, as it will generate
cross-references to the kerneldoc comments for those functions. In this
case, it would appear that these comments exist, but nobody has pulled
them into the docs yet. I took the liberty of adding :c:func: references
to this patch on its way into docs-next so that things will Just Work on
that happy day when somebody adds the function documentation.
Thanks,
jon
On Sat, Jan 06, 2018 at 11:09:08AM -0700, Jonathan Corbet wrote:
> On Tue, 2 Jan 2018 13:04:31 -0800
> Matthew Wilcox <[email protected]> wrote:
>
> > > +When replacing the group list, the new list must be sorted before it
> > > +is added to the credential, as a binary search is used to test for
> > > +membership. In practice, this means ``groups_sort()`` should be
> >
> > For a .rst file, shouldn't we be using :c:func:`groups_sort` instead of
> > ``groups_sort()``?
>
> There is value in using the c:func syntax, as it will generate
> cross-references to the kerneldoc comments for those functions. In this
> case, it would appear that these comments exist, but nobody has pulled
> them into the docs yet. I took the liberty of adding :c:func: references
> to this patch on its way into docs-next so that things will Just Work on
> that happy day when somebody adds the function documentation.
Thanks for making that substitution.
I've been thinking about all the kernel-doc we have that's completely
unincorporated. I've also been thinking about core-api/kernel-api.rst
which to my mind is completely unreadable in its current form -- look at
https://www.kernel.org/doc/html/latest/core-api/kernel-api.html and you
wouldn't really know there's anything in it beyond the List Management
Functions.
I think the right path forward is to have kernel-api.rst be the dumping
ground for all the files with kernel-doc but nothing more. That gives
us somewhere to link to.
Then we need little stories about how all the functions in a subsystem
fit together. For example, we can create a list.rst which explains how
this is a doubly-linked list that you use by embedding a list_head into
your data structure, and has O(1) insertion/deletion, etc, etc. Then we
would move all the list.h kernel-doc from kernel-api.rst into list.rst.
Is this a reasonable strategy to follow? Does anyone have a better
strategy? I mean ... you've written a book, you presumably have some
idea about how to present the vast amount of information we've accumulated
over the years :-)
On 01/06/18 12:20, Matthew Wilcox wrote:
>
> I've been thinking about all the kernel-doc we have that's completely
> unincorporated. I've also been thinking about core-api/kernel-api.rst
> which to my mind is completely unreadable in its current form -- look at
> https://www.kernel.org/doc/html/latest/core-api/kernel-api.html and you
> wouldn't really know there's anything in it beyond the List Management
> Functions.
The index is on the left side, but would be better (duplicated?) at the beginning
of the chapter. The left side is still useful for navigation, but then it
scrolls away too quickly when the right side text is scrolled.
> I think the right path forward is to have kernel-api.rst be the dumping
> ground for all the files with kernel-doc but nothing more. That gives
> us somewhere to link to.
FWFW, I have recently done firewire.rst, infiniband.rst, and some additions
to scsi.rst. But the new firewire.rst and infiniband.rst could use some
introductory material before just jumping into the API.
> Then we need little stories about how all the functions in a subsystem
> fit together. For example, we can create a list.rst which explains how
> this is a doubly-linked list that you use by embedding a list_head into
> your data structure, and has O(1) insertion/deletion, etc, etc. Then we
> would move all the list.h kernel-doc from kernel-api.rst into list.rst.
--
~Randy
On Sat, Jan 06 2018, Jonathan Corbet wrote:
> There is value in using the c:func syntax, as it will generate
> cross-references to the kerneldoc comments for those functions. In this
> case, it would appear that these comments exist, but nobody has pulled
> them into the docs yet. I took the liberty of adding :c:func: references
> to this patch on its way into docs-next so that things will Just Work on
> that happy day when somebody adds the function documentation.
Is this the sort of thing that might result in that happy day?
I didn't spend tooo much time on it, in case including the
kernel-doc in credentials.rst like this was the wrong direction.
Thanks,
NeilBrown
------------------------------8<-------------------------
From: NeilBrown <[email protected]>
Subject: [PATCH] Documentation: include kernel-doc in credentials.rst
- kernel-doc from include/linux/cred.h, kernel/cred.c, and
kernel/group.c is included in credentials.rst.
- Various function references in credentials.rst are changed
to link to this documentation.
- Various minor improvements to the documentation.
Signed-off-by: NeilBrown <[email protected]>
---
Documentation/security/credentials.rst | 55 ++++++++++++++++++----------------
kernel/groups.c | 19 +++++++++++-
2 files changed, 47 insertions(+), 27 deletions(-)
diff --git a/Documentation/security/credentials.rst b/Documentation/security/credentials.rst
index 66a2e24939d8..fd1b7d3b2a78 100644
--- a/Documentation/security/credentials.rst
+++ b/Documentation/security/credentials.rst
@@ -296,7 +296,7 @@ for example), it must be considered immutable, barring two exceptions:
To catch accidental credential alteration at compile time, struct task_struct
has _const_ pointers to its credential sets, as does struct file. Furthermore,
-certain functions such as ``get_cred()`` and ``put_cred()`` operate on const
+certain functions such as :c:func:`get_cred` and :c:func:`put_cred` operate on const
pointers, thus rendering casts unnecessary, but require to temporarily ditch
the const qualification to be able to alter the reference count.
@@ -391,8 +391,8 @@ This does all the RCU magic inside of it. The caller must call put_cred() on
the credentials so obtained when they're finished with.
.. note::
- The result of ``__task_cred()`` should not be passed directly to
- ``get_cred()`` as this may race with ``commit_cred()``.
+ The result of :c:func:`__task_cred()` should not be passed directly to
+ :c:func:`get_cred` as this may race with :c:func:`commit_cred`.
There are a couple of convenience functions to access bits of another task's
credentials, hiding the RCU magic from the caller::
@@ -406,7 +406,7 @@ If the caller is holding the RCU read lock at the time anyway, then::
__task_cred(task)->euid
should be used instead. Similarly, if multiple aspects of a task's credentials
-need to be accessed, RCU read lock should be used, ``__task_cred()`` called,
+need to be accessed, RCU read lock should be used, :c:func:`__task_cred` called,
the result stored in a temporary pointer and then the credential aspects called
from that before dropping the lock. This prevents the potentially expensive
RCU magic from being invoked multiple times.
@@ -433,11 +433,8 @@ alter those of another task. This means that it doesn't need to use any
locking to alter its own credentials.
To alter the current process's credentials, a function should first prepare a
-new set of credentials by calling::
-
- struct cred *prepare_creds(void);
-
-this locks current->cred_replace_mutex and then allocates and constructs a
+new set of credentials by calling :c:func:`prepare_creds`.
+This locks ``current->cred_replace_mutex`` and then allocates and constructs a
duplicate of the current process's credentials, returning with the mutex still
held if successful. It returns NULL if not successful (out of memory).
@@ -453,10 +450,7 @@ still at this point.
When the credential set is ready, it should be committed to the current process
-by calling::
-
- int commit_creds(struct cred *new);
-
+by calling :c:func:`commit_creds`.
This will alter various aspects of the credentials and the process, giving the
LSM a chance to do likewise, then it will use ``rcu_assign_pointer()`` to
actually commit the new credentials to ``current->cred``, it will release
@@ -467,20 +461,17 @@ This function is guaranteed to return 0, so that it can be tail-called at the
end of such functions as ``sys_setresuid()``.
Note that this function consumes the caller's reference to the new credentials.
-The caller should _not_ call ``put_cred()`` on the new credentials afterwards.
+The caller should _not_ call :c:func:`put_cred` on the new credentials afterwards.
Furthermore, once this function has been called on a new set of credentials,
those credentials may _not_ be changed further.
Should the security checks fail or some other error occur after
-``prepare_creds()`` has been called, then the following function should be
-invoked::
-
- void abort_creds(struct cred *new);
-
+:c:func:`prepare_creds` has been called, then the function
+:c:func:`abort_creds` should be invoked.
This releases the lock on ``current->cred_replace_mutex`` that
-``prepare_creds()`` got and then releases the new credentials.
+:c:func:`prepare_creds` got and then releases the new credentials.
A typical credentials alteration function would look something like this::
@@ -512,19 +503,20 @@ There are some functions to help manage credentials:
- ``void put_cred(const struct cred *cred);``
- This releases a reference to the given set of credentials. If the
+ :c:func:`put_cred` releases a reference to the given set of credentials. If the
reference count reaches zero, the credentials will be scheduled for
destruction by the RCU system.
- ``const struct cred *get_cred(const struct cred *cred);``
- This gets a reference on a live set of credentials, returning a pointer to
- that set of credentials.
+ :c:func:`get_cred` gets a reference on a live set of credentials,
+ returning a pointer to that set of credentials.
- ``struct cred *get_new_cred(struct cred *cred);``
- This gets a reference on a set of credentials that is under construction
- and is thus still mutable, returning a pointer to that set of credentials.
+ :c:func:`get_new_cred` gets a reference on a set of credentials
+ that is under construction and is thus still mutable, returning a
+ pointer to that set of credentials.
Open File Credentials
@@ -546,9 +538,20 @@ Overriding the VFS's Use of Credentials
=======================================
Under some circumstances it is desirable to override the credentials used by
-the VFS, and that can be done by calling into such as ``vfs_mkdir()`` with a
+the VFS, and that can be done by calling into such as :c:func:`vfs_mkdir` with a
different set of credentials. This is done in the following places:
* ``sys_faccessat()``.
* ``do_coredump()``.
* nfs4recover.c.
+
+List of functions for managing credentials
+==========================================
+
+.. kernel-doc:: include/linux/cred.h
+
+.. kernel-doc:: kernel/cred.c
+ :export:
+
+.. kernel-doc:: kernel/groups.c
+ :export:
diff --git a/kernel/groups.c b/kernel/groups.c
index daae2f2dc6d4..dddbb52f9035 100644
--- a/kernel/groups.c
+++ b/kernel/groups.c
@@ -86,6 +86,19 @@ static int gid_cmp(const void *_a, const void *_b)
return gid_gt(a, b) - gid_lt(a, b);
}
+/**
+ * groups_sort - sort supplementary group list numerically
+ * @group_info: The group list
+ *
+ * groups_search() uses a binary search to see if a
+ * given gid is a member of a group list, so the list must be
+ * sorted. This can be achieved by calling groups_sort().
+ * As a &struct group_info is often shared, and as this function
+ * can temporary permute elements even of a sorted list,
+ * groups_sort() must be called *before* a @struct group_info
+ * is added to a credential, typically using set_groups() or
+ * set_current_groups().
+ */
void groups_sort(struct group_info *group_info)
{
sort(group_info->gid, group_info->ngroups, sizeof(*group_info->gid),
@@ -119,6 +132,10 @@ int groups_search(const struct group_info *group_info, kgid_t grp)
* set_groups - Change a group subscription in a set of credentials
* @new: The newly prepared set of credentials to alter
* @group_info: The group list to install
+ *
+ * The group list must already be sorted (by groups_sort())
+ * and the credential must not be in active use. To change the
+ * groups list of an active credential, use set_current_groups().
*/
void set_groups(struct cred *new, struct group_info *group_info)
{
@@ -134,7 +151,7 @@ EXPORT_SYMBOL(set_groups);
* @group_info: The group list to impose
*
* Validate a group subscription and, if valid, impose it upon current's task
- * security record.
+ * security record. The group list must already be sorted.
*/
int set_current_groups(struct group_info *group_info)
{
--
2.14.0.rc0.dirty
On Sat, 6 Jan 2018 12:20:13 -0800
Matthew Wilcox <[email protected]> wrote:
> I've been thinking about all the kernel-doc we have that's completely
> unincorporated. I've also been thinking about core-api/kernel-api.rst
> which to my mind is completely unreadable in its current form -- look at
> https://www.kernel.org/doc/html/latest/core-api/kernel-api.html and you
> wouldn't really know there's anything in it beyond the List Management
> Functions.
Yes, it's a mess.
> I think the right path forward is to have kernel-api.rst be the dumping
> ground for all the files with kernel-doc but nothing more. That gives
> us somewhere to link to.
That's kind of what it is now :)
Note that we have a script now (scripts/find-unused-docs.sh) that can
seek out kerneldoc comments that aren't yet pulled in to the RST document
tree.
> Then we need little stories about how all the functions in a subsystem
> fit together. For example, we can create a list.rst which explains how
> this is a doubly-linked list that you use by embedding a list_head into
> your data structure, and has O(1) insertion/deletion, etc, etc. Then we
> would move all the list.h kernel-doc from kernel-api.rst into list.rst.
>
> Is this a reasonable strategy to follow? Does anyone have a better
> strategy?
That more-or-less corresponds to what I've been aiming at. All of
Documentation/ currently is a bit of a dumping ground; I'd really like to
knit it together into something coherent and useful. Progress on that
front has been slower than I would have liked - I've been distracted by a
number of other things. Funny, I've never heard of that happening before.
The "little stories", incidentally, can also go into DOC: sections in the
source itself. There's a bit of tension, though, between doing that and
putting the stories in the RST files. The former approach puts the docs
where they might be most useful and most likely to be updated, but it can
also leave the RST files as a collection of kerneldoc directives that is
rather unhelpful to read in unprocessed form. Since the whole idea is to
not require any sort of processing to make the docs useful, I don't
entirely like that outcome.
Thanks,
jon
On Mon, 08 Jan 2018 10:39:14 +1100
NeilBrown <[email protected]> wrote:
> > There is value in using the c:func syntax, as it will generate
> > cross-references to the kerneldoc comments for those functions. In this
> > case, it would appear that these comments exist, but nobody has pulled
> > them into the docs yet. I took the liberty of adding :c:func: references
> > to this patch on its way into docs-next so that things will Just Work on
> > that happy day when somebody adds the function documentation.
>
> Is this the sort of thing that might result in that happy day?
> I didn't spend tooo much time on it, in case including the
> kernel-doc in credentials.rst like this was the wrong direction.
>From a very quick look, yes. I'll take a closer look soon. Life has been
a bit ... busy ... around here...
Thanks,
jon