On Wed, Aug 11, 2021 at 05:41:42PM +0200, mwilck@xxxxxxxx wrote:
> From: Martin Wilck <mwilck@xxxxxxxx>
>
> We've seen a crash of multipath in disassemble_map because of a params
> string exceeding PARAMS_SIZE. While the crash could have been fixed by
> a simple error check, I believe multipath should be able to work with
> arbitrary long parameter strings passed from the kernel.
>
> The parameter list of dm_get_map() has changed. Bumped ABI version and
> removed dm_get_map() and some functions from the ABI, which are only
> called from libmultipath itself.
>
> Signed-off-by: Martin Wilck <mwilck@xxxxxxxx>
Reviewed-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>
> ---
> libmultipath/devmapper.c | 44 ++++++++++++++++++++-----------
> libmultipath/devmapper.h | 4 +--
> libmultipath/libmultipath.version | 6 +----
> libmultipath/structs_vec.c | 11 +++++---
> 4 files changed, 38 insertions(+), 27 deletions(-)
>
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index 945e625..c05dc20 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -648,7 +648,7 @@ int dm_map_present(const char * str)
> return (do_get_info(str, &info) == 0);
> }
>
> -int dm_get_map(const char *name, unsigned long long *size, char *outparams)
> +int dm_get_map(const char *name, unsigned long long *size, char **outparams)
> {
> int r = DMP_ERR;
> struct dm_task *dmt;
> @@ -682,12 +682,13 @@ int dm_get_map(const char *name, unsigned long long *size, char *outparams)
> if (size)
> *size = length;
>
> - if (!outparams) {
> + if (!outparams)
> r = DMP_OK;
> - goto out;
> + else {
> + *outparams = strdup(params);
> + r = *outparams ? DMP_OK : DMP_ERR;
> }
> - if (snprintf(outparams, PARAMS_SIZE, "%s", params) <= PARAMS_SIZE)
> - r = DMP_OK;
> +
> out:
> dm_task_destroy(dmt);
> return r;
> @@ -761,7 +762,7 @@ is_mpath_part(const char *part_name, const char *map_name)
> return 0;
> }
>
> -int dm_get_status(const char *name, char *outstatus)
> +int dm_get_status(const char *name, char **outstatus)
> {
> int r = DMP_ERR;
> struct dm_task *dmt;
> @@ -799,8 +800,12 @@ int dm_get_status(const char *name, char *outstatus)
> goto out;
> }
>
> - if (snprintf(outstatus, PARAMS_SIZE, "%s", status) <= PARAMS_SIZE)
> + if (!outstatus)
> r = DMP_OK;
> + else {
> + *outstatus = strdup(status);
> + r = *outstatus ? DMP_OK : DMP_ERR;
> + }
> out:
> if (r != DMP_OK)
> condlog(0, "%s: error getting map status string", name);
> @@ -1049,7 +1054,7 @@ int _dm_flush_map (const char * mapname, int need_sync, int deferred_remove,
> int queue_if_no_path = 0;
> int udev_flags = 0;
> unsigned long long mapsize;
> - char params[PARAMS_SIZE] = {0};
> + char *params = NULL;
>
> if (dm_is_mpath(mapname) != 1)
> return 0; /* nothing to do */
> @@ -1065,7 +1070,7 @@ int _dm_flush_map (const char * mapname, int need_sync, int deferred_remove,
> return 1;
>
> if (need_suspend &&
> - dm_get_map(mapname, &mapsize, params) == DMP_OK &&
> + dm_get_map(mapname, &mapsize, ¶ms) == DMP_OK &&
> strstr(params, "queue_if_no_path")) {
> if (!dm_queue_if_no_path(mapname, 0))
> queue_if_no_path = 1;
> @@ -1073,6 +1078,8 @@ int _dm_flush_map (const char * mapname, int need_sync, int deferred_remove,
> /* Leave queue_if_no_path alone if unset failed */
> queue_if_no_path = -1;
> }
> + free(params);
> + params = NULL;
>
> if (dm_remove_partmaps(mapname, need_sync, deferred_remove))
> return 1;
> @@ -1431,7 +1438,7 @@ do_foreach_partmaps (const char * mapname,
> struct dm_task *dmt;
> struct dm_names *names;
> unsigned next = 0;
> - char params[PARAMS_SIZE];
> + char *params = NULL;
> unsigned long long size;
> char dev_t[32];
> int r = 1;
> @@ -1474,7 +1481,7 @@ do_foreach_partmaps (const char * mapname,
> /*
> * and we can fetch the map table from the kernel
> */
> - dm_get_map(names->name, &size, ¶ms[0]) == DMP_OK &&
> + dm_get_map(names->name, &size, ¶ms) == DMP_OK &&
>
> /*
> * and the table maps over the multipath map
> @@ -1486,12 +1493,15 @@ do_foreach_partmaps (const char * mapname,
> goto out;
> }
>
> + free(params);
> + params = NULL;
> next = names->next;
> names = (void *) names + next;
> } while (next);
>
> r = 0;
> out:
> + free(params);
> dm_task_destroy (dmt);
> return r;
> }
> @@ -1620,17 +1630,19 @@ struct rename_data {
> static int
> rename_partmap (const char *name, void *data)
> {
> - char buff[PARAMS_SIZE];
> + char *buff = NULL;
> int offset;
> struct rename_data *rd = (struct rename_data *)data;
>
> if (strncmp(name, rd->old, strlen(rd->old)) != 0)
> return 0;
> for (offset = strlen(rd->old); name[offset] && !(isdigit(name[offset])); offset++); /* do nothing */
> - snprintf(buff, PARAMS_SIZE, "%s%s%s", rd->new, rd->delim,
> - name + offset);
> - dm_rename(name, buff, rd->delim, SKIP_KPARTX_OFF);
> - condlog(4, "partition map %s renamed", name);
> + if (asprintf(&buff, "%s%s%s", rd->new, rd->delim, name + offset) >= 0) {
> + dm_rename(name, buff, rd->delim, SKIP_KPARTX_OFF);
> + free(buff);
> + condlog(4, "partition map %s renamed", name);
> + } else
> + condlog(1, "failed to rename partition map %s", name);
> return 0;
> }
>
> diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
> index e29b4d4..45a676d 100644
> --- a/libmultipath/devmapper.h
> +++ b/libmultipath/devmapper.h
> @@ -44,8 +44,8 @@ int dm_addmap_create (struct multipath *mpp, char *params);
> int dm_addmap_reload (struct multipath *mpp, char *params, int flush);
> int dm_map_present (const char *);
> int dm_map_present_by_uuid(const char *uuid);
> -int dm_get_map(const char *, unsigned long long *, char *);
> -int dm_get_status(const char *, char *);
> +int dm_get_map(const char *, unsigned long long *, char **);
> +int dm_get_status(const char *, char **);
> int dm_type(const char *, char *);
> int dm_is_mpath(const char *);
> int _dm_flush_map (const char *, int, int, int, int);
> diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
> index 0cff311..7567837 100644
> --- a/libmultipath/libmultipath.version
> +++ b/libmultipath/libmultipath.version
> @@ -31,7 +31,7 @@
> * The new version inherits the previous ones.
> */
>
> -LIBMULTIPATH_5.0.0 {
> +LIBMULTIPATH_6.0.0 {
> global:
> /* symbols referenced by multipath and multipathd */
> add_foreign;
> @@ -58,8 +58,6 @@ global:
> count_active_paths;
> delete_all_foreign;
> delete_foreign;
> - disassemble_map;
> - disassemble_status;
> dlog;
> dm_cancel_deferred_remove;
> dm_enablegroup;
> @@ -70,10 +68,8 @@ global:
> dm_geteventnr;
> dm_get_info;
> dm_get_major_minor;
> - dm_get_map;
> dm_get_maps;
> dm_get_multipath;
> - dm_get_status;
> dm_get_uuid;
> dm_is_mpath;
> dm_mapname;
> diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
> index 7539019..24d6fd2 100644
> --- a/libmultipath/structs_vec.c
> +++ b/libmultipath/structs_vec.c
> @@ -416,12 +416,12 @@ int
> update_multipath_table (struct multipath *mpp, vector pathvec, int flags)
> {
> int r = DMP_ERR;
> - char params[PARAMS_SIZE] = {0};
> + char *params = NULL;
>
> if (!mpp)
> return r;
>
> - r = dm_get_map(mpp->alias, &mpp->size, params);
> + r = dm_get_map(mpp->alias, &mpp->size, ¶ms);
> if (r != DMP_OK) {
> condlog(2, "%s: %s", mpp->alias, (r == DMP_ERR)? "error getting table" : "map not present");
> return r;
> @@ -429,14 +429,17 @@ update_multipath_table (struct multipath *mpp, vector pathvec, int flags)
>
> if (disassemble_map(pathvec, params, mpp)) {
> condlog(2, "%s: cannot disassemble map", mpp->alias);
> + free(params);
> return DMP_ERR;
> }
>
> - *params = '\0';
> - if (dm_get_status(mpp->alias, params) != DMP_OK)
> + free(params);
> + params = NULL;
> + if (dm_get_status(mpp->alias, ¶ms) != DMP_OK)
> condlog(2, "%s: %s", mpp->alias, (r == DMP_ERR)? "error getting status" : "map not present");
> else if (disassemble_status(params, mpp))
> condlog(2, "%s: cannot disassemble status", mpp->alias);
> + free(params);
>
> /* FIXME: we should deal with the return value here */
> update_pathvec_from_dm(pathvec, mpp, flags);
> --
> 2.32.0
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
