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: <02ae38f6-698c-496f-9e96-1376ef9f1332@yandex.ru>
Date: Tue, 1 Oct 2024 14:37:18 +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 14:15, Oleg Nesterov пишет:
> I can't comment the intent, just some nits about implementation.
>
> On 09/30, Stas Sergeev wrote:
>>   struct group_info {
>>   	refcount_t	usage;
>> +	unsigned int	restrict_bitmap;
> Why not unsigned long?

My impl claims to support 31bit only.
Maybe use "unsigned long long" to
get like 63? Isn't "unsigned long"
arch-dependent? What would be the
benefit of an arch-dependent bitmap?

>>   int groups_search(const struct group_info *group_info, kgid_t grp)
>>   {
>>   	unsigned int left, right;
>> @@ -105,7 +108,7 @@ int groups_search(const struct group_info *group_info, kgid_t grp)
>>   		else if (gid_lt(grp, group_info->gid[mid]))
>>   			right = mid;
>>   		else
>> -			return 1;
>> +			return mid + 1;
> 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()
and so on. Also I am not sure if it really
helps: in_group_p() has also gid, so
if in_group_p() returns -1 for not found
and 0 for gid, then I still need to
increment the retval of groups_search(),
just as I do now.
So unless I am missing your intention,
this isn't really helping.