From: Andreas Dilger Date: Tue, 11 Mar 2025 00:17:59 +0000 (-0600) Subject: LU-17000 mgs: check lcfg parameters X-Git-Tag: 2.16.54~68 X-Git-Url: https://git.whamcloud.com/?a=commitdiff_plain;h=refs%2Fchanges%2F60%2F58360%2F7;p=fs%2Flustre-release.git 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 Change-Id: I9a7052a24d77124582df61340e520c1db6433892 Reviewed-on: https://review.whamcloud.com/c/fs/lustre-release/+/58360 Tested-by: jenkins Tested-by: Maloo Reviewed-by: Arshad Hussain Reviewed-by: Sebastien Buisson Reviewed-by: Oleg Drokin --- diff --git a/lustre/include/uapi/linux/lustre/lustre_cfg.h b/lustre/include/uapi/linux/lustre/lustre_cfg.h index de3dc29..1a2ef77 100644 --- 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 --- 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 . */ 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 --- 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 --- 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 --- 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 --- 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 --- 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);