diff mbox series

[v4,3/7] target: consistently null-terminate t10_wwn.model

Message ID 20181129010157.12687-4-ddiss@suse.de (mailing list archive)
State Superseded
Headers show
Series target: user configurable T10 Vendor ID | expand

Commit Message

David Disseldorp Nov. 29, 2018, 1:01 a.m. UTC
The pscsi_set_inquiry_info() and emulate_model_alias_store() codepaths
don't currently explicitly null-terminate t10_wwn.model.
Add an extra byte to the t10_wwn.model buffer and perform null string
termination in all cases.

dev_set_t10_wwn_model_alias() continues to truncate at the same length
to avoid changing the model string for existing deployments.

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 drivers/target/target_core_configfs.c | 8 +++++---
 drivers/target/target_core_device.c   | 8 +++++---
 drivers/target/target_core_pscsi.c    | 6 ++++--
 drivers/target/target_core_spc.c      | 2 +-
 drivers/target/target_core_stat.c     | 4 ++--
 include/target/target_core_base.h     | 3 ++-
 6 files changed, 19 insertions(+), 12 deletions(-)

Comments

Ly, Bryant Nov. 29, 2018, 1:19 a.m. UTC | #1
> On Nov 28, 2018, at 7:01 PM, David Disseldorp <ddiss@suse.de> wrote:
> 
> The pscsi_set_inquiry_info() and emulate_model_alias_store() codepaths
> don't currently explicitly null-terminate t10_wwn.model.
> Add an extra byte to the t10_wwn.model buffer and perform null string
> termination in all cases.
> 
> dev_set_t10_wwn_model_alias() continues to truncate at the same length
> to avoid changing the model string for existing deployments.
> 
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> ---
> drivers/target/target_core_configfs.c | 8 +++++---
> drivers/target/target_core_device.c   | 8 +++++---
> drivers/target/target_core_pscsi.c    | 6 ++++--
> drivers/target/target_core_spc.c      | 2 +-
> drivers/target/target_core_stat.c     | 4 ++--
> include/target/target_core_base.h     | 3 ++-
> 6 files changed, 19 insertions(+), 12 deletions(-)
> 
> 

Reviewed-by: Bryant G. Ly bly@catalogicsoftware.com
Lee Duncan Nov. 29, 2018, 1:26 a.m. UTC | #2
On 11/28/18 5:01 PM, David Disseldorp wrote:
> The pscsi_set_inquiry_info() and emulate_model_alias_store() codepaths
> don't currently explicitly null-terminate t10_wwn.model.
> Add an extra byte to the t10_wwn.model buffer and perform null string
> termination in all cases.
> 
> dev_set_t10_wwn_model_alias() continues to truncate at the same length
> to avoid changing the model string for existing deployments.
> 
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> ---
>  drivers/target/target_core_configfs.c | 8 +++++---
>  drivers/target/target_core_device.c   | 8 +++++---
>  drivers/target/target_core_pscsi.c    | 6 ++++--
>  drivers/target/target_core_spc.c      | 2 +-
>  drivers/target/target_core_stat.c     | 4 ++--
>  include/target/target_core_base.h     | 3 ++-
>  6 files changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
> index f6b1549f4142..9f49b1afd685 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -613,12 +613,12 @@ static void dev_set_t10_wwn_model_alias(struct se_device *dev)
>  	const char *configname;
>  
>  	configname = config_item_name(&dev->dev_group.cg_item);
> -	if (strlen(configname) >= 16) {
> +	if (strlen(configname) >= INQUIRY_MODEL_LEN) {
>  		pr_warn("dev[%p]: Backstore name '%s' is too long for "
>  			"INQUIRY_MODEL, truncating to 16 bytes\n", dev,
>  			configname);
>  	}
> -	snprintf(&dev->t10_wwn.model[0], 16, "%s", configname);
> +	snprintf(&dev->t10_wwn.model[0], INQUIRY_MODEL_LEN, "%s", configname);
>  }
>  
>  static ssize_t emulate_model_alias_store(struct config_item *item,
> @@ -640,11 +640,13 @@ static ssize_t emulate_model_alias_store(struct config_item *item,
>  	if (ret < 0)
>  		return ret;
>  
> +	BUILD_BUG_ON(sizeof(dev->t10_wwn.model) != INQUIRY_MODEL_LEN + 1);
>  	if (flag) {
>  		dev_set_t10_wwn_model_alias(dev);
>  	} else {
>  		strncpy(&dev->t10_wwn.model[0],
> -			dev->transport->inquiry_prod, 16);
> +			dev->transport->inquiry_prod, INQUIRY_MODEL_LEN);
> +		dev->t10_wwn.model[INQUIRY_MODEL_LEN] = '\0';
>  	}
>  	da->emulate_model_alias = flag;
>  	return count;
> diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
> index fe4c4db51137..0d7382efb2d4 100644
> --- a/drivers/target/target_core_device.c
> +++ b/drivers/target/target_core_device.c
> @@ -720,7 +720,7 @@ void core_dev_free_initiator_node_lun_acl(
>  static void scsi_dump_inquiry(struct se_device *dev)
>  {
>  	struct t10_wwn *wwn = &dev->t10_wwn;
> -	char buf[17];
> +	char buf[INQUIRY_MODEL_LEN + 1];
>  	int i, device_type;
>  	/*
>  	 * Print Linux/SCSI style INQUIRY formatting to the kernel ring buffer
> @@ -733,7 +733,7 @@ static void scsi_dump_inquiry(struct se_device *dev)
>  	buf[i] = '\0';
>  	pr_debug("  Vendor: %s\n", buf);
>  
> -	for (i = 0; i < 16; i++)
> +	for (i = 0; i < INQUIRY_MODEL_LEN; i++)
>  		if (wwn->model[i] >= 0x20)
>  			buf[i] = wwn->model[i];
>  		else
> @@ -1009,11 +1009,13 @@ int target_configure_device(struct se_device *dev)
>  	 * passthrough because this is being provided by the backend LLD.
>  	 */
>  	BUILD_BUG_ON(sizeof(dev->t10_wwn.vendor) != INQUIRY_VENDOR_LEN + 1);
> +	BUILD_BUG_ON(sizeof(dev->t10_wwn.model) != INQUIRY_MODEL_LEN + 1);
>  	if (!(dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)) {
>  		strncpy(&dev->t10_wwn.vendor[0], "LIO-ORG", INQUIRY_VENDOR_LEN);
>  		dev->t10_wwn.vendor[INQUIRY_VENDOR_LEN] = '\0';
>  		strncpy(&dev->t10_wwn.model[0],
> -			dev->transport->inquiry_prod, 16);
> +			dev->transport->inquiry_prod, INQUIRY_MODEL_LEN);
> +		dev->t10_wwn.model[INQUIRY_MODEL_LEN] = '\0';
>  		strncpy(&dev->t10_wwn.revision[0],
>  			dev->transport->inquiry_rev, 4);
>  	}
> diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
> index ee65b5bb674c..1633babc2d4e 100644
> --- a/drivers/target/target_core_pscsi.c
> +++ b/drivers/target/target_core_pscsi.c
> @@ -193,7 +193,9 @@ pscsi_set_inquiry_info(struct scsi_device *sdev, struct t10_wwn *wwn)
>  	BUILD_BUG_ON(sizeof(wwn->vendor) != INQUIRY_VENDOR_LEN + 1);
>  	memcpy(&wwn->vendor[0], &buf[8], INQUIRY_VENDOR_LEN);
>  	wwn->vendor[INQUIRY_VENDOR_LEN] = '\0';
> -	memcpy(&wwn->model[0], &buf[16], sizeof(wwn->model));
> +	BUILD_BUG_ON(sizeof(wwn->model) != INQUIRY_MODEL_LEN + 1);
> +	memcpy(&wwn->model[0], &buf[16], INQUIRY_MODEL_LEN);
> +	wwn->model[INQUIRY_MODEL_LEN] = '\0';
>  	memcpy(&wwn->revision[0], &buf[32], sizeof(wwn->revision));
>  }
>  
> @@ -835,7 +837,7 @@ static ssize_t pscsi_show_configfs_dev_params(struct se_device *dev, char *b)
>  				bl += sprintf(b + bl, " ");
>  		}
>  		bl += sprintf(b + bl, " Model: ");
> -		for (i = 0; i < 16; i++) {
> +		for (i = 0; i < INQUIRY_MODEL_LEN; i++) {
>  			if (ISPRINT(sd->model[i]))   /* printable character ? */
>  				bl += sprintf(b + bl, "%c", sd->model[i]);
>  			else
> diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
> index c37dd36ec77d..78eddee4b6e6 100644
> --- a/drivers/target/target_core_spc.c
> +++ b/drivers/target/target_core_spc.c
> @@ -116,7 +116,7 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char *buf)
>  	memset(&buf[8], 0x20, 8 + 16 + 4);
>  	memcpy(&buf[8], "LIO-ORG", sizeof("LIO-ORG") - 1);
>  	memcpy(&buf[16], dev->t10_wwn.model,
> -	       strnlen(dev->t10_wwn.model, 16));
> +	       strnlen(dev->t10_wwn.model, INQUIRY_MODEL_LEN));
>  	memcpy(&buf[32], dev->t10_wwn.revision,
>  	       strnlen(dev->t10_wwn.revision, 4));
>  	buf[4] = 31; /* Set additional length to 31 */
> diff --git a/drivers/target/target_core_stat.c b/drivers/target/target_core_stat.c
> index 4210cf625d84..9123c5137da5 100644
> --- a/drivers/target/target_core_stat.c
> +++ b/drivers/target/target_core_stat.c
> @@ -261,10 +261,10 @@ static ssize_t target_stat_lu_prod_show(struct config_item *item, char *page)
>  {
>  	struct se_device *dev = to_stat_lu_dev(item);
>  	int i;
> -	char str[sizeof(dev->t10_wwn.model)+1];
> +	char str[INQUIRY_MODEL_LEN+1];
>  
>  	/* scsiLuProductId */
> -	for (i = 0; i < sizeof(dev->t10_wwn.model); i++)
> +	for (i = 0; i < INQUIRY_MODEL_LEN; i++)
>  		str[i] = ISPRINT(dev->t10_wwn.model[i]) ?
>  			dev->t10_wwn.model[i] : ' ';
>  	str[i] = '\0';
> diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
> index cb1f3f574e2a..cfc279686cf4 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -47,6 +47,7 @@
>  #define INQUIRY_VPD_DEVICE_IDENTIFIER_LEN	254
>  
>  #define INQUIRY_VENDOR_LEN			8
> +#define INQUIRY_MODEL_LEN			16
>  
>  /* Attempts before moving from SHORT to LONG */
>  #define PYX_TRANSPORT_WINDOW_CLOSED_THRESHOLD	3
> @@ -321,7 +322,7 @@ struct t10_wwn {
>  	 * null terminator is always present.
>  	 */
>  	char vendor[INQUIRY_VENDOR_LEN + 1];
> -	char model[16];
> +	char model[INQUIRY_MODEL_LEN + 1];
>  	char revision[4];
>  	char unit_serial[INQUIRY_VPD_SERIAL_LEN];
>  	spinlock_t t10_vpd_lock;
> 


Reviewed-by: Lee Duncan <lduncan@suse.com>
Bart Van Assche Nov. 29, 2018, 4:24 p.m. UTC | #3
On Thu, 2018-11-29 at 02:01 +0100, David Disseldorp wrote:
> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
> index f6b1549f4142..9f49b1afd685 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -613,12 +613,12 @@ static void dev_set_t10_wwn_model_alias(struct se_device *dev)
>  	const char *configname;
>  
>  	configname = config_item_name(&dev->dev_group.cg_item);
> -	if (strlen(configname) >= 16) {
> +	if (strlen(configname) >= INQUIRY_MODEL_LEN) {
>  		pr_warn("dev[%p]: Backstore name '%s' is too long for "
>  			"INQUIRY_MODEL, truncating to 16 bytes\n", dev,
>  			configname);
>  	}
> -	snprintf(&dev->t10_wwn.model[0], 16, "%s", configname);
> +	snprintf(&dev->t10_wwn.model[0], INQUIRY_MODEL_LEN, "%s", configname);

Both the old and the new statement truncate inquiry strings that are 16 bytes
long, which is a bug. Additionally, have you considered to use strlcpy()
instead of snprintf()?

>  		strncpy(&dev->t10_wwn.model[0],
> -			dev->transport->inquiry_prod, 16);
> +			dev->transport->inquiry_prod, INQUIRY_MODEL_LEN);
> +		dev->t10_wwn.model[INQUIRY_MODEL_LEN] = '\0';

Have you considered to use strlcpy() instead of strncpy() followed by explicit
'\0'-termination?

>  		strncpy(&dev->t10_wwn.model[0],
> -			dev->transport->inquiry_prod, 16);
> +			dev->transport->inquiry_prod, INQUIRY_MODEL_LEN);
> +		dev->t10_wwn.model[INQUIRY_MODEL_LEN] = '\0';

Same question here: have you considered to use strlcpy() instead of strncpy()
followed by explicit '\0'-termination?

> -	memcpy(&wwn->model[0], &buf[16], sizeof(wwn->model));
> +	BUILD_BUG_ON(sizeof(wwn->model) != INQUIRY_MODEL_LEN + 1);
> +	memcpy(&wwn->model[0], &buf[16], INQUIRY_MODEL_LEN);
> +	wwn->model[INQUIRY_MODEL_LEN] = '\0';

Can the memcpy() and '\0'-termination be changed into an snprintf(..., "%.*s", ...)
call?

Thanks,

Bart.
David Disseldorp Nov. 29, 2018, 8:31 p.m. UTC | #4
On Thu, 29 Nov 2018 08:24:38 -0800, Bart Van Assche wrote:

> On Thu, 2018-11-29 at 02:01 +0100, David Disseldorp wrote:
> > diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
> > index f6b1549f4142..9f49b1afd685 100644
> > --- a/drivers/target/target_core_configfs.c
> > +++ b/drivers/target/target_core_configfs.c
> > @@ -613,12 +613,12 @@ static void dev_set_t10_wwn_model_alias(struct se_device *dev)
> >  	const char *configname;
> >  
> >  	configname = config_item_name(&dev->dev_group.cg_item);
> > -	if (strlen(configname) >= 16) {
> > +	if (strlen(configname) >= INQUIRY_MODEL_LEN) {
> >  		pr_warn("dev[%p]: Backstore name '%s' is too long for "
> >  			"INQUIRY_MODEL, truncating to 16 bytes\n", dev,
> >  			configname);
> >  	}
> > -	snprintf(&dev->t10_wwn.model[0], 16, "%s", configname);
> > +	snprintf(&dev->t10_wwn.model[0], INQUIRY_MODEL_LEN, "%s", configname);  
> 
> Both the old and the new statement truncate inquiry strings that are 16 bytes
> long, which is a bug.

As mentioned in the changelog, I don't think we can fix this without
potentially breaking existing deployments - e.g. a "fourfourfourfour"
backstore name with emulate_model_alias=1 would change inquiry product
ID from "fourfourfourfou" to "fourfourfourfour" following kernel
upgrade.

> Additionally, have you considered to use strlcpy()
> instead of snprintf()?

Happy to change the logic below over if you find it easier to follow.

Cheers, David
Bart Van Assche Nov. 29, 2018, 8:46 p.m. UTC | #5
On Thu, 2018-11-29 at 21:31 +0100, David Disseldorp wrote:
> On Thu, 29 Nov 2018 08:24:38 -0800, Bart Van Assche wrote:
> > On Thu, 2018-11-29 at 02:01 +0100, David Disseldorp wrote:
> > > [ ... ]
> > Additionally, have you considered to use strlcpy()
> > instead of snprintf()?
> 
> Happy to change the logic below over if you find it easier to follow.

It would make the code shorter without hurting readability, so I think
it would be better to use strlcpy(). But it's not that important to me.

Bart.
diff mbox series

Patch

diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index f6b1549f4142..9f49b1afd685 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -613,12 +613,12 @@  static void dev_set_t10_wwn_model_alias(struct se_device *dev)
 	const char *configname;
 
 	configname = config_item_name(&dev->dev_group.cg_item);
-	if (strlen(configname) >= 16) {
+	if (strlen(configname) >= INQUIRY_MODEL_LEN) {
 		pr_warn("dev[%p]: Backstore name '%s' is too long for "
 			"INQUIRY_MODEL, truncating to 16 bytes\n", dev,
 			configname);
 	}
-	snprintf(&dev->t10_wwn.model[0], 16, "%s", configname);
+	snprintf(&dev->t10_wwn.model[0], INQUIRY_MODEL_LEN, "%s", configname);
 }
 
 static ssize_t emulate_model_alias_store(struct config_item *item,
@@ -640,11 +640,13 @@  static ssize_t emulate_model_alias_store(struct config_item *item,
 	if (ret < 0)
 		return ret;
 
+	BUILD_BUG_ON(sizeof(dev->t10_wwn.model) != INQUIRY_MODEL_LEN + 1);
 	if (flag) {
 		dev_set_t10_wwn_model_alias(dev);
 	} else {
 		strncpy(&dev->t10_wwn.model[0],
-			dev->transport->inquiry_prod, 16);
+			dev->transport->inquiry_prod, INQUIRY_MODEL_LEN);
+		dev->t10_wwn.model[INQUIRY_MODEL_LEN] = '\0';
 	}
 	da->emulate_model_alias = flag;
 	return count;
diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index fe4c4db51137..0d7382efb2d4 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -720,7 +720,7 @@  void core_dev_free_initiator_node_lun_acl(
 static void scsi_dump_inquiry(struct se_device *dev)
 {
 	struct t10_wwn *wwn = &dev->t10_wwn;
-	char buf[17];
+	char buf[INQUIRY_MODEL_LEN + 1];
 	int i, device_type;
 	/*
 	 * Print Linux/SCSI style INQUIRY formatting to the kernel ring buffer
@@ -733,7 +733,7 @@  static void scsi_dump_inquiry(struct se_device *dev)
 	buf[i] = '\0';
 	pr_debug("  Vendor: %s\n", buf);
 
-	for (i = 0; i < 16; i++)
+	for (i = 0; i < INQUIRY_MODEL_LEN; i++)
 		if (wwn->model[i] >= 0x20)
 			buf[i] = wwn->model[i];
 		else
@@ -1009,11 +1009,13 @@  int target_configure_device(struct se_device *dev)
 	 * passthrough because this is being provided by the backend LLD.
 	 */
 	BUILD_BUG_ON(sizeof(dev->t10_wwn.vendor) != INQUIRY_VENDOR_LEN + 1);
+	BUILD_BUG_ON(sizeof(dev->t10_wwn.model) != INQUIRY_MODEL_LEN + 1);
 	if (!(dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)) {
 		strncpy(&dev->t10_wwn.vendor[0], "LIO-ORG", INQUIRY_VENDOR_LEN);
 		dev->t10_wwn.vendor[INQUIRY_VENDOR_LEN] = '\0';
 		strncpy(&dev->t10_wwn.model[0],
-			dev->transport->inquiry_prod, 16);
+			dev->transport->inquiry_prod, INQUIRY_MODEL_LEN);
+		dev->t10_wwn.model[INQUIRY_MODEL_LEN] = '\0';
 		strncpy(&dev->t10_wwn.revision[0],
 			dev->transport->inquiry_rev, 4);
 	}
diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
index ee65b5bb674c..1633babc2d4e 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -193,7 +193,9 @@  pscsi_set_inquiry_info(struct scsi_device *sdev, struct t10_wwn *wwn)
 	BUILD_BUG_ON(sizeof(wwn->vendor) != INQUIRY_VENDOR_LEN + 1);
 	memcpy(&wwn->vendor[0], &buf[8], INQUIRY_VENDOR_LEN);
 	wwn->vendor[INQUIRY_VENDOR_LEN] = '\0';
-	memcpy(&wwn->model[0], &buf[16], sizeof(wwn->model));
+	BUILD_BUG_ON(sizeof(wwn->model) != INQUIRY_MODEL_LEN + 1);
+	memcpy(&wwn->model[0], &buf[16], INQUIRY_MODEL_LEN);
+	wwn->model[INQUIRY_MODEL_LEN] = '\0';
 	memcpy(&wwn->revision[0], &buf[32], sizeof(wwn->revision));
 }
 
@@ -835,7 +837,7 @@  static ssize_t pscsi_show_configfs_dev_params(struct se_device *dev, char *b)
 				bl += sprintf(b + bl, " ");
 		}
 		bl += sprintf(b + bl, " Model: ");
-		for (i = 0; i < 16; i++) {
+		for (i = 0; i < INQUIRY_MODEL_LEN; i++) {
 			if (ISPRINT(sd->model[i]))   /* printable character ? */
 				bl += sprintf(b + bl, "%c", sd->model[i]);
 			else
diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index c37dd36ec77d..78eddee4b6e6 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -116,7 +116,7 @@  spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char *buf)
 	memset(&buf[8], 0x20, 8 + 16 + 4);
 	memcpy(&buf[8], "LIO-ORG", sizeof("LIO-ORG") - 1);
 	memcpy(&buf[16], dev->t10_wwn.model,
-	       strnlen(dev->t10_wwn.model, 16));
+	       strnlen(dev->t10_wwn.model, INQUIRY_MODEL_LEN));
 	memcpy(&buf[32], dev->t10_wwn.revision,
 	       strnlen(dev->t10_wwn.revision, 4));
 	buf[4] = 31; /* Set additional length to 31 */
diff --git a/drivers/target/target_core_stat.c b/drivers/target/target_core_stat.c
index 4210cf625d84..9123c5137da5 100644
--- a/drivers/target/target_core_stat.c
+++ b/drivers/target/target_core_stat.c
@@ -261,10 +261,10 @@  static ssize_t target_stat_lu_prod_show(struct config_item *item, char *page)
 {
 	struct se_device *dev = to_stat_lu_dev(item);
 	int i;
-	char str[sizeof(dev->t10_wwn.model)+1];
+	char str[INQUIRY_MODEL_LEN+1];
 
 	/* scsiLuProductId */
-	for (i = 0; i < sizeof(dev->t10_wwn.model); i++)
+	for (i = 0; i < INQUIRY_MODEL_LEN; i++)
 		str[i] = ISPRINT(dev->t10_wwn.model[i]) ?
 			dev->t10_wwn.model[i] : ' ';
 	str[i] = '\0';
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index cb1f3f574e2a..cfc279686cf4 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -47,6 +47,7 @@ 
 #define INQUIRY_VPD_DEVICE_IDENTIFIER_LEN	254
 
 #define INQUIRY_VENDOR_LEN			8
+#define INQUIRY_MODEL_LEN			16
 
 /* Attempts before moving from SHORT to LONG */
 #define PYX_TRANSPORT_WINDOW_CLOSED_THRESHOLD	3
@@ -321,7 +322,7 @@  struct t10_wwn {
 	 * null terminator is always present.
 	 */
 	char vendor[INQUIRY_VENDOR_LEN + 1];
-	char model[16];
+	char model[INQUIRY_MODEL_LEN + 1];
 	char revision[4];
 	char unit_serial[INQUIRY_VPD_SERIAL_LEN];
 	spinlock_t t10_vpd_lock;