linux-kernel - Re: [PATCH v3] add group restriction bitmap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <62362149-c550-490f-bd7a-0fd7a5cd22bc@yandex.ru>
Date: Tue, 1 Oct 2024 16:29:37 +0300
From: stsp <stsp2@...dex.ru>
To: Oleg Nesterov <oleg@...hat.com>
Cc: linux-kernel@...r.kernel.org, Alexander Viro <viro@...iv.linux.org.uk>,
 Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
 Jens Axboe <axboe@...nel.dk>, Andrew Morton <akpm@...ux-foundation.org>,
 Catalin Marinas <catalin.marinas@....com>,
 Florent Revest <revest@...omium.org>, Kees Cook <kees@...nel.org>,
 Palmer Dabbelt <palmer@...osinc.com>, Charlie Jenkins
 <charlie@...osinc.com>, Benjamin Gray <bgray@...ux.ibm.com>,
 Helge Deller <deller@....de>, Zev Weiss <zev@...ilderbeest.net>,
 Samuel Holland <samuel.holland@...ive.com>, linux-fsdevel@...r.kernel.org,
 Eric Biederman <ebiederm@...ssion.com>, Andy Lutomirski <luto@...nel.org>,
 Josh Triplett <josh@...htriplett.org>
Subject: Re: [PATCH v3] add group restriction bitmap

01.10.2024 16:02, Oleg Nesterov пишет:
> On 10/01, stsp wrote:
>> 01.10.2024 14:15, Oleg Nesterov пишет:
>>> Suppose we change groups_search()
>>>
>>> 	--- a/kernel/groups.c
>>> 	+++ b/kernel/groups.c
>>> 	@@ -104,8 +104,11 @@ int groups_search(const struct group_info *group_info, kgid_t grp)
>>> 				left = mid + 1;
>>> 			else if (gid_lt(grp, group_info->gid[mid]))
>>> 				right = mid;
>>> 	-		else
>>> 	-			return 1;
>>> 	+		else {
>>> 	+			bool r = mid < BITS_PER_LONG &&
>>> 	+				 test_bit(mid, &group_info->restrict_bitmap);
>>> 	+			return r ? -1 : 1;
>>> 	+		}
>>> 		}
>>> 		return 0;
>>> 	 }
>>>
>>> so that it returns, say, -1 if the found grp is restricted.
>>>
>>> Then everything else can be greatly simplified, afaics...
>> This will mean updating all callers
>> of groups_search(), in_group_p(),
>> in_egroup_p(), vfsxx_in_group_p()
> Why? I think with this change you do not need to touch in_group_p/etc at all.
>
>> if in_group_p() returns -1 for not found
>> and 0 for gid,
> With the the change above in_group_p() returns 0 if not found, !0 otherwise.
> It returns -1 if grp != cred->fsgid and the found grp is restricted.

in_group_p() doesn't check if the
group is restricted or not.
acl_permission_check() does, but
in your example it doesn't as well.
I think you mean to move the
restrict_bitmap check upwards to
in_group_p()?
Anyway, suppose you don't mean that.
In this case:
1. in_group_p() and in_egroup_p()
   should be changed:
-  int retval = 1;
+ int retval = -1;

But their callers should not.
There are also the callers of groups_search()
in kernel/auditsc.c and they should
be updated. But they are few.
Just to be clear, is this what you suggest?

> So acl_permission_check() can simply do
>
> 	if (mask & (mode ^ (mode >> 3))) {
> 		vfsgid_t vfsgid = i_gid_into_vfsgid(idmap, inode);
> 		int xxx = vfsgid_in_group_p(vfsgid);
>
> 		if (xxx) {
> 			if (mask & ~(mode >> 3))
> 				return -EACCES;
> 			if (xxx > 0)
> 				return 0;
> 			/* If we hit restrict_bitmap, then check Others. */
> 		}
> 	}

Well, in my impl it should check
the bitmap right here, but you removed
that. Maybe you want the check elsewhere?