Move groups_sort outisde of set_groups
Subsystems (8)
v2 cover Move groups_sort outisde of set_groups
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(-)
Signature · 1 line
2.9.5
PATCH v2 1/3 kernel: make groups_sort globally visible
In preparation to move group_info sorting to the caller, make group_sort globally visible.
--- include/linux/cred.h | 1 + kernel/groups.c | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-)
include/linux/cred.h +1 -0
@@ -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
kernel/groups.c +3 -1
@@ -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)
Signature · 1 line
2.9.5
PATCH v2 2/3 kernel: Move groups_sort to the caller of set_groups.
In reply to Thiago Rafael BeckerThe responsibility for calling groups_sort is now on the caller of set_groups.
--- kernel/groups.c | 1 + kernel/uid16.c | 1 + net/sunrpc/svcauth_unix.c | 7 +++++++ 3 files changed, 9 insertions(+)
kernel/groups.c +1 -0
@@ -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);
kernel/uid16.c +1 -0
@@ -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);
net/sunrpc/svcauth_unix.c +7 -0
@@ -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;
Signature · 1 line
2.9.5
kbuild test robot
build botRe: [PATCH 2/3, V2] kernel: Move groups_sort to the caller of set_groups.
Thank you for the patch! Yet something to improve:
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_forkcc1: 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
Re: [PATCH 2/3, V2] kernel: Move groups_sort to the caller of set_groups.
In reply to Thiago Rafael BeckerQuoted 49 lines · Thiago Rafael Becker
The responsibility for calling
groups_sortis now on the caller ofset_groups.Signed-off-by Thiago Rafael Becker <[email protected]>--- kernel/groups.c | 1 + kernel/uid16.c | 1 + net/sunrpc/svcauth_unix.c | 7 +++++++ 3 files changed, 9 insertions(+)kernel/groups.c +1
-0@@ -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);kernel/uid16.c +1
-0@@ -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);net/sunrpc/svcauth_unix.c +1
-0@@ -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; }
net/sunrpc/svcauth_unix.c static int unix_gid_parse(struct cache_detail *cd, · line 532
Quoted 10 lines
net/sunrpc/svcauth_unix.c +6
-0+/* 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
Re: [PATCH 2/3, V2] kernel: Move groups_sort to the caller of set_groups.
Quoted 4 lines · NeilBrown
I think you need to add
groups_sort()in a few more places. Almost anywhere that callsgroups_alloc()should be considered.net/sunrpc/svcauth_unix.c,net/sunrpc/auth_gss/svcauth_gss.c,fs/nfsd/auth.cdefinitely 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.
Quoted 1 line
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.
Quoted 3 lines
Thanks,
NeilBrown
Thanks,
trbecker
Re: [PATCH 2/3, V2] kernel: Move groups_sort to the caller of set_groups.
Quoted 13 lines · Thiago Rafael Becker
Quoted 4 lines · NeilBrown
I think you need to add
groups_sort()in a few more places. Almost anywhere that callsgroups_alloc()should be considered.net/sunrpc/svcauth_unix.c,net/sunrpc/auth_gss/svcauth_gss.c,fs/nfsd/auth.cdefinitely 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.
Quoted 7 lines
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.
Quoted 17 lines
Quoted 2 lines
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_userand others to make this viable, but I like it and I'll try to make it happen.Quoted 3 lines
Thanks,
NeilBrownThanks,
trbecker
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3, V2] kernel: Move groups_sort to the caller of set_groups.
Quoted 22 lines · J. Bruce Fields
Quoted 13 lines · Thiago Rafael Becker
Quoted 4 lines · NeilBrown
I think you need to add
groups_sort()in a few more places. Almost anywhere that callsgroups_alloc()should be considered.net/sunrpc/svcauth_unix.c,net/sunrpc/auth_gss/svcauth_gss.c,fs/nfsd/auth.cdefinitely 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.Quoted 7 lines
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.
Quoted 5 lines
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.
Quoted 20 lines
--b.
Quoted 17 lines
Quoted 2 lines
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_userand others to make this viable, but I like it and I'll try to make it happen.Quoted 3 lines
Thanks,
NeilBrownThanks,
trbecker
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3, V2] kernel: Move groups_sort to the caller of set_groups.
In reply to Thiago Rafael BeckerQuoted 7 lines · Thiago Rafael Becker
Quoted 4 lines · NeilBrown
I think you need to add
groups_sort()in a few more places. Almost anywhere that callsgroups_alloc()should be considered.net/sunrpc/svcauth_unix.c,net/sunrpc/auth_gss/svcauth_gss.c,fs/nfsd/auth.cdefinitely need it.
Quoted 7 lines
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
Quoted 18 lines
Also, that patch
doesn't change behavior ofset_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.Quoted 1 line
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_userand others to make this viable, but I like it and I'll try to make it happen.Quoted 3 lines
Thanks,
NeilBrownThanks,
trbecker
Re: [PATCH 2/3, V2] kernel: Move groups_sort to the caller of set_groups.
Quoted 4 lines · NeilBrown
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 sharedgroup_infoearly. This does not stop corruption.
Quoted 8 lines
Every time a thread calls
set_groups()on thatgroup_infoit 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 togroups_sort()to race. We *need* to movegroups_sort()out ofset_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.
lib/sort.c +8 -1
@@ -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.
Quoted 30 lines · Matthew Wilcox
Quoted 4 lines · NeilBrown
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 sharedgroup_infoearly. This does not stop corruption.Quoted 8 lines
Every time a thread calls
set_groups()on thatgroup_infoit 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 togroups_sort()to race. We *need* to movegroups_sort()out ofset_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_funcwrong.lib/sort.c +1
-1@@ -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 */
lib/sort.c void sort(void *base, size_t num, size_t size, · line 88
Quoted 10 lines
lib/sort.c +7
-0+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
Re: [PATCH 2/3, V2] kernel: Move groups_sort to the caller of set_groups.
In reply to Matthew WilcoxQuoted 13 lines · Matthew Wilcox
Quoted 4 lines · NeilBrown
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 sharedgroup_infoearly. This does not stop corruption.Quoted 8 lines
Every time a thread calls
set_groups()on thatgroup_infoit 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 togroups_sort()to race. We *need* to movegroups_sort()out ofset_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.
Quoted 16 lines
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_funcwrong.lib/sort.c +1
-1@@ -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 */
lib/sort.c void sort(void *base, size_t num, size_t size, · line 88
Quoted 10 lines
lib/sort.c +7
-0+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_sortWouldn'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.
Quoted 15 lines · Thiago Rafael Becker
Quoted 2 lines · Matthew Wilcox
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_groupsfor 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
Entergroups_sort
Entergroups_sort
.
.
.
Return fromgroups_sort
Quoted 7 lines
. . . Return from groups_sortWouldn'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.
In reply to NeilBrownQuoted 17 lines · NeilBrown
Quoted 7 lines · Thiago Rafael Becker
Quoted 4 lines · NeilBrown
I think you need to add
groups_sort()in a few more places. Almost anywhere that callsgroups_alloc()should be considered.net/sunrpc/svcauth_unix.c,net/sunrpc/auth_gss/svcauth_gss.c,fs/nfsd/auth.cdefinitely need it.Quoted 7 lines
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.
Quoted 12 lines
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 sharedgroup_infoearly. This does not stop corruption. Every time a thread callsset_groups()on thatgroup_infoit 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 togroups_sort()to race. We *need* to movegroups_sort()out ofset_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.
Quoted 40 lines · J. Bruce Fields
Quoted 17 lines · NeilBrown
Quoted 7 lines · Thiago Rafael Becker
Quoted 4 lines · NeilBrown
I think you need to add
groups_sort()in a few more places. Almost anywhere that callsgroups_alloc()should be considered.net/sunrpc/svcauth_unix.c,net/sunrpc/auth_gss/svcauth_gss.c,fs/nfsd/auth.cdefinitely need it.Quoted 7 lines
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.
Quoted 12 lines
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 sharedgroup_infoearly. This does not stop corruption. Every time a thread callsset_groups()on thatgroup_infoit 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 togroups_sort()to race. We *need* to movegroups_sort()out ofset_groups().By the way,
https://bugzilla.kernel.org/show_bug.cgi?id=197887looks 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 convertedgroups_sortto a function that is no longer a no-op in the already-sorted case.
Quoted 4 lines
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
PATCH v2 3/3 kernel: set_groups doesn't call groups_sort anymore.
In reply to Thiago Rafael BeckerWhen 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.
--- kernel/groups.c | 1 - 1 file changed, 1 deletion(-)
kernel/groups.c +0 -1
@@ -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; }
Signature · 1 line
2.9.5
v3 cover Move groups_sort outisde of set_groups
In reply to Thiago Rafael BeckerIn 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(-)
Signature · 1 line
2.9.5
PATCH v3 1/3 kernel: make groups_sort globally visible
In preparation to move group_info sorting to the caller, make group_sort globally visible.
--- include/linux/cred.h | 1 + kernel/groups.c | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-)
include/linux/cred.h +1 -0
@@ -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
kernel/groups.c +3 -1
@@ -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)
Signature · 1 line
2.9.5
PATCH v3 2/3 kernel: Move groups_sort to the caller of set_groups.
In reply to Thiago Rafael BeckerThe responsibility for calling groups_sort is now on the caller of set_groups.
--- 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(+)
fs/nfsd/auth.c +3 -0
@@ -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);
kernel/groups.c +1 -0
@@ -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);
kernel/uid16.c +1 -0
@@ -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);
net/sunrpc/auth_gss/gss_rpc_xdr.c +1 -0
@@ -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:
net/sunrpc/auth_gss/svcauth_gss.c +1 -0
@@ -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);
net/sunrpc/svcauth_unix.c +8 -0
@@ -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 ;
Signature · 1 line
2.9.5
PATCH v2 2/3 kernel: Move groups_sort to the caller of set_groups.
The responsibility for calling groups_sort is now on the caller of set_groups.
--- 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(+)
arch/s390/kernel/compat_linux.c +1 -0
@@ -263,6 +263,7 @@ COMPAT_SYSCALL_DEFINE2(s390_setgroups16, int, gidsetsize, u16 __user *, grouplisreturn retval; } +groups_sort (group_info); retval =set_current_groups (group_info);put_group_info (group_info);
fs/nfsd/auth.c +3 -0
@@ -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);
kernel/groups.c +1 -0
@@ -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);
kernel/uid16.c +1 -0
@@ -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);
net/sunrpc/auth_gss/gss_rpc_xdr.c +1 -0
@@ -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:
net/sunrpc/auth_gss/svcauth_gss.c +1 -0
@@ -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);
net/sunrpc/svcauth_unix.c +8 -0
@@ -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 ;
Signature · 1 line
2.9.5
Re: [PATCH 2/3] kernel: Move groups_sort to the caller of set_groups.
Quoted 123 lines · Thiago Rafael Becker
The responsibility for calling
groups_sortis now on the caller ofset_groups.Signed-off-by Thiago Rafael Becker <[email protected]>--- arch/s390/kernel/compat_linux.c | 1 + fs/nfsd/auth.c | 3 +++ kernel/groups.c | 1 + kernel/uid16.c | 1 + net/sunrpc/auth_gss/gss_rpc_xdr.c | 1 + net/sunrpc/auth_gss/svcauth_gss.c | 1 + net/sunrpc/svcauth_unix.c | 8 ++++++++ 7 files changed, 16 insertions(+)arch/s390/kernel/compat_linux.c +1
-0@@ -263,6 +263,7 @@ COMPAT_SYSCALL_DEFINE2(s390_setgroups16, int, gidsetsize, u16 __user *, grouplisreturn retval; } +groups_sort (group_info); retval =set_current_groups (group_info);put_group_info (group_info);fs/nfsd/auth.c +3
-0@@ -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);kernel/groups.c +1
-0@@ -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);kernel/uid16.c +1
-0@@ -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);net/sunrpc/auth_gss/gss_rpc_xdr.c +1
-0@@ -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:net/sunrpc/auth_gss/svcauth_gss.c +1
-0@@ -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);net/sunrpc/svcauth_unix.c +8
-0@@ -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 ;Signature · 1 line
2.9.5
Looks good to me, thanks.
NeilBrown
Re: [PATCH 2/3 v3] kernel: Move groups_sort to the caller of set_groups.
In reply to Thiago Rafael BeckerIt seems I missed another one in arch/s390, uploaded an update.
Quoted 2 lines · Thiago Rafael Becker
The responsibility for calling
groups_sortis now on the caller ofset_groups.
PATCH v3 3/3 kernel: set_groups doesn't call groups_sort anymore.
In reply to Thiago Rafael BeckerWhen 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.
--- kernel/groups.c | 1 - 1 file changed, 1 deletion(-)
kernel/groups.c +0 -1
@@ -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; }
Signature · 1 line
2.9.5
ACK to these patches from me. I'm not sure who should pick them up....
--b.
Quoted 32 lines · Thiago Rafael Becker
In cases where
group_infois cached (e.g. sunrpc), multiplpe threads may callset_groupswith a freshly createdgroup_infocache (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 ofset_groups.These patches:
- Exportgroups_sortininclude/linux/cred.h
- Add a call togroups_sortafter the groups are inserted ingroup_info
- Remove the call tosort_groupsfromset_groupsThiago Rafael Becker (3):
kernel: makegroups_sortglobally visible
kernel: Movegroups_sortto the caller ofset_groups.
kernel:set_groupsdoesn't callgroups_sortanymore.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(-)Signature · 1 line
2.9.5
PATCH v4 1/1 kernel: make groups_sort calling a responsibility group_info allocators
In reply to Thiago Rafael BeckerIn 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
--- 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(-)
arch/s390/kernel/compat_linux.c +1 -0
@@ -263,6 +263,7 @@ COMPAT_SYSCALL_DEFINE2(s390_setgroups16, int, gidsetsize, u16 __user *, grouplisreturn retval; } +groups_sort (group_info); retval =set_current_groups (group_info);put_group_info (group_info);
fs/nfsd/auth.c +3 -0
@@ -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);
include/linux/cred.h +1 -0
@@ -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
kernel/groups.c +4 -2
@@ -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);
kernel/uid16.c +1 -0
@@ -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);
net/sunrpc/auth_gss/gss_rpc_xdr.c +1 -0
@@ -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:
net/sunrpc/auth_gss/svcauth_gss.c +1 -0
@@ -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);
net/sunrpc/svcauth_unix.c +7 -0
@@ -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 ;
Signature · 1 line
2.9.5
Re: [PATCH v4] kernel: make groups_sort calling a responsibility group_info allocators
fs/nfsd/auth.c int nfsd_setuser(struct svc_rqst *rqstp, struct svc_export *exp) · line 66
Quoted 9 lines · Thiago Rafael Becker
+++
b/fs/nfsd/auth.cfs/nfsd/auth.c +3
-0@@ -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 */
Quoted 4 lines
kernel/groups.c +1
-0+++ 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); }
kernel/groups.c static int gid_cmp(const void *_a, const void *_b) · line 96
Quoted 10 lines
kernel/groups.c +3
-1-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
Quoted 7 lines
net/sunrpc/svcauth_unix.c +0
-0@@ -520,6 +520,12 @@ static int unix_gid_parse(struct cache_detail *cd, ug.gi->gid[i] = kgid; }
net/sunrpc/svcauth_unix.c static int unix_gid_parse(struct cache_detail *cd, · line 531
Quoted 10 lines
net/sunrpc/svcauth_unix.c +6
-0+/* 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 1/1 kernel: make groups_sort calling a responsibility group_info allocators
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
--- 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(-)
arch/s390/kernel/compat_linux.c +1 -0
@@ -263,6 +263,7 @@ COMPAT_SYSCALL_DEFINE2(s390_setgroups16, int, gidsetsize, u16 __user *, grouplisreturn retval; } +groups_sort (group_info); retval =set_current_groups (group_info);put_group_info (group_info);
fs/nfsd/auth.c +3 -0
@@ -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);
include/linux/cred.h +1 -0
@@ -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
kernel/groups.c +3 -2
@@ -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);
kernel/uid16.c +1 -0
@@ -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);
net/sunrpc/auth_gss/gss_rpc_xdr.c +1 -0
@@ -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:
net/sunrpc/auth_gss/svcauth_gss.c +1 -0
@@ -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);
net/sunrpc/svcauth_unix.c +2 -0
@@ -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 ;
Signature · 1 line
2.9.5
Re: [PATCH v5] kernel: make groups_sort calling a responsibility group_info allocators
Quoted 11 lines · Thiago Rafael Becker
In testing, we found that nfsd threads may call
set_groupsin parallel for the same entry cached in auth.unix.gid, racing in the call ofgroups_sort, corrupting the groups for that entry and leading to permission denials for the client.This patch:
- Makegroups_sortglobally visible.
- Move the call togroups_sortto the modifiers ofgroup_info
- Remove the call togroups_sortfromset_groupsSigned-off-by Thiago Rafael Becker <[email protected]>
Re: [PATCH v5] kernel: make groups_sort calling a responsibility group_info allocators
In reply to Thiago Rafael BeckerACK. (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.
Quoted 157 lines · Thiago Rafael Becker
In testing, we found that nfsd threads may call
set_groupsin parallel for the same entry cached in auth.unix.gid, racing in the call ofgroups_sort, corrupting the groups for that entry and leading to permission denials for the client.This patch:
- Makegroups_sortglobally visible.
- Move the call togroups_sortto the modifiers ofgroup_info
- Remove the call togroups_sortfromset_groupsSigned-off-by Thiago Rafael Becker <[email protected]>--- arch/s390/kernel/compat_linux.c | 1 + fs/nfsd/auth.c | 3 +++ include/linux/cred.h | 1 + kernel/groups.c | 5 +++-- kernel/uid16.c | 1 + net/sunrpc/auth_gss/gss_rpc_xdr.c | 1 + net/sunrpc/auth_gss/svcauth_gss.c | 1 + net/sunrpc/svcauth_unix.c | 2 ++ 8 files changed, 13 insertions(+), 2 deletions(-)arch/s390/kernel/compat_linux.c +1
-0@@ -263,6 +263,7 @@ COMPAT_SYSCALL_DEFINE2(s390_setgroups16, int, gidsetsize, u16 __user *, grouplisreturn retval; } +groups_sort (group_info); retval =set_current_groups (group_info);put_group_info (group_info);fs/nfsd/auth.c +3
-0@@ -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);include/linux/cred.h +1
-0@@ -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 kernel/groups.c +3
-2@@ -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);kernel/uid16.c +1
-0@@ -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);net/sunrpc/auth_gss/gss_rpc_xdr.c +1
-0@@ -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:net/sunrpc/auth_gss/svcauth_gss.c +1
-0@@ -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);net/sunrpc/svcauth_unix.c +2
-0@@ -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 ;Signature · 6 lines
2.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] kernel: make groups_sort calling a responsibility group_info allocators
In reply to Thiago Rafael BeckerQuoted 11 lines · Thiago Rafael Becker
In testing, we found that nfsd threads may call
set_groupsin parallel for the same entry cached in auth.unix.gid, racing in the call ofgroups_sort, corrupting the groups for that entry and leading to permission denials for the client.This patch:
- Makegroups_sortglobally visible.
- Move the call togroups_sortto the modifiers ofgroup_info
- Remove the call togroups_sortfromset_groupsSigned-off-by Thiago Rafael Becker <[email protected]>
Thanks. I like this better as a single patch.
I'd probably suggest "Cc: [email protected]". Less important bugfixes have gone to stable...
It might be nice to enhance
Documentation/security/credentials.rst
to explain that the groups list is always sorted, and must be sorted before set_groups() or set_current_groups() is called, but as the document doesn't mention any of this at all, this is purely an extra enhancement and doesn't need to be included in the same patch.
David: do you agree that this sort of content would be appropriate in that file (which you apparently authored). Would you like to update it, or shall I?
Thanks,
NeilBrown
Quoted 146 lines
--- 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(-)arch/s390/kernel/compat_linux.c +1
-0@@ -263,6 +263,7 @@ COMPAT_SYSCALL_DEFINE2(s390_setgroups16, int, gidsetsize, u16 __user *, grouplisreturn retval; } +groups_sort (group_info); retval =set_current_groups (group_info);put_group_info (group_info);fs/nfsd/auth.c +3
-0@@ -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);include/linux/cred.h +1
-0@@ -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 kernel/groups.c +3
-2@@ -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);kernel/uid16.c +1
-0@@ -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);net/sunrpc/auth_gss/gss_rpc_xdr.c +1
-0@@ -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:net/sunrpc/auth_gss/svcauth_gss.c +1
-0@@ -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);net/sunrpc/svcauth_unix.c +2
-0@@ -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 ;Signature · 6 lines
2.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] kernel: make groups_sort calling a responsibility group_info allocators
In reply to Thiago Rafael BeckerQuoted 11 lines · Thiago Rafael Becker
In testing, we found that nfsd threads may call
set_groupsin parallel for the same entry cached in auth.unix.gid, racing in the call ofgroups_sort, corrupting the groups for that entry and leading to permission denials for the client.This patch:
- Makegroups_sortglobally visible.
- Move the call togroups_sortto the modifiers ofgroup_info
- Remove the call togroups_sortfromset_groupsSigned-off-by Thiago Rafael Becker <[email protected]>
Thanks. I like this better as a single patch.
I'd probably suggest "Cc: [email protected]". Less important bugfixes have gone to stable...
It might be nice to enhance
Documentation/security/credentials.rst
to explain that the groups list is always sorted, and must be sorted before set_groups() or set_current_groups() is called, but as the document doesn't mention any of this at all, this is purely an extra enhancement and doesn't need to be included in the same patch.
David: do you agree that this sort of content would be appropriate in that file (which you apparently authored). Would you like to update it, or shall I?
Thanks,
NeilBrown
Quoted 146 lines
--- 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(-)arch/s390/kernel/compat_linux.c +1
-0@@ -263,6 +263,7 @@ COMPAT_SYSCALL_DEFINE2(s390_setgroups16, int, gidsetsize, u16 __user *, grouplisreturn retval; } +groups_sort (group_info); retval =set_current_groups (group_info);put_group_info (group_info);fs/nfsd/auth.c +3
-0@@ -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);include/linux/cred.h +1
-0@@ -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 kernel/groups.c +3
-2@@ -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);kernel/uid16.c +1
-0@@ -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);net/sunrpc/auth_gss/gss_rpc_xdr.c +1
-0@@ -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:net/sunrpc/auth_gss/svcauth_gss.c +1
-0@@ -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);net/sunrpc/svcauth_unix.c +2
-0@@ -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 ;Signature · 6 lines
2.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] kernel: make groups_sort calling a responsibility group_info allocators
Quoted 3 lines · NeilBrown
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 1/1 Documentation: security/credentials.rst: explain need to sort group_list
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.
--- Documentation/security/credentials.rst | 7 +++++++ 1 file changed, 7 insertions(+)
Documentation/security/credentials.rst +7 -0
@@ -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::
Signature · 1 line
2.14.0.rc0.dirty
Re: [PATCH] Documentation: security/credentials.rst: explain need to sort group_list
Documentation/security/credentials.rst checks and hooks done. Both the current and the proposed sets of credentials · line 456
Quoted 4 lines · NeilBrown
Documentation/security/credentials.rst +3
-0+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()``?
Quoted 10 lines
Documentation/security/credentials.rst +4
-0+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::Signature · 1 line
2.14.0.rc0.dirty
Jonathan Corbet
maintainerRe: [PATCH] Documentation: security/credentials.rst: explain need to sort group_list
Quoted 6 lines · On Tue, 2 Jan 2018 13:04:31 -0800 Matthew Wilcox <[email protected]> wrote:
Documentation/security/credentials.rst checks and hooks done. Both the current and the proposed sets of credentials · line 456
Quoted 3 lines
Documentation/security/credentials.rst +3
-0+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 beFor a .rst file, shouldn't we be using :c:func:
groups_sortinstead 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
Quoted 10 lines · Jonathan Corbet
Quoted 6 lines · On Tue, 2 Jan 2018 13:04:31 -0800 Matthew Wilcox <[email protected]> wrote:
Documentation/security/credentials.rst checks and hooks done. Both the current and the proposed sets of credentials · line 456
Quoted 3 lines
Documentation/security/credentials.rst +3
-0+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 beFor a .rst file, shouldn't we be using :c:func:
groups_sortinstead of ``groups_sort()``?
Quoted 6 lines
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 :-)
Randy Dunlap
reviewerRe: [PATCH] Documentation: security/credentials.rst: explain need to sort group_list
Quoted 7 lines · Matthew Wilcox
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.rstwhich 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.
Quoted 3 lines
I think the right path forward is to have
kernel-api.rstbe 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.
Quoted 5 lines
Then we need little stories about how all the functions in a subsystem fit together. For example, we can create a
list.rstwhich explains how this is a doubly-linked list that you use by embedding alist_headinto your data structure, and has O(1) insertion/deletion, etc, etc. Then we would move all thelist.hkernel-doc fromkernel-api.rstintolist.rst.
Signature · 1 line
~Randy
Jonathan Corbet
maintainerRe: [PATCH] Documentation: security/credentials.rst: explain need to sort group_list
In reply to Matthew WilcoxQuoted 6 lines · On Sat, 6 Jan 2018 12:20:13 -0800 Matthew Wilcox <[email protected]> wrote:
I've been thinking about all the kernel-doc we have that's completely unincorporated. I've also been thinking about
core-api/kernel-api.rstwhich 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.
Quoted 3 lines
I think the right path forward is to have
kernel-api.rstbe 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.
Quoted 8 lines
Then we need little stories about how all the functions in a subsystem fit together. For example, we can create a
list.rstwhich explains how this is a doubly-linked list that you use by embedding alist_headinto your data structure, and has O(1) insertion/deletion, etc, etc. Then we would move all thelist.hkernel-doc fromkernel-api.rstintolist.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
In reply to Jonathan CorbetQuoted 6 lines · Jonathan Corbet
There is value in using the c:func syntax, as it will generate cross-references to the kerneldoc comments for those functions. In this case, it would appear that these comments exist, but nobody has pulled them into the docs yet. I took the liberty of adding :c:func: references to this patch on its way into docs-next so that things will Just Work on that happy day when somebody adds the function documentation.
Is this the sort of thing that might result in that happy day? I didn't spend tooo much time on it, in case including the kernel-doc in credentials.rst like this was the wrong direction.
Thanks,
NeilBrown
------------------------------8<-------------------------
From: NeilBrown <[email protected]>
Subject: [PATCH] Documentation: include kernel-doc in credentials.rst
- kernel-doc from include/linux/cred.h, kernel/cred.c, and kernel/group.c is included in credentials.rst.
- Various function references in credentials.rst are changed to link to this documentation.
- Various minor improvements to the documentation.
--- Documentation/security/credentials.rst | 55 ++++++++++++++++++---------------- kernel/groups.c | 19 +++++++++++- 2 files changed, 47 insertions(+), 27 deletions(-)
Documentation/security/credentials.rst +29 -26
@@ -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:
kernel/groups.c +18 -1
@@ -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) {
Signature · 1 line
2.14.0.rc0.dirty
Jonathan Corbet
maintainerRe: [PATCH] Documentation: security/credentials.rst: explain need to sort group_list
Quoted 7 lines · On Mon, 08 Jan 2018 10:39:14 +1100 NeilBrown <[email protected]> wrote:
Quoted 6 lines
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.
Quoted 5 lines
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.rstlike this was the wrong direction.From a very quick look, yes. I'll take a closer look soon. Life has been
a bit ... busy ... around here...
Thanks,
jon