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: <20241001150258.GD23907@redhat.com>
Date: Tue, 1 Oct 2024 17:02:58 +0200
From: Oleg Nesterov <oleg@...hat.com>
To: stsp <stsp2@...dex.ru>
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

We can't understand each other. I guess I missed something...

On 10/01, stsp wrote:
>
> 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.

And it shouldn't. It returns the result of groups_search() if this
grp is supplementary or "not found".

> acl_permission_check() does, but
> in your example it doesn't as well.

But it does??? see below...

> I think you mean to move the
> restrict_bitmap check upwards to
> in_group_p()?

No, I meant to move the restrict_bitmap check to groups_search(), see the patch
above.

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

Why? -1 means that the grp is supplementary and restricted.

> There are also the callers of groups_search()
> in kernel/auditsc.c and they should
> be updated.

Why? I don't think so. audit_filter_rules() uses the result of groups_search()
as a boolean.

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

No, I didn't remove the check, this code relies on the change in
groups_search(). Note the "xxx > 0" check.

I must have missed something :/

Oleg.