Re: [PATCH v3] add group restriction bitmap

[stsp]

Вторник, 1 октября 2024 г получено от Oleg Nesterov:
> 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.

Ah, I see now!
Sorry.
I didn't expect to move that check
that far, so I thought you mean just
make groups_search() 0-based and
-1 if not found... even if you said
otherwise.

Yes, -1 when found but restricted
is the very interesting simplification!
Will send an update.
Thanks.

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