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

[PATCH 0/3, V2] 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, V2] 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, V2] 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 f81eaa8..91e3d34 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

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1858 bytes --]

Hi Thiago,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.15-rc1 next-20171201]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Thiago-Rafael-Becker/kernel-Move-groups_sort-to-the-caller-of-set_groups/20171203-191757
config: x86_64-randconfig-x011-201749 (attached as .config)
compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   kernel/uid16.c: In function 'SYSC_setgroups16':
>> kernel/uid16.c:195:2: error: implicit declaration of function 'groups_sort'; did you mean 'cgroup_fork'? [-Werror=implicit-function-declaration]
     groups_sort(group_info);
     ^~~~~~~~~~~
     cgroup_fork
   cc1: some warnings being treated as errors

vim +195 kernel/uid16.c

   175	
   176	SYSCALL_DEFINE2(setgroups16, int, gidsetsize, old_gid_t __user *, grouplist)
   177	{
   178		struct group_info *group_info;
   179		int retval;
   180	
   181		if (!may_setgroups())
   182			return -EPERM;
   183		if ((unsigned)gidsetsize > NGROUPS_MAX)
   184			return -EINVAL;
   185	
   186		group_info = groups_alloc(gidsetsize);
   187		if (!group_info)
   188			return -ENOMEM;
   189		retval = groups16_from_user(group_info, grouplist);
   190		if (retval) {
   191			put_group_info(group_info);
   192			return retval;
   193		}
   194	
 > 195		groups_sort(group_info);
   196		retval = set_current_groups(group_info);
   197		put_group_info(group_info);
   198	
   199		return retval;
   200	}
   201	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24716 bytes --]

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

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2656 bytes --]

On Thu, Nov 30 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 f81eaa8..91e3d34 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;


I think you need to add groups_sort() in a few more places.
Almost anywhere that calls groups_alloc() should be considered.
net/sunrpc/svcauth_unix.c, net/sunrpc/auth_gss/svcauth_gss.c,
fs/nfsd/auth.c definitely need it.

I wonder if there is a more robust way to fix this.
Maybe:
  groups_alloc() could initialize ->usage to -1
  groups_sort() to require it be -1, and change it to 1
  get_group_info() complains if ->usage is -1 ???

or something.
Maybe it could be done with types.
'struct group_info' could be declared with ngroups and
gid[] as const.
'struct group_info_unsorted' could be the same structure but
without the 'const'.
Then groups_sort() takes a 'struct group_info_unsorted *' and returns
a 'struct group_info *'...

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Thiago Rafael Becker]

On Mon, 4 Dec 2017, NeilBrown wrote:

> I think you need to add groups_sort() in a few more places.
> Almost anywhere that calls groups_alloc() should be considered.
> net/sunrpc/svcauth_unix.c, net/sunrpc/auth_gss/svcauth_gss.c,
> fs/nfsd/auth.c definitely need it.

So are any other functions that modify group_info. OK, I think I'll 
implement the type detection below as it helps detecting where these 
situations are located.

This may take some time to make sane. I wonder if we shouldn't 
accept the first change suggested to fix the corruption detected in 
auth.unix.gid while I work on a new set of patches. Also, that patch 
doesn't change behavior of set_groups, and is easier to backport if 
distros relying on older kernels need to do so and change behavior. The 
first suggestion is undergoing tests, and so far we didn't detect any 
new corruptions on auth.unix.gid.

> Maybe it could be done with types.

I changed the interfaces on groups_{alloc,sort} to check. There are some 
extra changes needed in groups_from_user and others to make this viable, 
but I like it and I'll try to make it happen.

> Thanks,
> NeilBrown
>

Thanks,
trbecker

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

[J. Bruce Fields]

On Mon, Dec 04, 2017 at 01:39:37PM -0200, Thiago Rafael Becker wrote:
> 
> 
> On Mon, 4 Dec 2017, NeilBrown wrote:
> 
> >I think you need to add groups_sort() in a few more places.
> >Almost anywhere that calls groups_alloc() should be considered.
> >net/sunrpc/svcauth_unix.c, net/sunrpc/auth_gss/svcauth_gss.c,
> >fs/nfsd/auth.c definitely need it.
> 
> So are any other functions that modify group_info. OK, I think I'll
> implement the type detection below as it helps detecting where these
> situations are located.
> 
> This may take some time to make sane. I wonder if we shouldn't
> accept the first change suggested to fix the corruption detected in
> auth.unix.gid while I work on a new set of patches. Also, that patch
> doesn't change behavior of set_groups, and is easier to backport if
> distros relying on older kernels need to do so and change behavior.
> The first suggestion is undergoing tests, and so far we didn't
> detect any new corruptions on auth.unix.gid.

I'm a little confused--we can remedy the oversight Neil points out just
by adding a few more group_sort()s, and that shouldn't be hard, right?

I'd be OK with doing that first and then adding a code to enforce the
sorting second.

--b.

> 
> >Maybe it could be done with types.
> 
> I changed the interfaces on groups_{alloc,sort} to check. There are
> some extra changes needed in groups_from_user and others to make
> this viable, but I like it and I'll try to make it happen.
> 
> >Thanks,
> >NeilBrown
> >
> 
> Thanks,
> trbecker
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Thiago Rafael Becker]

On Mon, 4 Dec 2017, J. Bruce Fields wrote:

> On Mon, Dec 04, 2017 at 01:39:37PM -0200, Thiago Rafael Becker wrote:
>>
>>
>> On Mon, 4 Dec 2017, NeilBrown wrote:
>>
>>> I think you need to add groups_sort() in a few more places.
>>> Almost anywhere that calls groups_alloc() should be considered.
>>> net/sunrpc/svcauth_unix.c, net/sunrpc/auth_gss/svcauth_gss.c,
>>> fs/nfsd/auth.c definitely need it.
>>
>> So are any other functions that modify group_info. OK, I think I'll
>> implement the type detection below as it helps detecting where these
>> situations are located.
>>
>> This may take some time to make sane. I wonder if we shouldn't
>> accept the first change suggested to fix the corruption detected in
>> auth.unix.gid while I work on a new set of patches. Also, that patch
>> doesn't change behavior of set_groups, and is easier to backport if
>> distros relying on older kernels need to do so and change behavior.
>> The first suggestion is undergoing tests, and so far we didn't
>> detect any new corruptions on auth.unix.gid.
>
> I'm a little confused--we can remedy the oversight Neil points out just
> by adding a few more group_sort()s, and that shouldn't be hard, right?
>
> I'd be OK with doing that first and then adding a code to enforce the
> sorting second.

Ok. Sending a new set. The set will cover all calls to group_alloc, after 
the function has ended filling the groups.

> --b.
>
>>
>>> Maybe it could be done with types.
>>
>> I changed the interfaces on groups_{alloc,sort} to check. There are
>> some extra changes needed in groups_from_user and others to make
>> this viable, but I like it and I'll try to make it happen.
>>
>>> Thanks,
>>> NeilBrown
>>>
>>
>> Thanks,
>> trbecker
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1978 bytes --]

On Mon, Dec 04 2017, Thiago Rafael Becker wrote:

> On Mon, 4 Dec 2017, NeilBrown wrote:
>
>> I think you need to add groups_sort() in a few more places.
>> Almost anywhere that calls groups_alloc() should be considered.
>> net/sunrpc/svcauth_unix.c, net/sunrpc/auth_gss/svcauth_gss.c,
>> fs/nfsd/auth.c definitely need it.
>
> So are any other functions that modify group_info. OK, I think I'll 
> implement the type detection below as it helps detecting where these 
> situations are located.
>
> This may take some time to make sane. I wonder if we shouldn't 
> accept the first change suggested to fix the corruption detected in 
> auth.unix.gid while I work on a new set of patches. 

As we don't seem to be pursuing this possibility is probably isn't very
important, but I'd like to point out that the original fix isn't a true
fix.
It just sorts a shared group_info early.  This does not stop corruption.
Every time a thread calls set_groups() on that group_info it will be
sorted again.
The sort algorithm used is the heap sort, and a heap sort always moves
elements in the array around - it does not leave a sorted array
untouched (unlike e.g. the quick sort which doesn't move anything in a
sorted array).
So it is still possible for two calls to groups_sort() to race.
We *need* to move groups_sort() out of set_groups().

Thanks,
NeilBrown


>    Also, that patch 
> doesn't change behavior of set_groups, and is easier to backport if 
> distros relying on older kernels need to do so and change behavior. The 
> first suggestion is undergoing tests, and so far we didn't detect any 
> new corruptions on auth.unix.gid.
>
>> Maybe it could be done with types.
>
> I changed the interfaces on groups_{alloc,sort} to check. There are some 
> extra changes needed in groups_from_user and others to make this viable, 
> but I like it and I'll try to make it happen.
>
>> Thanks,
>> NeilBrown
>>
>
> Thanks,
> trbecker

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Matthew Wilcox]

On Tue, Dec 05, 2017 at 07:11:00AM +1100, NeilBrown wrote:
> As we don't seem to be pursuing this possibility is probably isn't very
> important, but I'd like to point out that the original fix isn't a true
> fix.
> It just sorts a shared group_info early.  This does not stop corruption.
> Every time a thread calls set_groups() on that group_info it will be
> sorted again.
> The sort algorithm used is the heap sort, and a heap sort always moves
> elements in the array around - it does not leave a sorted array
> untouched (unlike e.g. the quick sort which doesn't move anything in a
> sorted array).
> So it is still possible for two calls to groups_sort() to race.
> We *need* to move groups_sort() out of set_groups().

It must be relatively common to sort an already-sorted array.  I wonder
if something like this patch would be worthwhile?

I have deliberately broken this patch so it can't be applied.  I haven't
tested it, and for all I know, I got the sign of cmp_func wrong.

diff --git a/lib/sort.c b/lib/sort.c
index d6b7a202b0b6..2b527fde6dad 100644
--- a/lib/sort.c
+++ b/lib/sort.c
@@ -75,7 +75,14 @@ void sort(void *base, size_t num, size_t size,
 			swap_func = generic_swap;
 	}
 
-       /* heapify */
+	/* Do not sort an already-sorted array */
+	for (c = 0; c < (n - size); c += size) {
+		if (cmp_func(base + c, base + c + size) < 0)
+			goto heapify;
+	}
+	return;
+
+heapify:
 	for ( ; i >= 0; i -= size) {
 		for (r = i; r * 2 + size < n; r  = c) {
 			c = r * 2 + size;

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

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2032 bytes --]

On Tue, Dec 05 2017, Matthew Wilcox wrote:

> On Tue, Dec 05, 2017 at 07:11:00AM +1100, NeilBrown wrote:
>> As we don't seem to be pursuing this possibility is probably isn't very
>> important, but I'd like to point out that the original fix isn't a true
>> fix.
>> It just sorts a shared group_info early.  This does not stop corruption.
>> Every time a thread calls set_groups() on that group_info it will be
>> sorted again.
>> The sort algorithm used is the heap sort, and a heap sort always moves
>> elements in the array around - it does not leave a sorted array
>> untouched (unlike e.g. the quick sort which doesn't move anything in a
>> sorted array).
>> So it is still possible for two calls to groups_sort() to race.
>> We *need* to move groups_sort() out of set_groups().
>
> It must be relatively common to sort an already-sorted array.  I wonder
> if something like this patch would be worthwhile?
>
> I have deliberately broken this patch so it can't be applied.  I haven't
> tested it, and for all I know, I got the sign of cmp_func wrong.
>
> diff --git a/lib/sort.c b/lib/sort.c
> index d6b7a202b0b6..2b527fde6dad 100644
> --- a/lib/sort.c
> +++ b/lib/sort.c
> @@ -75,7 +75,14 @@ void sort(void *base, size_t num, size_t size,
>  			swap_func = generic_swap;
>  	}
>  
> -       /* heapify */
> +	/* Do not sort an already-sorted array */
> +	for (c = 0; c < (n - size); c += size) {
> +		if (cmp_func(base + c, base + c + size) < 0)
> +			goto heapify;
> +	}
> +	return;
> +
> +heapify:
>  	for ( ; i >= 0; i -= size) {
>  		for (r = i; r * 2 + size < n; r  = c) {
>  			c = r * 2 + size;

I wondered about this possibility...
It is a clear win from a defensive-programming perspective.
Adding a small overhead to every sort call isn't ideal, but I doubt it
is noticeable (typically 2 or 3 tests I suspect).
I probably wouldn't advocate this approach, but I certainly wouldn't
fight it.
I *do* like the way you changed a comment to a label!

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Thiago Rafael Becker]

On Tue, 5 Dec 2017, Matthew Wilcox wrote:

> On Tue, Dec 05, 2017 at 07:11:00AM +1100, NeilBrown wrote:
>> As we don't seem to be pursuing this possibility is probably isn't very
>> important, but I'd like to point out that the original fix isn't a true
>> fix.
>> It just sorts a shared group_info early.  This does not stop corruption.
>> Every time a thread calls set_groups() on that group_info it will be
>> sorted again.
>> The sort algorithm used is the heap sort, and a heap sort always moves
>> elements in the array around - it does not leave a sorted array
>> untouched (unlike e.g. the quick sort which doesn't move anything in a
>> sorted array).
>> So it is still possible for two calls to groups_sort() to race.
>> We *need* to move groups_sort() out of set_groups().

Hum, makes sense. I've applied it to the most recent Fedora kernel (that 
uses heapsort) and I didn't see the problem again. I should run a few more 
repetitions to be sure.

> It must be relatively common to sort an already-sorted array.  I wonder
> if something like this patch would be worthwhile?
>
> I have deliberately broken this patch so it can't be applied.  I haven't
> tested it, and for all I know, I got the sign of cmp_func wrong.
>
> diff --git a/lib/sort.c b/lib/sort.c
> index d6b7a202b0b6..2b527fde6dad 100644
> --- a/lib/sort.c
> +++ b/lib/sort.c
> @@ -75,7 +75,14 @@ void sort(void *base, size_t num, size_t size,
> 			swap_func = generic_swap;
> 	}
>
> -       /* heapify */
> +	/* Do not sort an already-sorted array */
> +	for (c = 0; c < (n - size); c += size) {
> +		if (cmp_func(base + c, base + c + size) < 0)
> +			goto heapify;
> +	}
> +	return;
> +
> +heapify:
> 	for ( ; i >= 0; i -= size) {
> 		for (r = i; r * 2 + size < n; r  = c) {
> 			c = r * 2 + size;

The bug happens when two threads enter sort_groups for the same 
group info in parallel, and one thread starts overwriting values 
that another thread may already have "heapified" or sorted.

Thread A                  Thread B
Enter groups_sort
                           Enter groups_sort
.
.
.
Return from groups_sort
                           .
                           .
                           .
                           Return from groups_sort

Wouldn't this patch just make both threads see the structure as unsorted 
and sort them?

Thanks,
trbecker

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

[Matthew Wilcox]

On Tue, Dec 05, 2017 at 09:03:02PM -0200, Thiago Rafael Becker wrote:
> On Tue, 5 Dec 2017, Matthew Wilcox wrote:
> > It must be relatively common to sort an already-sorted array.  I wonder
> > if something like this patch would be worthwhile?
> 
> The bug happens when two threads enter sort_groups for the same group info
> in parallel, and one thread starts overwriting values that another thread
> may already have "heapified" or sorted.
> 
> Thread A                  Thread B
> Enter groups_sort
>                           Enter groups_sort
> .
> .
> .
> Return from groups_sort
>                           .
>                           .
>                           .
>                           Return from groups_sort
> 
> Wouldn't this patch just make both threads see the structure as unsorted and
> sort them?

I'm sorry, I wasn't clear.  I wasn't commenting on the original bug (and
I believe your analysis to be correct unless there's some locking which
prevents two calls to group_sort from happening at the same time).

I was wondering about whether our sort() implementation is the best that
it could possibly be, and whether having the trait of not modifying an
already-sorted array is worthwhile (eg it would not cause cachelines to
enter the dirty state if they were already clean).

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

[J. Bruce Fields]

On Tue, Dec 05, 2017 at 07:11:00AM +1100, NeilBrown wrote:
> On Mon, Dec 04 2017, Thiago Rafael Becker wrote:
> 
> > On Mon, 4 Dec 2017, NeilBrown wrote:
> >
> >> I think you need to add groups_sort() in a few more places.
> >> Almost anywhere that calls groups_alloc() should be considered.
> >> net/sunrpc/svcauth_unix.c, net/sunrpc/auth_gss/svcauth_gss.c,
> >> fs/nfsd/auth.c definitely need it.
> >
> > So are any other functions that modify group_info. OK, I think I'll 
> > implement the type detection below as it helps detecting where these 
> > situations are located.
> >
> > This may take some time to make sane. I wonder if we shouldn't 
> > accept the first change suggested to fix the corruption detected in 
> > auth.unix.gid while I work on a new set of patches. 
> 
> As we don't seem to be pursuing this possibility is probably isn't very
> important, but I'd like to point out that the original fix isn't a true
> fix.
> It just sorts a shared group_info early.  This does not stop corruption.
> Every time a thread calls set_groups() on that group_info it will be
> sorted again.
> The sort algorithm used is the heap sort, and a heap sort always moves
> elements in the array around - it does not leave a sorted array
> untouched (unlike e.g. the quick sort which doesn't move anything in a
> sorted array).
> So it is still possible for two calls to groups_sort() to race.
> We *need* to move groups_sort() out of set_groups().

By the way,

	https://bugzilla.kernel.org/show_bug.cgi?id=197887

looks like it might be this bug.  They report it started to happen on
upgrade from a 4.10-ish kernel to a 4.13-ish kernel, which would include
the commit (b7b2562f725) that converted groups_sort to a function that
is no longer a no-op in the already-sorted case.

Looks like rpc.mountd just uses getgrouplist(), and I don't think that
guarantees any particular oder.  I wonder if it's the case that many
common configurations always pass down an already-sorted list.  In that
case this may show up as a 4.13 regression for some users.

--b.

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

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3045 bytes --]

On Tue, Dec 19 2017, J. Bruce Fields wrote:

> On Tue, Dec 05, 2017 at 07:11:00AM +1100, NeilBrown wrote:
>> On Mon, Dec 04 2017, Thiago Rafael Becker wrote:
>> 
>> > On Mon, 4 Dec 2017, NeilBrown wrote:
>> >
>> >> I think you need to add groups_sort() in a few more places.
>> >> Almost anywhere that calls groups_alloc() should be considered.
>> >> net/sunrpc/svcauth_unix.c, net/sunrpc/auth_gss/svcauth_gss.c,
>> >> fs/nfsd/auth.c definitely need it.
>> >
>> > So are any other functions that modify group_info. OK, I think I'll 
>> > implement the type detection below as it helps detecting where these 
>> > situations are located.
>> >
>> > This may take some time to make sane. I wonder if we shouldn't 
>> > accept the first change suggested to fix the corruption detected in 
>> > auth.unix.gid while I work on a new set of patches. 
>> 
>> As we don't seem to be pursuing this possibility is probably isn't very
>> important, but I'd like to point out that the original fix isn't a true
>> fix.
>> It just sorts a shared group_info early.  This does not stop corruption.
>> Every time a thread calls set_groups() on that group_info it will be
>> sorted again.
>> The sort algorithm used is the heap sort, and a heap sort always moves
>> elements in the array around - it does not leave a sorted array
>> untouched (unlike e.g. the quick sort which doesn't move anything in a
>> sorted array).
>> So it is still possible for two calls to groups_sort() to race.
>> We *need* to move groups_sort() out of set_groups().
>
> By the way,
>
> 	https://bugzilla.kernel.org/show_bug.cgi?id=197887
>
> looks like it might be this bug.  They report it started to happen on
> upgrade from a 4.10-ish kernel to a 4.13-ish kernel, which would include
> the commit (b7b2562f725) that converted groups_sort to a function that
> is no longer a no-op in the already-sorted case.
>
> Looks like rpc.mountd just uses getgrouplist(), and I don't think that
> guarantees any particular oder.  I wonder if it's the case that many
> common configurations always pass down an already-sorted list.  In that
> case this may show up as a 4.13 regression for some users.

I think the 4.13 patch makes the problem worse, but isn't the only
cause.  We had this reported against SLE12 which is based on 3.12 and
doesn't have that patch backported.

Before 4.13 the problem only occurs if getgrouplist() returns an
unsorted list, and if two threads both try to sort this list at the same
time they can corrupt it.
If you are using /etc/passwd and /etc/group, then the order of groups
returned by getgrouplist is the order that the groups were added to
/etc/group (or at least, the order they appear in the file).
This will often be numerical order, but site-local policies could easily
result in a different order.

4.13-stable is EOL and the patch has already been queued for 4.14.8.
Greg tried 4.9 and 4.4, but the patch didn't apply.  Is anyone
volunteering to do the backport???

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[PATCH 3/3, V2] 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

[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 *);
 
 /*
  * 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>
---
 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

[PATCH 2/3] 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>
---
 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

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

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 4362 bytes --]

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

Looks good to me, thanks.
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Thiago Rafael Becker]

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.

[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 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

[PATCH v4] kernel: make groups_sort calling a responsibility group_info allocators

[Thiago Rafael Becker]

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

Re: [PATCH v4] kernel: make groups_sort calling a responsibility group_info allocators

[Matthew Wilcox]

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.

[PATCH v5] kernel: make groups_sort calling a responsibility group_info allocators

[Thiago Rafael Becker]

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

Re: [PATCH v5] kernel: make groups_sort calling a responsibility group_info allocators

[Matthew Wilcox]

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

Reviewed-by: Matthew Wilcox <mawilcox@microsoft.com>

Re: [PATCH v5] kernel: make groups_sort calling a responsibility group_info allocators

[J. Bruce Fields]

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 <thiago.becker@gmail.com>
> ---
>  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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] kernel: make groups_sort calling a responsibility group_info allocators

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 6512 bytes --]

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

Reviewed-by: NeilBrown <neilb@suse.com>

Thanks.  I like this better as a single patch.

I'd probably suggest "Cc: stable@vger.kernel.org".
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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v5] kernel: make groups_sort calling a responsibility group_info allocators

[David Howells]

NeilBrown <neilb@suse.com> 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

[PATCH] Documentation: security/credentials.rst: explain need to sort group_list

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1476 bytes --]


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 <neilb@suse.com>
---
 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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] Documentation: security/credentials.rst: explain need to sort group_list

[Matthew Wilcox]

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
> 

Re: [PATCH] Documentation: security/credentials.rst: explain need to sort group_list

[Jonathan Corbet]

On Tue, 2 Jan 2018 13:04:31 -0800
Matthew Wilcox <willy@infradead.org> 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

Re: [PATCH] Documentation: security/credentials.rst: explain need to sort group_list

[Matthew Wilcox]

On Sat, Jan 06, 2018 at 11:09:08AM -0700, Jonathan Corbet wrote:
> On Tue, 2 Jan 2018 13:04:31 -0800
> Matthew Wilcox <willy@infradead.org> 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 :-)

Re: [PATCH] Documentation: security/credentials.rst: explain need to sort group_list

[Randy Dunlap]

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

Re: [PATCH] Documentation: security/credentials.rst: explain need to sort group_list

[Jonathan Corbet]

On Sat, 6 Jan 2018 12:20:13 -0800
Matthew Wilcox <willy@infradead.org> 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

Re: [PATCH] Documentation: security/credentials.rst: explain need to sort group_list

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 9308 bytes --]

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 <neilb@suse.com>
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 <neilb@suse.com>
---
 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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] Documentation: security/credentials.rst: explain need to sort group_list

[Jonathan Corbet]

On Mon, 08 Jan 2018 10:39:14 +1100
NeilBrown <neilb@suse.com> 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.