[PATCH v2 0/4] [PATCH 0/4] libmpathpersist allocation size fixes

[PATCH v2 0/4] [PATCH 0/4] libmpathpersist allocation size fixes

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Hi Christophe,

We recently found a problem with mpathpersist using an allocation length
of > 8192 bytes for PERSISTENT RESERVE IN/OUT commands. This patch set
tries to fix it. The last patch would change the ABI for external programs,
it's optional.

(Christophe, to avoid misunderstanding:	This set is not meant to be included
in the upcoming merge).

Reviews and comments welcome.

Changes in v2:
 - 02/04: fix overflow calculation (Ben)
 - 04/04: improve commit description wrt ABI breakage

Martin Wilck (4):
  libmpathpersist: limit PRIN allocation length to 8192 bytes
  libmpathpersist: format_transportids(): avoid PROUT overflow
  libmpathpersist: mpath_format_readfullstatus(): use real buffer size
  libmpathpersist: ABI change: limit data-in/out size to 8192 bytes

 libmpathpersist/mpath_persist.h  | 15 ++++++++---
 libmpathpersist/mpath_pr_ioctl.c | 45 +++++++++++++++++++++++++++-----
 2 files changed, 50 insertions(+), 10 deletions(-)

--

2.25.1

[PATCH v2 1/4] libmpathpersist: limit PRIN allocation length to 8192 bytes

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Some targets (notably the qemu-pr-helper) don't support PERSISTENT
RESERVE IN  commands with more than 8192 bytes allocation length.
While I have found no explicit requirement in the SCSI specs that
the allocation lengh may not exceed 8k, an 8k limit is also enforced
by sg_persist(8), and actually by mpathpersist itself for the
--allocation-length option, but not for the auto-determined length.

Fix that.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmpathpersist/mpath_pr_ioctl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libmpathpersist/mpath_pr_ioctl.c b/libmpathpersist/mpath_pr_ioctl.c
index 74b26b0c..1a28cba7 100644
--- a/libmpathpersist/mpath_pr_ioctl.c
+++ b/libmpathpersist/mpath_pr_ioctl.c
@@ -543,5 +543,7 @@ int get_prin_length(int rq_servact)
 			mx_resp_len = 0;
 			break;
 	}
+	if (mx_resp_len > MPATH_MAX_PARAM_LEN)
+		mx_resp_len = MPATH_MAX_PARAM_LEN;
 	return mx_resp_len;
 }
-- 
2.25.1

[PATCH v2 2/4] libmpathpersist: format_transportids(): avoid PROUT overflow

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This limits the PERSISTENT RESERVE OUT data size to max. 8192 bytes.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmpathpersist/mpath_pr_ioctl.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/libmpathpersist/mpath_pr_ioctl.c b/libmpathpersist/mpath_pr_ioctl.c
index 1a28cba7..c78e8000 100644
--- a/libmpathpersist/mpath_pr_ioctl.c
+++ b/libmpathpersist/mpath_pr_ioctl.c
@@ -1,5 +1,6 @@
 #include <stdio.h>
 #include <stdlib.h>
+#include <stddef.h>
 
 #include <sys/types.h>
 #include <sys/stat.h>
@@ -138,38 +139,64 @@ retry :
 	return status;
 }
 
+/*
+ * Helper macro to avoid overflow of prout_param_descriptor in
+ * format_transportids(). Data must not be written past
+ * MPATH_MAX_PARAM_LEN bytes from struct prout_param_descriptor.
+ */
+#define check_overflow(ofs, n, start, label)				\
+	do {								\
+		if ((ofs) + (n) +					\
+		    offsetof(struct prout_param_descriptor, private_buffer) \
+		    > MPATH_MAX_PARAM_LEN)				\
+		{							\
+			(ofs) = (start);				\
+			goto label;					\
+		}							\
+	} while(0)
+
 uint32_t  format_transportids(struct prout_param_descriptor *paramp)
 {
 	unsigned int i = 0, len;
 	uint32_t buff_offset = 4;
-	memset(paramp->private_buffer, 0, MPATH_MAX_PARAM_LEN);
+	memset(paramp->private_buffer, 0, sizeof(paramp->private_buffer));
 	for (i=0; i < paramp->num_transportid; i++ )
 	{
+		uint32_t start_offset = buff_offset;
+
+		check_overflow(buff_offset, 1, start_offset, end_loop);
 		paramp->private_buffer[buff_offset] = (uint8_t)((paramp->trnptid_list[i]->format_code & 0xff)|
 							(paramp->trnptid_list[i]->protocol_id & 0xff));
 		buff_offset += 1;
 		switch(paramp->trnptid_list[i]->protocol_id)
 		{
 			case MPATH_PROTOCOL_ID_FC:
+				check_overflow(buff_offset, 7 + 8 + 8,
+					       start_offset, end_loop);
 				buff_offset += 7;
 				memcpy(&paramp->private_buffer[buff_offset], &paramp->trnptid_list[i]->n_port_name, 8);
 				buff_offset +=8 ;
 				buff_offset +=8 ;
 				break;
 			case MPATH_PROTOCOL_ID_SAS:
+				check_overflow(buff_offset, 3 + 12,
+					       start_offset, end_loop);
 				buff_offset += 3;
 				memcpy(&paramp->private_buffer[buff_offset], &paramp->trnptid_list[i]->sas_address, 8);
 				buff_offset += 12;
 				break;
 			case MPATH_PROTOCOL_ID_ISCSI:
-				buff_offset += 1;
 				len = (paramp->trnptid_list[i]->iscsi_name[1] & 0xff)+2;
+				check_overflow(buff_offset, 1 + len,
+					       start_offset, end_loop);
+				buff_offset += 1;
 				memcpy(&paramp->private_buffer[buff_offset], &paramp->trnptid_list[i]->iscsi_name,len);
 				buff_offset += len ;
 				break;
 		}
 
 	}
+end_loop:
 	buff_offset -= 4;
 	paramp->private_buffer[0] = (unsigned char)((buff_offset >> 24) & 0xff);
 	paramp->private_buffer[1] = (unsigned char)((buff_offset >> 16) & 0xff);
-- 
2.25.1

[PATCH v2 3/4] libmpathpersist: mpath_format_readfullstatus(): use real buffer size

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This changes no semantics, but it will allow changing the size of
prin_readfd.private_buffer in a follow-up patch.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmpathpersist/mpath_pr_ioctl.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/libmpathpersist/mpath_pr_ioctl.c b/libmpathpersist/mpath_pr_ioctl.c
index c78e8000..fadc9e10 100644
--- a/libmpathpersist/mpath_pr_ioctl.c
+++ b/libmpathpersist/mpath_pr_ioctl.c
@@ -238,6 +238,8 @@ static void mpath_format_readfullstatus(struct prin_resp *pr_buff)
 	uint32_t additional_length, k, tid_len_len = 0;
 	char tempbuff[MPATH_MAX_PARAM_LEN];
 	struct prin_fulldescr fdesc;
+	static const int pbuf_size =
+		sizeof(pr_buff->prin_descriptor.prin_readfd.private_buffer);
 
 	convert_be32_to_cpu(&pr_buff->prin_descriptor.prin_readfd.prgeneration);
 	convert_be32_to_cpu(&pr_buff->prin_descriptor.prin_readfd.number_of_descriptor);
@@ -249,16 +251,18 @@ static void mpath_format_readfullstatus(struct prin_resp *pr_buff)
 	}
 
 	additional_length = pr_buff->prin_descriptor.prin_readfd.number_of_descriptor;
-	if (additional_length > MPATH_MAX_PARAM_LEN) {
+	if (additional_length > pbuf_size) {
 		condlog(3, "PRIN length %u exceeds max length %d", additional_length,
-			MPATH_MAX_PARAM_LEN);
+			pbuf_size);
 		return;
 	}
 
 	memset(&fdesc, 0, sizeof(struct prin_fulldescr));
 
-	memcpy( tempbuff, pr_buff->prin_descriptor.prin_readfd.private_buffer,MPATH_MAX_PARAM_LEN );
-	memset(&pr_buff->prin_descriptor.prin_readfd.private_buffer, 0, MPATH_MAX_PARAM_LEN);
+	memcpy( tempbuff, pr_buff->prin_descriptor.prin_readfd.private_buffer,
+		pbuf_size);
+	memset(&pr_buff->prin_descriptor.prin_readfd.private_buffer, 0,
+	       pbuf_size);
 
 	p =(unsigned char *)tempbuff;
 	ppbuff = (char *)pr_buff->prin_descriptor.prin_readfd.private_buffer;
-- 
2.25.1

[PATCH v2 4/4] libmpathpersist: ABI change: limit data-in/out size to 8192 bytes

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Make sure that data structures used for PERSISTENT RESERVE IN/OUT
fit into 8k buffers.

This patch breaks the libmpathpersist ABI, because the offsets of
struct print_fulldescr_list.descriptors and of the fields num_transportid and
trnptid_list in struct prout_param_descriptor change.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmpathpersist/mpath_persist.h | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/libmpathpersist/mpath_persist.h b/libmpathpersist/mpath_persist.h
index 7cf4faf9..bdf4069d 100644
--- a/libmpathpersist/mpath_persist.h
+++ b/libmpathpersist/mpath_persist.h
@@ -92,7 +92,7 @@ struct prin_readdescr
 	uint32_t prgeneration;
 	uint32_t additional_length;	/* The value should be either 0 or divisible by 8.
 					   0 indicates no registered reservation key. */
-	uint8_t	 key_list[MPATH_MAX_PARAM_LEN];
+	uint8_t	 key_list[MPATH_MAX_PARAM_LEN - 2 * sizeof(uint32_t)];
 };
 
 struct prin_resvdescr
@@ -141,7 +141,9 @@ struct print_fulldescr_list
 {
 	uint32_t prgeneration;
 	uint32_t number_of_descriptor;
-	uint8_t private_buffer[MPATH_MAX_PARAM_LEN]; /*Private buffer for list storage*/
+	/* Data-in; Private buffer for list storage */
+	uint8_t private_buffer[MPATH_MAX_PARAM_LEN - 2 * sizeof(uint32_t)];
+	/* array of pointers into private_buffer */
 	struct prin_fulldescr *descriptors[];
 };
 
@@ -163,8 +165,13 @@ struct prout_param_descriptor {		/* PROUT parameter descriptor */
 	uint8_t	 sa_flags;
 	uint8_t _reserved;
 	uint16_t _obsolete1;
-	uint8_t  private_buffer[MPATH_MAX_PARAM_LEN]; /*private buffer for list storage*/
-	uint32_t num_transportid;	/* Number of Transport ID listed in trnptid_list[]*/
+	/*private buffer for list storage; data-out */
+	/* 24: offsetof (struct prout_param_descriptor, private_buffer) */
+	uint8_t  private_buffer[MPATH_MAX_PARAM_LEN - 24];
+	/* Internal use below here */
+	/* Number of Transport ID listed in trnptid_list[] */
+	uint32_t num_transportid;
+	/* pointers into private_buffer */
 	struct transportid *trnptid_list[];
 };
 
-- 
2.25.1

Re: [PATCH v2 2/4] libmpathpersist: format_transportids(): avoid PROUT overflow

[Benjamin Marzinski]

On Sun, Mar 15, 2020 at 10:05:19PM +0100, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> This limits the PERSISTENT RESERVE OUT data size to max. 8192 bytes.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmpathpersist/mpath_pr_ioctl.c | 31 +++++++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/libmpathpersist/mpath_pr_ioctl.c b/libmpathpersist/mpath_pr_ioctl.c
> index 1a28cba7..c78e8000 100644
> --- a/libmpathpersist/mpath_pr_ioctl.c
> +++ b/libmpathpersist/mpath_pr_ioctl.c
> @@ -1,5 +1,6 @@
>  #include <stdio.h>
>  #include <stdlib.h>
> +#include <stddef.h>
>  
>  #include <sys/types.h>
>  #include <sys/stat.h>
> @@ -138,38 +139,64 @@ retry :
>  	return status;
>  }
>  
> +/*
> + * Helper macro to avoid overflow of prout_param_descriptor in
> + * format_transportids(). Data must not be written past
> + * MPATH_MAX_PARAM_LEN bytes from struct prout_param_descriptor.
> + */
> +#define check_overflow(ofs, n, start, label)				\
> +	do {								\
> +		if ((ofs) + (n) +					\
> +		    offsetof(struct prout_param_descriptor, private_buffer) \
> +		    > MPATH_MAX_PARAM_LEN)				\
> +		{							\
> +			(ofs) = (start);				\
> +			goto label;					\
> +		}							\
> +	} while(0)
> +
>  uint32_t  format_transportids(struct prout_param_descriptor *paramp)
>  {
>  	unsigned int i = 0, len;
>  	uint32_t buff_offset = 4;
> -	memset(paramp->private_buffer, 0, MPATH_MAX_PARAM_LEN);
> +	memset(paramp->private_buffer, 0, sizeof(paramp->private_buffer));
>  	for (i=0; i < paramp->num_transportid; i++ )
>  	{
> +		uint32_t start_offset = buff_offset;
> +
> +		check_overflow(buff_offset, 1, start_offset, end_loop);
>  		paramp->private_buffer[buff_offset] = (uint8_t)((paramp->trnptid_list[i]->format_code & 0xff)|
>  							(paramp->trnptid_list[i]->protocol_id & 0xff));
>  		buff_offset += 1;
>  		switch(paramp->trnptid_list[i]->protocol_id)
>  		{
>  			case MPATH_PROTOCOL_ID_FC:
> +				check_overflow(buff_offset, 7 + 8 + 8,
> +					       start_offset, end_loop);
>  				buff_offset += 7;
>  				memcpy(&paramp->private_buffer[buff_offset], &paramp->trnptid_list[i]->n_port_name, 8);
>  				buff_offset +=8 ;
>  				buff_offset +=8 ;
>  				break;
>  			case MPATH_PROTOCOL_ID_SAS:
> +				check_overflow(buff_offset, 3 + 12,
> +					       start_offset, end_loop);
>  				buff_offset += 3;
>  				memcpy(&paramp->private_buffer[buff_offset], &paramp->trnptid_list[i]->sas_address, 8);
>  				buff_offset += 12;
>  				break;
>  			case MPATH_PROTOCOL_ID_ISCSI:
> -				buff_offset += 1;
>  				len = (paramp->trnptid_list[i]->iscsi_name[1] & 0xff)+2;
> +				check_overflow(buff_offset, 1 + len,
> +					       start_offset, end_loop);
> +				buff_offset += 1;
>  				memcpy(&paramp->private_buffer[buff_offset], &paramp->trnptid_list[i]->iscsi_name,len);
>  				buff_offset += len ;
>  				break;
>  		}
>  
>  	}
> +end_loop:
>  	buff_offset -= 4;
>  	paramp->private_buffer[0] = (unsigned char)((buff_offset >> 24) & 0xff);
>  	paramp->private_buffer[1] = (unsigned char)((buff_offset >> 16) & 0xff);
> -- 
> 2.25.1

Re: [PATCH v2 4/4] libmpathpersist: ABI change: limit data-in/out size to 8192 bytes

[Benjamin Marzinski]

On Sun, Mar 15, 2020 at 10:05:21PM +0100, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Make sure that data structures used for PERSISTENT RESERVE IN/OUT
> fit into 8k buffers.
> 
> This patch breaks the libmpathpersist ABI, because the offsets of
> struct print_fulldescr_list.descriptors and of the fields num_transportid and
> trnptid_list in struct prout_param_descriptor change.

I agree with the idea of holding off on this unless we actually bump the
version number on libmpathpersist

-Ben

> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmpathpersist/mpath_persist.h | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/libmpathpersist/mpath_persist.h b/libmpathpersist/mpath_persist.h
> index 7cf4faf9..bdf4069d 100644
> --- a/libmpathpersist/mpath_persist.h
> +++ b/libmpathpersist/mpath_persist.h
> @@ -92,7 +92,7 @@ struct prin_readdescr
>  	uint32_t prgeneration;
>  	uint32_t additional_length;	/* The value should be either 0 or divisible by 8.
>  					   0 indicates no registered reservation key. */
> -	uint8_t	 key_list[MPATH_MAX_PARAM_LEN];
> +	uint8_t	 key_list[MPATH_MAX_PARAM_LEN - 2 * sizeof(uint32_t)];
>  };
>  
>  struct prin_resvdescr
> @@ -141,7 +141,9 @@ struct print_fulldescr_list
>  {
>  	uint32_t prgeneration;
>  	uint32_t number_of_descriptor;
> -	uint8_t private_buffer[MPATH_MAX_PARAM_LEN]; /*Private buffer for list storage*/
> +	/* Data-in; Private buffer for list storage */
> +	uint8_t private_buffer[MPATH_MAX_PARAM_LEN - 2 * sizeof(uint32_t)];
> +	/* array of pointers into private_buffer */
>  	struct prin_fulldescr *descriptors[];
>  };
>  
> @@ -163,8 +165,13 @@ struct prout_param_descriptor {		/* PROUT parameter descriptor */
>  	uint8_t	 sa_flags;
>  	uint8_t _reserved;
>  	uint16_t _obsolete1;
> -	uint8_t  private_buffer[MPATH_MAX_PARAM_LEN]; /*private buffer for list storage*/
> -	uint32_t num_transportid;	/* Number of Transport ID listed in trnptid_list[]*/
> +	/*private buffer for list storage; data-out */
> +	/* 24: offsetof (struct prout_param_descriptor, private_buffer) */
> +	uint8_t  private_buffer[MPATH_MAX_PARAM_LEN - 24];
> +	/* Internal use below here */
> +	/* Number of Transport ID listed in trnptid_list[] */
> +	uint32_t num_transportid;
> +	/* pointers into private_buffer */
>  	struct transportid *trnptid_list[];
>  };
>  
> -- 
> 2.25.1