[PATCH 0/3, v3] Move groups_sort outisde of set_groups

[PATCH 0/3, v3] Move groups_sort outisde of set_groups

[Thiago Rafael Becker]

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

[PATCH 1/3, v3] kernel: make groups_sort globally visible

[Thiago Rafael Becker]

In preparation to move group_info sorting to the caller,
make group_sort globally visible.

Signed-off-by: Thiago Rafael Becker <thiago.becker@gmail.com>
---
 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 *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

[PATCH 2/3, v3] kernel: Move groups_sort to the caller of set_groups.

[Thiago Rafael Becker]

The responsibility for calling groups_sort is now on the caller
of set_groups.

Signed-off-by: Thiago Rafael Becker <thiago.becker@gmail.com>
---
 kernel/groups.c           | 1 +
 kernel/uid16.c            | 1 +
 net/sunrpc/svcauth_unix.c | 7 +++++++
 3 files changed, 9 insertions(+)

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/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
index 740b67d..94e2ac0 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;
-- 
2.9.5

[PATCH 3/3, v3] kernel: set_groups doesn't call groups_sort anymore.

[Thiago Rafael Becker]

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 <thiago.becker@gmail.com>
---
 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

Re: [PATCH 2/3, v3] kernel: Move groups_sort to the caller of set_groups.

[Thiago Rafael Becker]

Disregard this. git reset --hard without a git stash. Sorry about it.

On Mon, 4 Dec 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 <thiago.becker@gmail.com>
> ---
> kernel/groups.c           | 1 +
> kernel/uid16.c            | 1 +
> net/sunrpc/svcauth_unix.c | 7 +++++++
> 3 files changed, 9 insertions(+)
>
> 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/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
> index 740b67d..94e2ac0 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;
> -- 
> 2.9.5
>
>

[PATCH 0/3 v3] Move groups_sort outisde of set_groups

[Thiago Rafael Becker]

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

Re: [PATCH 0/3 v3] Move groups_sort outisde of set_groups

[J. Bruce Fields]

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