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>

diff --git a/module/spl/spl-cred.c b/module/spl/spl-cred.c
index 0ed65725eb53a9dcab10e77dc47b0bf49c6051a0..602bd74e883d24720af58f422a370e8500f569e7 100644 (file)
--- a/module/spl/spl-cred.c
+++ b/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;
-               int cmp = KGID_TO_SGID(grp) - KGID_TO_SGID(GROUP_AT(group_info, mid));
+               mid = (left + right) / 2;
+               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;

diff --git a/module/splat/splat-cred.c b/module/splat/splat-cred.c
index 47dfa02f6b8cd51620b0e444745bf5249bccf5dd..fadf9bca0c7c724b176576d38572b31d757adaa9 100644 (file)
--- a/module/splat/splat-cred.c
+++ b/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
- * permissions also is a member of the root group,  Since the
- * 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.
+ * Verify the groupmember() works correctly by constructing an interesting
+ * CRED() and checking that the expected gids are part of it.
  */
 static int
 splat_cred_test3(struct file *file, void *arg)
 {
-       gid_t root_gid, fake_gid;
-       int rc;
+       gid_t known_gid, missing_gid, tmp_gid;
+       unsigned char rnd;
+       struct group_info *gi;
+       int i, rc;
+
+       get_random_bytes((void *)&rnd, 1);
+       known_gid = (rnd > 0) ? rnd : 1;
+       missing_gid = 0;
+
+       /*
+        * Create an interesting known set of gids for test purposes. The
+        * gids are pseudo randomly selected are will be in the range of
+        * 1:(NGROUPS_MAX-1).  Gid 0 is explicitly avoided so we can reliably
+        * test for its absence in the test cases.
+        */
+       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;
+       }
+
+       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;
+       }
 
-       root_gid = 0;
-       fake_gid = NGROUPS_MAX-1;
+       /* Set the new groups in the CRED() and release our reference. */
+       rc = set_current_groups(gi);
+       put_group_info(gi);
 
-       rc = groupmember(root_gid, CRED());
+       if (rc) {
+               splat_vprint(file, SPLAT_CRED_TEST3_NAME, "Failed to add "
+                   "gid %d to current group: %d\n", known_gid, rc);
+               goto show_groups;
+       }
+
+       /* Verify groupmember() finds the known_gid in the CRED() */
+       rc = groupmember(known_gid, CRED());
        if (!rc) {
-               splat_vprint(file, SPLAT_CRED_TEST3_NAME,
-                            "Failed root git %d expected to be member "
-                            "of CRED() groups: %d\n", root_gid, rc);
-               return -EIDRM;
+               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;
        }
 
-       rc = groupmember(fake_gid, CRED());
+       /* 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 fake git %d expected not to be member "
-                            "of CRED() groups: %d\n", fake_gid, rc);
-               return -EIDRM;
+               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");
        }
 
-       splat_vprint(file, SPLAT_CRED_TEST3_NAME, "Success root gid "
-                    "is a member of the expected groups: %d\n", rc);
 
-       return rc;
+       return (rc);
 } /* splat_cred_test3() */
 
 splat_subsystem_t *