Re: [dm-devel] [PATCH 1/9] libmultipath: variable-size parameters in dm_get_map()

[Benjamin Marzinski <bmarzins@redhat.com>]

On Thu, Jul 15, 2021 at 12:52:15PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> 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@suse.com>
> ---
>  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..a5194d8 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;

Missing the dereference here "r = *outstatus ?"

-Ben

> +	}
>  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, &params) == 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, &params[0]) == DMP_OK &&
> +		    dm_get_map(names->name, &size, &params) == 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, &params);
>  	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, &params) != 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@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel