From 7de178ba4afcf8c50c5430b861c9d0eeb84f800f Mon Sep 17 00:00:00 2001 From: Andreas Dilger Date: Wed, 28 Mar 2018 15:42:06 -0600 Subject: [PATCH] LU-10822 utils: stop bogus buffer overflow errors Over-zealous Fortify checks assume that the buffer being used for snprintf() in get_lmd_info() is sizeof(*lmd) when in fact a larger buffer has been allocated. This causes runtime checks to fail and lfs to core dump: *** buffer overflow detected ***: /usr/bin/lfs terminated Instead of printing directly into "struct lov_user_mds_data", use a generic buffer to hold the filename passed into the ioctl and the return data. There are several places in the code which do the same operations, namely cb_getstripe(), get_lmd_info(), and ct_md_getattr(), so change them all to call get_lmd_info() or a new get_lmd_info_fd() helper to consolidate common code. Also check the return values from snprintf() in case there are new callers of this code in the future that do not actually pass large-enough buffers. Test-Parameters: clientdistro=ubuntu1604 serverdistro=el7 testlist=sanity Test-Parameters: trivial testlist=sanity-hsm Signed-off-by: Andreas Dilger Change-Id: I41b1fcba1f7937fbce3cc7180ed5d73d067cab07 Reviewed-on: https://review.whamcloud.com/31822 Tested-by: Jenkins Reviewed-by: John L. Hammond Tested-by: Maloo Reviewed-by: Oleg Drokin --- lustre/include/uapi/linux/lustre/lustre_user.h | 5 - lustre/utils/liblustreapi.c | 147 ++++++++++++++----------- lustre/utils/liblustreapi_hsm.c | 22 ++-- lustre/utils/lustreapi_internal.h | 8 ++ 4 files changed, 101 insertions(+), 81 deletions(-) diff --git a/lustre/include/uapi/linux/lustre/lustre_user.h b/lustre/include/uapi/linux/lustre/lustre_user.h index f516f9d..65f718a 100644 --- a/lustre/include/uapi/linux/lustre/lustre_user.h +++ b/lustre/include/uapi/linux/lustre/lustre_user.h @@ -687,11 +687,6 @@ struct lov_user_mds_data_v1 { lstat_t lmd_st; /* MDS stat struct */ struct lov_user_md_v1 lmd_lmm; /* LOV EA V1 user data */ } __attribute__((packed)); - -struct lov_user_mds_data_v3 { - lstat_t lmd_st; /* MDS stat struct */ - struct lov_user_md_v3 lmd_lmm; /* LOV EA V3 user data */ -} __attribute__((packed)); #endif struct lmv_user_mds_data { diff --git a/lustre/utils/liblustreapi.c b/lustre/utils/liblustreapi.c index 1d3218a..a5c1347 100644 --- a/lustre/utils/liblustreapi.c +++ b/lustre/utils/liblustreapi.c @@ -1644,7 +1644,7 @@ static int common_param_init(struct find_param *param, char *path) lum_size = PATH_MAX + 1; param->fp_lum_size = lum_size; - param->fp_lmd = calloc(1, sizeof(lstat_t) + param->fp_lum_size); + param->fp_lmd = calloc(1, sizeof(lstat_t) + lum_size); if (param->fp_lmd == NULL) { llapi_error(LLAPI_MSG_ERROR, -ENOMEM, "error: allocation of %zu bytes for ioctl", @@ -1747,64 +1747,95 @@ again: return ret; } -static int get_lmd_info(char *path, DIR *parent, DIR *dir, - struct lov_user_mds_data *lmd, int lumlen) +int get_lmd_info_fd(char *path, int parent_fd, int dir_fd, + void *lmdbuf, int lmdlen, enum get_lmd_info_type type) { - lstat_t *st = &lmd->lmd_st; - int ret = 0; + struct lov_user_mds_data *lmd = lmdbuf; + lstat_t *st = &lmd->lmd_st; + int ret = 0; - if (parent == NULL && dir == NULL) - return -EINVAL; + if (parent_fd < 0 && dir_fd < 0) + return -EINVAL; + if (type != GET_LMD_INFO && type != GET_LMD_STRIPE) + return -EINVAL; - if (dir) { - ret = ioctl(dirfd(dir), LL_IOC_MDC_GETINFO, (void *)lmd); - } else if (parent) { + if (dir_fd >= 0) { + /* LL_IOC_MDC_GETINFO operates on the current directory inode + * and returns struct lov_user_mds_data, while + * LL_IOC_LOV_GETSTRIPE returns only struct lov_user_md. + */ + ret = ioctl(dir_fd, type == GET_LMD_INFO ? LL_IOC_MDC_GETINFO : + LL_IOC_LOV_GETSTRIPE, + lmdbuf); + } else if (parent_fd >= 0) { char *fname = strrchr(path, '/'); - /* To avoid opening, locking, and closing each file on the - * client if that is not needed. The GETFILEINFO ioctl can - * be done on the patent dir with a single open for all + /* IOC_MDC_GETFILEINFO takes as input the filename (relative to + * the parent directory) and returns struct lov_user_mds_data, + * while IOC_MDC_GETFILESTRIPE returns only struct lov_user_md. + * + * This avoids opening, locking, and closing each file on the + * client if that is not needed. Multiple of these ioctl() can + * be done on the parent dir with a single open for all * files in that directory, and it also doesn't pollute the * client dcache with millions of dentries when traversing - * a large filesystem. */ + * a large filesystem. + */ fname = (fname == NULL ? path : fname + 1); - /* retrieve needed file info */ - snprintf((char *)lmd, lumlen, "%s", fname); - ret = ioctl(dirfd(parent), IOC_MDC_GETFILEINFO, (void *)lmd); - } - if (ret) { - if (errno == ENOTTY) { - /* ioctl is not supported, it is not a lustre fs. - * Do the regular lstat(2) instead. */ - ret = lstat_f(path, st); - if (ret) { - ret = -errno; - llapi_error(LLAPI_MSG_ERROR, ret, - "error: %s: lstat failed for %s", - __func__, path); - } - } else if (errno == ENOENT) { - ret = -errno; - llapi_error(LLAPI_MSG_WARN, ret, - "warning: %s: %s does not exist", - __func__, path); - } else if (errno != EISDIR) { - ret = -errno; - llapi_error(LLAPI_MSG_ERROR, ret, - "%s ioctl failed for %s.", - dir ? "LL_IOC_MDC_GETINFO" : - "IOC_MDC_GETFILEINFO", path); - } else { + ret = snprintf(lmdbuf, lmdlen, "%s", fname); + if (ret < 0) + errno = -ret; + else if (ret >= lmdlen || ret++ == 0) + errno = EINVAL; + else + ret = ioctl(parent_fd, type == GET_LMD_INFO ? + IOC_MDC_GETFILEINFO : + IOC_MDC_GETFILESTRIPE, lmdbuf); + } + + if (ret && type == GET_LMD_INFO) { + if (errno == ENOTTY) { + /* ioctl is not supported, it is not a lustre fs. + * Do the regular lstat(2) instead. + */ + ret = lstat_f(path, st); + if (ret) { + ret = -errno; + llapi_error(LLAPI_MSG_ERROR, ret, + "error: %s: lstat failed for %s", + __func__, path); + } + } else if (errno == ENOENT) { + ret = -errno; + llapi_error(LLAPI_MSG_WARN, ret, + "warning: %s does not exist", path); + } else if (errno != EISDIR && errno != ENODATA) { ret = -errno; llapi_error(LLAPI_MSG_ERROR, ret, - "error: %s: IOC_MDC_GETFILEINFO failed for %s", - __func__, path); + "%s ioctl failed for %s.", + dir_fd >= 0 ? "LL_IOC_MDC_GETINFO" : + "IOC_MDC_GETFILEINFO", path); } } + return ret; } +static int get_lmd_info(char *path, DIR *parent, DIR *dir, void *lmdbuf, + int lmdlen, enum get_lmd_info_type type) +{ + int parent_fd = -1; + int dir_fd = -1; + + if (parent) + parent_fd = dirfd(parent); + if (dir) + dir_fd = dirfd(dir); + + return get_lmd_info_fd(path, parent_fd, dir_fd, lmdbuf, lmdlen, type); +} + static int llapi_semantic_traverse(char *path, int size, DIR *parent, semantic_func_t sem_init, semantic_func_t sem_fini, void *data, @@ -1859,7 +1890,7 @@ static int llapi_semantic_traverse(char *path, int size, DIR *parent, lstat_t *st = ¶m->fp_lmd->lmd_st; rc = get_lmd_info(path, d, NULL, param->fp_lmd, - param->fp_lum_size); + param->fp_lum_size, GET_LMD_INFO); if (rc == 0) dent->d_type = IFTODT(st->st_mode); else if (ret == 0) @@ -3984,7 +4015,7 @@ static int cb_find_init(char *path, DIR *parent, DIR **dirp, param->fp_lmd->lmd_lmm.lmm_magic = 0; ret = get_lmd_info(path, parent, dir, param->fp_lmd, - param->fp_lum_size); + param->fp_lum_size, GET_LMD_INFO); if (ret == 0 && param->fp_lmd->lmd_lmm.lmm_magic == 0 && find_check_lmm_info(param)) { struct lov_user_md *lmm = ¶m->fp_lmd->lmd_lmm; @@ -4507,26 +4538,14 @@ static int cb_getstripe(char *path, DIR *parent, DIR **dirp, void *data, return ret; } - if (d) { - if (param->fp_get_lmv || param->fp_get_default_lmv) { - ret = cb_get_dirstripe(path, d, param); - } else { - ret = ioctl(dirfd(d), LL_IOC_LOV_GETSTRIPE, - (void *)¶m->fp_lmd->lmd_lmm); - } - - } else if (parent && !param->fp_get_lmv && !param->fp_get_default_lmv) { - char *fname = strrchr(path, '/'); - fname = (fname == NULL ? path : fname + 1); - - snprintf((char *)¶m->fp_lmd->lmd_lmm, param->fp_lum_size, - "%s", fname); - - ret = ioctl(dirfd(parent), IOC_MDC_GETFILESTRIPE, - (void *)¶m->fp_lmd->lmd_lmm); - } else { + if (d && (param->fp_get_lmv || param->fp_get_default_lmv)) + ret = cb_get_dirstripe(path, d, param); + else if (d || + (parent && !param->fp_get_lmv && !param->fp_get_default_lmv)) + ret = get_lmd_info(path, parent, d, ¶m->fp_lmd->lmd_lmm, + param->fp_lum_size, GET_LMD_STRIPE); + else return 0; - } if (ret) { if (errno == ENODATA && d != NULL) { diff --git a/lustre/utils/liblustreapi_hsm.c b/lustre/utils/liblustreapi_hsm.c index d3dc449..41eac0f 100644 --- a/lustre/utils/liblustreapi_hsm.c +++ b/lustre/utils/liblustreapi_hsm.c @@ -994,32 +994,30 @@ static int ct_md_getattr(const struct hsm_copytool_private *ct, lstat_t *st) { struct lov_user_mds_data *lmd; + char fname[FID_NOBRACE_LEN + 1] = ""; size_t lmd_size; int rc; + rc = snprintf(fname, sizeof(fname), DFID_NOBRACE, PFID(fid)); + if (rc < 0) + return rc; + if (rc >= sizeof(fname) || rc == 0) + return -EINVAL; + lmd_size = sizeof(lmd->lmd_st) + lov_user_md_size(LOV_MAX_STRIPE_COUNT, LOV_USER_MAGIC_V3); if (lmd_size < sizeof(lmd->lmd_st) + XATTR_SIZE_MAX) lmd_size = sizeof(lmd->lmd_st) + XATTR_SIZE_MAX; - if (lmd_size < FID_NOBRACE_LEN + 1) - lmd_size = FID_NOBRACE_LEN + 1; - lmd = malloc(lmd_size); if (lmd == NULL) return -ENOMEM; - snprintf((char *)lmd, lmd_size, DFID_NOBRACE, PFID(fid)); - - rc = ioctl(ct->open_by_fid_fd, IOC_MDC_GETFILEINFO, lmd); - if (rc != 0) { - rc = -errno; - llapi_error(LLAPI_MSG_ERROR, rc, - "cannot get metadata attributes of "DFID" in '%s'", - PFID(fid), ct->mnt); + rc = get_lmd_info_fd(fname, ct->open_by_fid_fd, -1, + lmd, lmd_size, GET_LMD_INFO); + if (rc) goto out; - } *st = lmd->lmd_st; out: diff --git a/lustre/utils/lustreapi_internal.h b/lustre/utils/lustreapi_internal.h index 92e4994..4d636f4 100644 --- a/lustre/utils/lustreapi_internal.h +++ b/lustre/utils/lustreapi_internal.h @@ -155,4 +155,12 @@ int libcfs_ukuc_stop(struct lustre_kernelcomm *l); int libcfs_ukuc_get_rfd(struct lustre_kernelcomm *link); int libcfs_ukuc_msg_get(struct lustre_kernelcomm *l, char *buf, int maxsize, int transport); + +enum get_lmd_info_type { + GET_LMD_INFO = 1, + GET_LMD_STRIPE = 2, +}; + +int get_lmd_info_fd(char *path, int parentfd, int dirfd, + void *lmd_buf, int lmd_len, enum get_lmd_info_type type); #endif /* _LUSTREAPI_INTERNAL_H_ */ -- 1.8.3.1