Re: [PATCH 4/4] libmultipath: fix over-long NVME WWIDs

["Benjamin Marzinski" <bmarzins@redhat.com>]

ACK, with one small nit below.

-Ben

On Fri, Jul 14, 2017 at 01:32:09PM +0200, Martin Wilck wrote:
> Fix for NVME wwids looking like this:
> nvme.0000-3163653363666438366239656630386200-4c696e75780000000000000000000000000000000000000000000000000000000000000000000000-00000002
> which are encountered in some combinations of Linux NVME host and target,
> where the 2nd field represents the serial number (SN) and the 3rd the
> model number (MN).
> 
> The device mapper allows map names only up to 128 characters.
> Strip the "00" sequences at the end of the serial and model field,
> they are hex-encoded 0-bytes which are forbidden by the NVME spec
> anyway.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/discovery.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 9951af84..f46b9b17 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1598,6 +1598,82 @@ get_prio (struct path * pp)
>  	return 0;
>  }
>  
> +/*
> + * Mangle string of length *len starting at start
> + * by removing character sequence "00" (hex for a 0 byte),
> + * starting at end, backwards.
> + * Changes the value of *len if characters were removed.
> + * Returns a pointer to the position where "end" was moved to.
> + */
> +static char
> +*skip_zeroes_backward(char* start, int *len, char *end)
> +{
> +	char *p = end;
> +
> +	while (p >= start + 2 && *(p - 1) == '0' && *(p - 2) == '0')
> +		p -= 2;
> +
> +	if (p == end)
> +		return p;
> +
> +	memmove(p, end, start + *len + 1 - end);
> +	*len -= end - p;
> +
> +	return p;
> +}
> +
> +/*
> + * Fix for NVME wwids looking like this:
> + * nvme.0000-3163653363666438366239656630386200-4c696e75780000000000000000000000000000000000000000000000000000000000000000000000-00000002
> + * which are encountered in some combinations of Linux NVME host and target.
> + * The '00' are hex-encoded 0-bytes which are forbidden in the serial (SN)
> + * and model (MN) fields. Discard them.
> + * If a WWID of the above type is found, sets pp->wwid and returns a value > 0.
> + * Otherwise, returns 0.
> + */
> +static int
> +fix_broken_nvme_wwid(struct path *pp, const char *value, int size)
> +{
> +	static const char _nvme[] = "nvme.";
> +	int len, i;
> +	char mangled[256];
> +	char *p;
> +
> +	len = strlen(value);
> +	if (len >= sizeof(mangled) || len + 1 < size)
> +		return 0;

Don't we check (len + 1 < size) before calling this? It seems odd to
return that we can't do anything when in reality, we simply don't need
to do anything.

-Ben

> +
> +	/* Check that value starts with "nvme.%04x-" */
> +	if (memcmp(value, _nvme, sizeof(_nvme) - 1) || value[9] != '-')
> +		return 0;
> +	for (i = 5; i < 9; i++)
> +		if (!isxdigit(value[i]))
> +			return 0;
> +
> +	memcpy(mangled, value, len + 1);
> +
> +	/* search end of "model" part and strip trailing '00' */
> +	p = memrchr(mangled, '-', len);
> +	if (p == NULL)
> +		return 0;
> +
> +	p = skip_zeroes_backward(mangled, &len, p);
> +
> +	/* search end of "serial" part */
> +	p = memrchr(mangled, '-', p - mangled);
> +	if (p == NULL || memrchr(mangled, '-', p - mangled) != mangled + 9)
> +	    /* We expect exactly 3 '-' in the value */
> +		return 0;
> +
> +	p = skip_zeroes_backward(mangled, &len, p);
> +	if (len >= size)
> +		return 0;
> +
> +	memcpy(pp->wwid, mangled, len + 1);
> +	condlog(2, "%s: over-long WWID shortened to %s", pp->dev, pp->wwid);
> +	return len;
> +}
> +
>  static int
>  get_udev_uid(struct path * pp, char *uid_attribute, struct udev_device *udev)
>  {
> @@ -1609,6 +1685,9 @@ get_udev_uid(struct path * pp, char *uid_attribute, struct udev_device *udev)
>  		value = getenv(uid_attribute);
>  	if (value && strlen(value)) {
>  		if (strlen(value) + 1 > WWID_SIZE) {
> +			len = fix_broken_nvme_wwid(pp, value, WWID_SIZE);
> +			if (len > 0)
> +				return len;
>  			condlog(0, "%s: wwid overflow", pp->dev);
>  			len = WWID_SIZE;
>  		} else {
> -- 
> 2.13.2