splat cred:groupmember: Fix false positives

Due to certain assumptions made in the the cred:groupmember test it
could result in false positives when run on specific distributions.
This was solely a bug in the test case and not in the groupmember()
function which the test case was validating.

To prevent future false positives the test case has been rewritten
to be both more rigerous and to make fewer assumptions about the
system.

Minor style cleanup was done to cr_groups_search() and groupmember()
functions.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>

module/spl/spl-cred.c

@@ -44,7 +44,8 @@ cr_groups_search(const struct group_info *group_info, kgid_t grp)
 cr_groups_search(const struct group_info *group_info, gid_t grp)
 #endif
 {
-	unsigned int left, right;
+	unsigned int left, right, mid;
+	int cmp;
 
 	if (!group_info)
 		return 0;
@@ -52,8 +53,10 @@ cr_groups_search(const struct group_info *group_info, gid_t grp)
 	left = 0;
 	right = group_info->ngroups;
 	while (left < right) {
-		unsigned int mid = (left+right)/2;
+		mid = (left + right) / 2;
-		int cmp = KGID_TO_SGID(grp) - KGID_TO_SGID(GROUP_AT(group_info, mid));
+		cmp = KGID_TO_SGID(grp) -
+		    KGID_TO_SGID(GROUP_AT(group_info, mid));
+
 		if (cmp > 0)
 			left = mid + 1;
 		else if (cmp < 0)
@@ -120,7 +123,7 @@ crgetgroups(const cred_t *cr)
 	return gids;
 }
 
-/* Check if the passed gid is available is in supplied credential. */
+/* Check if the passed gid is available in supplied credential. */
 int
 groupmember(gid_t gid, const cred_t *cr)
 {
@@ -128,7 +131,7 @@ groupmember(gid_t gid, const cred_t *cr)
 	int rc;
 
 	gi = get_group_info(cr->group_info);
-	rc = cr_groups_search(cr->group_info, SGID_TO_KGID(gid));
+	rc = cr_groups_search(gi, SGID_TO_KGID(gid));
 	put_group_info(gi);
 
 	return rc;

module/splat/splat-cred.c

@@ -25,6 +25,7 @@
 \*****************************************************************************/
 
 #include <sys/cred.h>
+#include <sys/random.h>
 #include "splat-internal.h"
 
 #define SPLAT_CRED_NAME			"cred"
@@ -166,42 +167,89 @@ splat_cred_test2(struct file *file, void *arg)
 } /* splat_cred_test2() */
 
 /*
- * On most/all systems it can be expected that a task with root
+ * Verify the groupmember() works correctly by constructing an interesting
- * permissions also is a member of the root group,  Since the
+ * CRED() and checking that the expected gids are part of it.
- * test suite is always run as root we check first that CRED() is
- * a member of the root group, and secondly that it is not a member
- * of our fake group.  This test will break is someone happens to
- * create group number NGROUPS_MAX-1 and then added root to it.
  */
 static int
 splat_cred_test3(struct file *file, void *arg)
 {
-	gid_t root_gid, fake_gid;
+	gid_t known_gid, missing_gid, tmp_gid;
-	int rc;
+	unsigned char rnd;
+	struct group_info *gi;
+	int i, rc;
 
-	root_gid = 0;
+	get_random_bytes((void *)&rnd, 1);
-	fake_gid = NGROUPS_MAX-1;
+	known_gid = (rnd > 0) ? rnd : 1;
+	missing_gid = 0;
 
-	rc = groupmember(root_gid, CRED());
+	/*
-	if (!rc) {
+	 * Create an interesting known set of gids for test purposes. The
-		splat_vprint(file, SPLAT_CRED_TEST3_NAME,
+	 * gids are pseudo randomly selected are will be in the range of
-			     "Failed root git %d expected to be member "
+	 * 1:(NGROUPS_MAX-1).  Gid 0 is explicitly avoided so we can reliably
-			     "of CRED() groups: %d\n", root_gid, rc);
+	 * test for its absence in the test cases.
-		return -EIDRM;
+	 */
+	gi = groups_alloc(NGROUPS_SMALL);
+	if (gi == NULL) {
+		splat_vprint(file, SPLAT_CRED_TEST3_NAME, "Failed create "
+		    "group_info for known gids: %d\n", -ENOMEM);
+		rc = -ENOMEM;
+		goto show_groups;
 	}
 
-	rc = groupmember(fake_gid, CRED());
+	for (i = 0, tmp_gid = known_gid; i < NGROUPS_SMALL; i++) {
+		splat_vprint(file, SPLAT_CRED_TEST3_NAME, "Adding gid %d "
+		    "to current CRED() (%d/%d)\n", tmp_gid, i, gi->ngroups);
+#ifdef HAVE_KUIDGID_T
+		GROUP_AT(gi, i) = make_kgid(current_user_ns(), tmp_gid);
+#else
+		GROUP_AT(gi, i) = tmp_gid;
+#endif /* HAVE_KUIDGID_T */
+		tmp_gid = ((tmp_gid * 17) % (NGROUPS_MAX - 1)) + 1;
+	}
+
+	/* Set the new groups in the CRED() and release our reference. */
+	rc = set_current_groups(gi);
+	put_group_info(gi);
+
 	if (rc) {
-		splat_vprint(file, SPLAT_CRED_TEST3_NAME,
+		splat_vprint(file, SPLAT_CRED_TEST3_NAME, "Failed to add "
-			     "Failed fake git %d expected not to be member "
+		    "gid %d to current group: %d\n", known_gid, rc);
-			     "of CRED() groups: %d\n", fake_gid, rc);
+		goto show_groups;
-		return -EIDRM;
 	}
 
-	splat_vprint(file, SPLAT_CRED_TEST3_NAME, "Success root gid "
+	/* Verify groupmember() finds the known_gid in the CRED() */
-		     "is a member of the expected groups: %d\n", rc);
+	rc = groupmember(known_gid, CRED());
+	if (!rc) {
+		splat_vprint(file, SPLAT_CRED_TEST3_NAME, "Failed to find "
+		    "known gid %d in CRED()'s groups.\n", known_gid);
+		rc = -EIDRM;
+		goto show_groups;
+	}
 
-	return rc;
+	/* Verify groupmember() does NOT finds the missing gid in the CRED() */
+	rc = groupmember(missing_gid, CRED());
+	if (rc) {
+		splat_vprint(file, SPLAT_CRED_TEST3_NAME, "Failed missing "
+		    "gid %d was found in CRED()'s groups.\n", missing_gid);
+		rc = -EIDRM;
+		goto show_groups;
+	}
+
+	splat_vprint(file, SPLAT_CRED_TEST3_NAME, "Success groupmember() "
+	    "correctly detects expected gids in CRED(): %d\n", rc);
+
+show_groups:
+	if (rc) {
+		int i, grps = crgetngroups(CRED());
+
+		splat_vprint(file, SPLAT_CRED_TEST3_NAME, "%d groups: ", grps);
+		for (i = 0; i < grps; i++)
+			splat_print(file, "%d ", crgetgroups(CRED())[i]);
+		splat_print(file, "%s", "\n");
+	}
+
+
+	return (rc);
 } /* splat_cred_test3() */
 
 splat_subsystem_t *