LU-17497 obdclass: check upcall incorrect values

Identity upcall is set via lctl set_param mdt.*.identity_upcall=xxx,
and rsi upcall is set via lctl set_param sptlrpc.gss.rsi_upcall=xxx.
Possible values are a valid path to an executable, and also INTERNAL
to enable support of supplementary groups from client, or NONE to
disable identity upcall.
Add an upcall cache function that checks the user provided string, to
make sure we do not store an invalid value. And print a message to
stdout to explain the accepted values.

Lustre-change: https://review.whamcloud.com/53878
Lustre-commit: 2153e86541884ef7a5c1697a5d00daf6fa6461a4

Add sanity-sec test_69 to exercise this.

Signed-off-by: Sebastien Buisson <sbuisson@ddn.com>
Change-Id: Iaf59e72aa1612f5579db175d8999dcf0053308ed
Reviewed-on: https://review.whamcloud.com/c/ex/lustre-release/+/53879
Reviewed-by: Andreas Dilger <adilger@whamcloud.com>
Reviewed-by: Lai Siyao <lai.siyao@whamcloud.com>
Tested-by: jenkins <devops@whamcloud.com>
Tested-by: Maloo <maloo@whamcloud.com>

diff --git a/lustre/include/upcall_cache.h b/lustre/include/upcall_cache.h
index 3567f84..010c6fc 100644 (file)
--- a/lustre/include/upcall_cache.h
+++ b/lustre/include/upcall_cache.h
@@ -158,6 +158,8 @@ struct upcall_cache {
        struct upcall_cache_ops *uc_ops;
 };
 
+int upcall_cache_set_upcall(struct upcall_cache *cache, const char *buffer,
+                           size_t count, bool path_only);
 struct upcall_cache_entry *upcall_cache_get_entry(struct upcall_cache *cache,
                                                  __u64 key, void *args);
 void upcall_cache_get_entry_raw(struct upcall_cache_entry *entry);

diff --git a/lustre/mdt/mdt_lproc.c b/lustre/mdt/mdt_lproc.c
index 3885e5f..af0e875 100644 (file)
--- a/lustre/mdt/mdt_lproc.c
+++ b/lustre/mdt/mdt_lproc.c
@@ -296,17 +296,16 @@ static ssize_t identity_upcall_store(struct kobject *kobj,
                                              obd_kset.kobj);
        struct mdt_device *mdt = mdt_dev(obd->obd_lu_dev);
        struct upcall_cache *hash = mdt->mdt_identity_cache;
+       int rc;
 
-       if (count >= UC_CACHE_UPCALL_MAXPATH) {
-               CERROR("%s: identity upcall too long\n", mdt_obd_name(mdt));
-               return -EINVAL;
+       rc = upcall_cache_set_upcall(hash, buffer, count, false);
+       if (rc) {
+               CERROR("%s: incorrect identity upcall %.*s. Valid values for mdt.%s.identity_upcall are NONE or INTERNAL, or an executable pathname: rc = %d\n",
+                      mdt_obd_name(mdt), (int)count, buffer,
+                      mdt_obd_name(mdt), rc);
+               return rc;
        }
 
-       /* Remove any extraneous bits from the upcall (e.g. linefeeds) */
-       down_write(&hash->uc_upcall_rwsem);
-       sscanf(buffer, "%s", hash->uc_upcall);
-       up_write(&hash->uc_upcall_rwsem);
-
        if (strcmp(hash->uc_name, mdt_obd_name(mdt)) != 0)
                CWARN("%s: write to upcall name %s\n",
                      mdt_obd_name(mdt), hash->uc_upcall);
@@ -317,7 +316,7 @@ static ssize_t identity_upcall_store(struct kobject *kobj,
 
        CDEBUG(D_CONFIG, "%s: identity upcall set to %s\n", mdt_obd_name(mdt),
               hash->uc_upcall);
-       RETURN(count);
+       return count;
 }
 LUSTRE_RW_ATTR(identity_upcall);
 

diff --git a/lustre/obdclass/upcall_cache.c b/lustre/obdclass/upcall_cache.c
index e7aea75..5b5db34 100644 (file)
--- a/lustre/obdclass/upcall_cache.c
+++ b/lustre/obdclass/upcall_cache.c
@@ -126,6 +126,51 @@ static int check_unlink_entry(struct upcall_cache *cache,
        return 1;
 }
 
+int upcall_cache_set_upcall(struct upcall_cache *cache, const char *buffer,
+                           size_t count, bool path_only)
+{
+       char *upcall;
+
+       if (count >= UC_CACHE_UPCALL_MAXPATH)
+               return -E2BIG;
+
+       OBD_ALLOC(upcall, count + 1);
+       if (upcall == NULL)
+               return -ENOMEM;
+
+       /* Remove any extraneous bits from the upcall (e.g. linefeeds) */
+       if (sscanf(buffer, "%s", upcall) != 1)
+               goto invalid;
+
+       if (upcall[0] == '/')
+               goto valid;
+
+       if (path_only)
+               goto invalid;
+
+       if (strcasecmp(upcall, "NONE") == 0) {
+               snprintf(upcall, count + 1, "NONE");
+               goto valid;
+       }
+       if (strcasecmp(upcall, "INTERNAL") == 0) {
+               snprintf(upcall, count + 1, "INTERNAL");
+               goto valid;
+       }
+
+invalid:
+       OBD_FREE(upcall, count + 1);
+       return -EINVAL;
+
+valid:
+       down_write(&cache->uc_upcall_rwsem);
+       strcpy(cache->uc_upcall, upcall);
+       up_write(&cache->uc_upcall_rwsem);
+
+       OBD_FREE(upcall, count + 1);
+       return 0;
+}
+EXPORT_SYMBOL(upcall_cache_set_upcall);
+
 static inline int refresh_entry(struct upcall_cache *cache,
                                struct upcall_cache_entry *entry, __u32 fsgid)
 {

diff --git a/lustre/ptlrpc/gss/lproc_gss.c b/lustre/ptlrpc/gss/lproc_gss.c
index 245838a..e998498 100644 (file)
--- a/lustre/ptlrpc/gss/lproc_gss.c
+++ b/lustre/ptlrpc/gss/lproc_gss.c
@@ -199,11 +199,6 @@ static ssize_t rsi_upcall_seq_write(struct file *file,
        char *kbuf = NULL;
        int rc;
 
-       if (count >= UC_CACHE_UPCALL_MAXPATH) {
-               CERROR("%s: rsi upcall too long\n", rsicache->uc_name);
-               return -EINVAL;
-       }
-
        OBD_ALLOC(kbuf, count + 1);
        if (kbuf == NULL)
                return -ENOMEM;
@@ -213,22 +208,20 @@ static ssize_t rsi_upcall_seq_write(struct file *file,
 
        kbuf[count] = '\0';
 
-       /* Remove any extraneous bits from the upcall (e.g. linefeeds) */
-       down_write(&rsicache->uc_upcall_rwsem);
-       rc = sscanf(kbuf, "%s", rsicache->uc_upcall);
-       up_write(&rsicache->uc_upcall_rwsem);
-
-       if (rc != 1) {
-               CERROR("%s: invalid rsi upcall provided\n", rsicache->uc_name);
-               GOTO(out, rc = -EINVAL);
+       rc = upcall_cache_set_upcall(rsicache, kbuf, count, true);
+       if (rc) {
+               CERROR("%s: incorrect rsi upcall %.*s. Valid value for sptlrpc.gss.rsi_upcall is an executable pathname: rc = %d\n",
+                      rsicache->uc_name, (int)count, buffer, rc);
+               GOTO(out, rc);
        }
 
        CDEBUG(D_CONFIG, "%s: rsi upcall set to %s\n", rsicache->uc_name,
               rsicache->uc_upcall);
+       rc = count;
 
 out:
        OBD_FREE(kbuf, count + 1);
-       return count;
+       return rc;
 }
 LPROC_SEQ_FOPS(rsi_upcall);
 

diff --git a/lustre/tests/sanity-sec.sh b/lustre/tests/sanity-sec.sh
index cae6c61..505e95e 100755 (executable)
--- a/lustre/tests/sanity-sec.sh
+++ b/lustre/tests/sanity-sec.sh
@@ -6344,6 +6344,46 @@ test_68() {
 }
 run_test 68 "all config logs are processed"
 
+test_69() {
+       local mdt="$(mdtname_from_index 0 $MOUNT)"
+       local param
+       local orig
+
+       param="mdt.$mdt.identity_upcall"
+       orig="$(do_facet mds1 "$LCTL get_param -n $param")"
+       stack_trap "do_facet mds1 $LCTL set_param $param=$orig" EXIT
+
+       # identity_upcall accepts a path to an executable,
+       # or NONE (case insensitive) or INTERNAL (case insensitive)
+       do_facet mds1 $LCTL set_param $param=/path/to/prog ||
+               error "set_param $param=/path/to/prog failed"
+       do_facet mds1 $LCTL set_param $param=prog &&
+               error "set_param $param=prog should failed"
+       do_facet mds1 $LCTL set_param $param=NONE ||
+               error "set_param $param=NONE failed"
+       do_facet mds1 $LCTL set_param $param=none ||
+               error "set_param $param=none failed"
+       do_facet mds1 $LCTL set_param $param=INTERNAL ||
+               error "set_param $param=INTERNAL failed"
+       do_facet mds1 $LCTL set_param $param=internal ||
+               error "set_param $param=internal failed"
+
+       if $GSS; then
+               param="sptlrpc.gss.rsi_upcall"
+               orig="$(do_facet mds1 "$LCTL get_param -n $param")"
+               stack_trap "do_facet mds1 $LCTL set_param $param=$orig" EXIT
+
+               # rsi_upcall only accepts a path to an executable
+               do_facet mds1 $LCTL set_param $param=prog &&
+                       error "set_param $param=prog should failed"
+               do_facet mds1 $LCTL set_param $param=NONE &&
+                       error "set_param $param=NONE should fail"
+               do_facet mds1 $LCTL set_param $param=/path/to/prog ||
+                       error "set_param $param=/path/to/prog failed"
+       fi
+}
+run_test 69 "check upcall incorrect values"
+
 log "cleanup: ======================================================"
 
 sec_unsetup() {