LU-17000 mgs: check lcfg parameters

Check lcfg parameters in mgs_iocontrol() and mgs_iocontrol_pool().
Make the callers more consistent with buffer access and error checks.
REC_DATA_LEN() should be used instead of "rec_len" for lcfg size,
since "rec_len" is the full llog record size including hdr/tail.

CoverityID: 397130 ("Passing tainted expression lcfg->lcfg_buflens")
CoverityID: 397132 ("Passing tainted expression lcfg->lcfg_buflens")
CoverityID: 425252 ("Untrusted value as argument")

Signed-off-by: Andreas Dilger <adilger@whamcloud.com>
Change-Id: I9a7052a24d77124582df61340e520c1db6433892
Reviewed-on: https://review.whamcloud.com/c/fs/lustre-release/+/58360
Tested-by: jenkins <devops@whamcloud.com>
Tested-by: Maloo <maloo@whamcloud.com>
Reviewed-by: Arshad Hussain <arshad.hussain@aeoncomputing.com>
Reviewed-by: Sebastien Buisson <sbuisson@ddn.com>
Reviewed-by: Oleg Drokin <green@whamcloud.com>

diff --git a/lustre/include/uapi/linux/lustre/lustre_cfg.h b/lustre/include/uapi/linux/lustre/lustre_cfg.h
index de3dc29..1a2ef77 100644 (file)
--- a/lustre/include/uapi/linux/lustre/lustre_cfg.h
+++ b/lustre/include/uapi/linux/lustre/lustre_cfg.h
@@ -321,11 +321,11 @@ static inline int lustre_cfg_sanity_check(void *buf, __kernel_size_t len)
                return -EINVAL;
 
        /* check that the buflens are valid */
-       if (len < LCFG_HDR_SIZE(lcfg->lcfg_bufcount))
+       if (LCFG_HDR_SIZE(lcfg->lcfg_bufcount) > len)
                return -EINVAL;
 
        /* make sure all the pointers point inside the data */
-       if (len < lustre_cfg_len(lcfg->lcfg_bufcount, lcfg->lcfg_buflens))
+       if (lustre_cfg_len(lcfg->lcfg_bufcount, lcfg->lcfg_buflens) > len)
                return -EINVAL;
 
        return 0;

diff --git a/lustre/mgs/mgs_handler.c b/lustre/mgs/mgs_handler.c
index a268de6..d4b945c 100644 (file)
--- a/lustre/mgs/mgs_handler.c
+++ b/lustre/mgs/mgs_handler.c
@@ -801,8 +801,12 @@ static int mgs_iocontrol_pool(const struct lu_env *env,
        if (copy_from_user(lcfg, data->ioc_pbuf1, data->ioc_plen1))
                GOTO(out_lcfg, rc = -EFAULT);
 
+       rc = lustre_cfg_sanity_check(lcfg, data->ioc_plen1);
+       if (rc)
+               GOTO(out_lcfg, rc);
+
        if (lcfg->lcfg_bufcount < 2)
-               GOTO(out_lcfg, rc = -EFAULT);
+               GOTO(out_lcfg, rc = -EINVAL);
 
        /* first arg is always <fsname>.<poolname> */
        rc = mgs_extract_fs_pool(lustre_cfg_string(lcfg, 1), mgi->mgi_fsname,
@@ -892,9 +896,13 @@ static int mgs_iocontrol(unsigned int cmd, struct obd_export *exp, int len,
                if (copy_from_user(lcfg, data->ioc_pbuf1, data->ioc_plen1))
                        GOTO(out_free, rc = -EFAULT);
 
-               if (lcfg->lcfg_bufcount < 1)
+               rc = lustre_cfg_sanity_check(lcfg, data->ioc_plen1);
+               if (rc)
                        GOTO(out_free, rc);
 
+               if (lcfg->lcfg_bufcount < 1)
+                       GOTO(out_free, rc = -EINVAL);
+
                rc = mgs_set_param(&env, mgs, lcfg);
                if (rc)
                        CERROR("%s: setparam err: rc = %d\n",

diff --git a/lustre/mgs/mgs_llog.c b/lustre/mgs/mgs_llog.c
index fbe800c..4c542f5 100644 (file)
--- a/lustre/mgs/mgs_llog.c
+++ b/lustre/mgs/mgs_llog.c
@@ -232,9 +232,7 @@ static int mgs_fsdb_handler(const struct lu_env *env, struct llog_handle *llh,
 {
        struct mgs_fsdb_handler_data *d = data;
        struct fs_db *fsdb = d->fsdb;
-       int cfg_len = rec->lrh_len;
-       char *cfg_buf = (char *)(rec + 1);
-       struct lustre_cfg *lcfg;
+       struct lustre_cfg *lcfg = REC_DATA(rec);
        u32 index;
        int rc = 0;
 
@@ -244,14 +242,12 @@ static int mgs_fsdb_handler(const struct lu_env *env, struct llog_handle *llh,
                RETURN(-EINVAL);
        }
 
-       rc = lustre_cfg_sanity_check(cfg_buf, cfg_len);
+       rc = lustre_cfg_sanity_check(lcfg, REC_DATA_LEN(rec));
        if (rc) {
                CERROR("Insane cfg\n");
                RETURN(rc);
        }
 
-       lcfg = (struct lustre_cfg *)cfg_buf;
-
        CDEBUG(D_INFO, "cmd %x %s %s\n", lcfg->lcfg_command,
               lustre_cfg_string(lcfg, 0), lustre_cfg_string(lcfg, 1));
 
@@ -811,7 +807,6 @@ static int mgs_search_pool_cb(const struct lu_env *env,
 {
        struct mgs_search_pool_data *d = data;
        struct lustre_cfg *lcfg = REC_DATA(rec);
-       int cfg_len = REC_DATA_LEN(rec);
        char *fsname;
        char *poolname;
        char *ostname = NULL;
@@ -823,7 +818,7 @@ static int mgs_search_pool_cb(const struct lu_env *env,
                RETURN(-EINVAL);
        }
 
-       rc = lustre_cfg_sanity_check(lcfg, cfg_len);
+       rc = lustre_cfg_sanity_check(lcfg, REC_DATA_LEN(rec));
        if (rc) {
                CDEBUG(D_ERROR, "Insane cfg\n");
                RETURN(rc);
@@ -956,7 +951,6 @@ static int mgs_modify_handler(const struct lu_env *env,
        struct mgs_modify_lookup *mml = data;
        struct cfg_marker *marker;
        struct lustre_cfg *lcfg = REC_DATA(rec);
-       int cfg_len = REC_DATA_LEN(rec);
        int rc;
 
        ENTRY;
@@ -965,7 +959,7 @@ static int mgs_modify_handler(const struct lu_env *env,
                RETURN(-EINVAL);
        }
 
-       rc = lustre_cfg_sanity_check(lcfg, cfg_len);
+       rc = lustre_cfg_sanity_check(lcfg, REC_DATA_LEN(rec));
        if (rc) {
                CERROR("Insane cfg\n");
                RETURN(rc);
@@ -1381,13 +1375,11 @@ static int mgs_replace_nids_handler(const struct lu_env *env,
                                    struct llog_rec_hdr *rec,
                                    void *data)
 {
-       struct mgs_replace_data *mrd;
+       struct mgs_replace_data *mrd = data;
        struct lustre_cfg *lcfg = REC_DATA(rec);
-       int cfg_len = REC_DATA_LEN(rec);
        int rc;
-       ENTRY;
 
-       mrd = (struct mgs_replace_data *)data;
+       ENTRY;
 
        if (rec->lrh_type != OBD_CFG_REC) {
                CERROR("unhandled lrh_type: %#x, cmd %x %s %s\n",
@@ -1397,11 +1389,9 @@ static int mgs_replace_nids_handler(const struct lu_env *env,
                RETURN(-EINVAL);
        }
 
-       rc = lustre_cfg_sanity_check(lcfg, cfg_len);
-       if (rc) {
-               /* Do not copy any invalidated records */
+       rc = lustre_cfg_sanity_check(lcfg, REC_DATA_LEN(rec));
+       if (rc) /* Do not copy any invalidated records */
                GOTO(skip_out, rc = 0);
-       }
 
        rc = check_markers(lcfg, mrd);
        if (rc || mrd->state == REPLACE_SKIP)
@@ -1780,15 +1770,12 @@ static int mgs_clear_config_handler(const struct lu_env *env,
                                    struct llog_handle *llh,
                                    struct llog_rec_hdr *rec, void *data)
 {
-       struct mgs_replace_data *mrd;
+       struct mgs_replace_data *mrd = data;
        struct lustre_cfg *lcfg = REC_DATA(rec);
-       int cfg_len = REC_DATA_LEN(rec);
        int rc;
 
        ENTRY;
 
-       mrd = (struct mgs_replace_data *)data;
-
        if (rec->lrh_type != OBD_CFG_REC) {
                CDEBUG(D_MGS, "Config llog Name=%s, Record Index=%u, "
                       "Unhandled Record Type=%#x\n", llh->lgh_name,
@@ -1796,7 +1783,7 @@ static int mgs_clear_config_handler(const struct lu_env *env,
                RETURN(-EINVAL);
        }
 
-       rc = lustre_cfg_sanity_check(lcfg, cfg_len);
+       rc = lustre_cfg_sanity_check(lcfg, REC_DATA_LEN(rec));
        if (rc) {
                CDEBUG(D_MGS, "Config llog Name=%s, Invalid config file.",
                       llh->lgh_name);
@@ -2286,9 +2273,7 @@ static int mgs_copy_mdt_llog_handler(const struct lu_env *env,
        struct mgs_target_info *mti = mcd->mcd_mti;
        struct llog_handle *mdt_llh = mcd->mcd_llh;
        struct fs_db *fsdb = mcd->mcd_fsdb;
-       int cfg_len = rec->lrh_len;
-       char *cfg_buf = (char *)(rec + 1);
-       struct lustre_cfg *lcfg;
+       struct lustre_cfg *lcfg = REC_DATA(rec);
        char *mdtsrc = llh->lgh_name;
        char *s[5] = { 0 };
        int i;
@@ -2301,14 +2286,12 @@ static int mgs_copy_mdt_llog_handler(const struct lu_env *env,
                RETURN(-EINVAL);
        }
 
-       rc = lustre_cfg_sanity_check(cfg_buf, cfg_len);
+       rc = lustre_cfg_sanity_check(lcfg, REC_DATA_LEN(rec));
        if (rc) {
                CERROR("Insane cfg\n");
                RETURN(rc);
        }
 
-       lcfg = (struct lustre_cfg *)cfg_buf;
-
        if (lcfg->lcfg_command == LCFG_MARKER) {
                struct cfg_marker *marker = lustre_cfg_buf(lcfg, 1);
 
@@ -2532,9 +2515,7 @@ static int mgs_steal_client_llog_handler(const struct lu_env *env,
 {
        struct mgs_steal_data *msd = data;
        struct fs_db *fsdb = msd->msd_fsdb;
-       int cfg_len = rec->lrh_len;
-       char *cfg_buf = (char *)(rec + 1);
-       struct lustre_cfg *lcfg;
+       struct lustre_cfg *lcfg = REC_DATA(rec);
        char *s[5] = { 0 };
        int i;
        int rc = 0;
@@ -2547,14 +2528,12 @@ static int mgs_steal_client_llog_handler(const struct lu_env *env,
                RETURN(-EINVAL);
        }
 
-       rc = lustre_cfg_sanity_check(cfg_buf, cfg_len);
+       rc = lustre_cfg_sanity_check(lcfg, REC_DATA_LEN(rec));
        if (rc) {
                CERROR("%s: insane cfg: rc = %d\n", llh->lgh_name, rc);
                RETURN(rc);
        }
 
-       lcfg = (struct lustre_cfg *)cfg_buf;
-
        if (lcfg->lcfg_command == LCFG_MARKER) {
                struct cfg_marker *marker = lustre_cfg_buf(lcfg, 1);
                char *comment = marker->cm_comment;
@@ -4133,10 +4112,10 @@ static int mgs_srpc_read_handler(const struct lu_env *env,
                                 struct llog_rec_hdr *rec, void *data)
 {
        struct mgs_srpc_read_data *msrd = data;
-       struct cfg_marker         *marker;
-       struct lustre_cfg         *lcfg = REC_DATA(rec);
-       char                      *svname, *param;
-       int                        cfg_len, rc;
+       struct cfg_marker *marker;
+       struct lustre_cfg *lcfg = REC_DATA(rec);
+       char *svname, *param;
+       int rc;
 
        ENTRY;
        if (rec->lrh_type != OBD_CFG_REC) {
@@ -4144,9 +4123,7 @@ static int mgs_srpc_read_handler(const struct lu_env *env,
                RETURN(-EINVAL);
        }
 
-       cfg_len = REC_DATA_LEN(rec);
-
-       rc = lustre_cfg_sanity_check(lcfg, cfg_len);
+       rc = lustre_cfg_sanity_check(lcfg, REC_DATA_LEN(rec));
        if (rc) {
                CERROR("Insane cfg\n");
                RETURN(rc);

diff --git a/lustre/obdclass/class_obd.c b/lustre/obdclass/class_obd.c
index f505440..bf6aa51 100644 (file)
--- a/lustre/obdclass/class_obd.c
+++ b/lustre/obdclass/class_obd.c
@@ -339,12 +339,14 @@ int class_handle_ioctl(unsigned int cmd, void __user *uarg)
                OBD_ALLOC(lcfg, data->ioc_plen1);
                if (lcfg == NULL)
                        GOTO(out, rc = -ENOMEM);
-               rc = copy_from_user(lcfg, data->ioc_pbuf1, data->ioc_plen1);
-               if (!rc)
-                       rc = lustre_cfg_sanity_check(lcfg, data->ioc_plen1);
-               if (!rc)
-                       rc = class_process_config(lcfg, NULL);
-
+               if (copy_from_user(lcfg, data->ioc_pbuf1, data->ioc_plen1))
+                       GOTO(out_lcfg, rc = -EFAULT);
+               rc = lustre_cfg_sanity_check(lcfg, data->ioc_plen1);
+               if (rc)
+                       GOTO(out_lcfg, rc);
+               rc = class_process_config(lcfg, NULL);
+
+out_lcfg:
                OBD_FREE(lcfg, data->ioc_plen1);
                GOTO(out, rc);
        }

diff --git a/lustre/obdclass/obd_config.c b/lustre/obdclass/obd_config.c
index f5ddaef..09cec64 100644 (file)
--- a/lustre/obdclass/obd_config.c
+++ b/lustre/obdclass/obd_config.c
@@ -1752,28 +1752,27 @@ int class_config_llog_handler(const struct lu_env *env,
                              struct llog_rec_hdr *rec, void *data)
 {
        struct config_llog_instance *cfg = data;
-       int cfg_len = rec->lrh_len;
-       char *cfg_buf = (char *) (rec + 1);
        int rc = 0;
+
        ENTRY;
 
        /* class_config_dump_handler(handle, rec, data); */
 
        switch (rec->lrh_type) {
        case OBD_CFG_REC: {
-               struct lustre_cfg *lcfg, *lcfg_new;
+               struct lustre_cfg *lcfg = REC_DATA(rec);
+               struct lustre_cfg *lcfg_new;
                struct lustre_cfg_bufs bufs;
                char *inst_name = NULL;
                int inst_len = 0;
                int swab = 0;
 
-               lcfg = (struct lustre_cfg *)cfg_buf;
                if (lcfg->lcfg_version == __swab32(LUSTRE_CFG_VERSION)) {
                        lustre_swab_lustre_cfg(lcfg);
                        swab = 1;
                }
 
-               rc = lustre_cfg_sanity_check(cfg_buf, cfg_len);
+               rc = lustre_cfg_sanity_check(lcfg, REC_DATA_LEN(rec));
                if (rc)
                        GOTO(out, rc);
 
@@ -2080,7 +2079,7 @@ void llog_get_marker_cfg_flags(struct llog_rec_hdr *rec,
 int class_config_yaml_output(struct llog_rec_hdr *rec, char *buf, int size,
                             unsigned int *cfg_flags, bool raw)
 {
-       struct lustre_cfg *lcfg = (struct lustre_cfg *)(rec + 1);
+       struct lustre_cfg *lcfg = REC_DATA(rec);
        char *ptr = buf;
        char *end = buf + size;
        int rc = 0, i;
@@ -2091,7 +2090,7 @@ int class_config_yaml_output(struct llog_rec_hdr *rec, char *buf, int size,
        if (lcfg->lcfg_version == __swab32(LUSTRE_CFG_VERSION))
                lustre_swab_lustre_cfg(lcfg);
 
-       rc = lustre_cfg_sanity_check(lcfg, rec->lrh_len);
+       rc = lustre_cfg_sanity_check(lcfg, REC_DATA_LEN(rec));
        if (rc < 0)
                return rc;
 
@@ -2217,15 +2216,15 @@ out_overflow:
  */
 static int class_config_parse_rec(struct llog_rec_hdr *rec, char *buf, int size)
 {
-       struct lustre_cfg       *lcfg = (struct lustre_cfg *)(rec + 1);
-       char                    *ptr = buf;
-       char                    *end = buf + size;
-       int                      rc = 0;
+       struct lustre_cfg *lcfg = REC_DATA(rec);
+       char *ptr = buf;
+       char *end = buf + size;
+       int rc = 0;
 
        ENTRY;
 
        LASSERT(rec->lrh_type == OBD_CFG_REC);
-       rc = lustre_cfg_sanity_check(lcfg, rec->lrh_len);
+       rc = lustre_cfg_sanity_check(lcfg, REC_DATA_LEN(rec));
        if (rc < 0)
                RETURN(rc);
 

diff --git a/lustre/ptlrpc/nodemap_handler.c b/lustre/ptlrpc/nodemap_handler.c
index 3cd6eef..5233bb7 100644 (file)
--- a/lustre/ptlrpc/nodemap_handler.c
+++ b/lustre/ptlrpc/nodemap_handler.c
@@ -2870,8 +2870,9 @@ int server_iocontrol_nodemap(struct obd_device *obd,
 
        if (copy_from_user(lcfg, data->ioc_pbuf1, data->ioc_plen1))
                GOTO(out_lcfg, rc = -EFAULT);
-       if (lustre_cfg_sanity_check(lcfg, data->ioc_plen1))
-               GOTO(out_lcfg, rc = -EINVAL);
+       rc = lustre_cfg_sanity_check(lcfg, data->ioc_plen1);
+       if (rc)
+               GOTO(out_lcfg, rc);
 
        cmd = lcfg->lcfg_command;
 

diff --git a/lustre/target/tgt_mount.c b/lustre/target/tgt_mount.c
index e3f3df1..97757f7 100644 (file)
--- a/lustre/target/tgt_mount.c
+++ b/lustre/target/tgt_mount.c
@@ -798,9 +798,7 @@ static int client_lwp_config_process(const struct lu_env *env,
                                     struct llog_rec_hdr *rec, void *data)
 {
        struct config_llog_instance *cfg = data;
-       int cfg_len = rec->lrh_len;
-       char *cfg_buf = (char *) (rec + 1);
-       struct lustre_cfg *lcfg = NULL;
+       struct lustre_cfg *lcfg = REC_DATA(rec);
        struct lustre_sb_info *lsi;
        int rc = 0, swab = 0;
 
@@ -815,13 +813,12 @@ static int client_lwp_config_process(const struct lu_env *env,
                GOTO(out, rc = -EINVAL);
        lsi = s2lsi(cfg->cfg_sb);
 
-       lcfg = (struct lustre_cfg *)cfg_buf;
        if (lcfg->lcfg_version == __swab32(LUSTRE_CFG_VERSION)) {
                lustre_swab_lustre_cfg(lcfg);
                swab = 1;
        }
 
-       rc = lustre_cfg_sanity_check(cfg_buf, cfg_len);
+       rc = lustre_cfg_sanity_check(lcfg, REC_DATA_LEN(rec));
        if (rc < 0)
                GOTO(out, rc);