* [PATCH v4 3/7] target: consistently null-terminate t10_wwn.model
@ 2018-11-29 1:01 David Disseldorp
2018-11-29 1:19 ` Ly, Bryant
` (4 more replies)
0 siblings, 5 replies; 6+ messages in thread
From: David Disseldorp @ 2018-11-29 1:01 UTC (permalink / raw)
To: target-devel
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;
--
2.13.7
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v4 3/7] target: consistently null-terminate t10_wwn.model
2018-11-29 1:01 [PATCH v4 3/7] target: consistently null-terminate t10_wwn.model David Disseldorp
@ 2018-11-29 1:19 ` Ly, Bryant
2018-11-29 1:26 ` Lee Duncan
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Ly, Bryant @ 2018-11-29 1:19 UTC (permalink / raw)
To: target-devel
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v4 3/7] target: consistently null-terminate t10_wwn.model
2018-11-29 1:01 [PATCH v4 3/7] target: consistently null-terminate t10_wwn.model David Disseldorp
2018-11-29 1:19 ` Ly, Bryant
@ 2018-11-29 1:26 ` Lee Duncan
2018-11-29 16:24 ` Bart Van Assche
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Lee Duncan @ 2018-11-29 1:26 UTC (permalink / raw)
To: target-devel
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>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v4 3/7] target: consistently null-terminate t10_wwn.model
2018-11-29 1:01 [PATCH v4 3/7] target: consistently null-terminate t10_wwn.model David Disseldorp
2018-11-29 1:19 ` Ly, Bryant
2018-11-29 1:26 ` Lee Duncan
@ 2018-11-29 16:24 ` Bart Van Assche
2018-11-29 20:31 ` David Disseldorp
2018-11-29 20:46 ` Bart Van Assche
4 siblings, 0 replies; 6+ messages in thread
From: Bart Van Assche @ 2018-11-29 16:24 UTC (permalink / raw)
To: target-devel
On Thu, 2018-11-29 at 02:01 +-0100, David Disseldorp wrote:
+AD4 diff --git a/drivers/target/target+AF8-core+AF8-configfs.c b/drivers/target/target+AF8-core+AF8-configfs.c
+AD4 index f6b1549f4142..9f49b1afd685 100644
+AD4 --- a/drivers/target/target+AF8-core+AF8-configfs.c
+AD4 +-+-+- b/drivers/target/target+AF8-core+AF8-configfs.c
+AD4 +AEAAQA -613,12 +-613,12 +AEAAQA static void dev+AF8-set+AF8-t10+AF8-wwn+AF8-model+AF8-alias(struct se+AF8-device +ACo-dev)
+AD4 const char +ACo-configname+ADs
+AD4
+AD4 configname +AD0 config+AF8-item+AF8-name(+ACY-dev-+AD4-dev+AF8-group.cg+AF8-item)+ADs
+AD4 - if (strlen(configname) +AD4APQ 16) +AHs
+AD4 +- if (strlen(configname) +AD4APQ INQUIRY+AF8-MODEL+AF8-LEN) +AHs
+AD4 pr+AF8-warn(+ACI-dev+AFsAJQ-p+AF0: Backstore name '+ACU-s' is too long for +ACI
+AD4 +ACI-INQUIRY+AF8-MODEL, truncating to 16 bytes+AFw-n+ACI, dev,
+AD4 configname)+ADs
+AD4 +AH0
+AD4 - snprintf(+ACY-dev-+AD4-t10+AF8-wwn.model+AFs-0+AF0, 16, +ACIAJQ-s+ACI, configname)+ADs
+AD4 +- snprintf(+ACY-dev-+AD4-t10+AF8-wwn.model+AFs-0+AF0, INQUIRY+AF8-MODEL+AF8-LEN, +ACIAJQ-s+ACI, configname)+ADs
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()?
+AD4 strncpy(+ACY-dev-+AD4-t10+AF8-wwn.model+AFs-0+AF0,
+AD4 - dev-+AD4-transport-+AD4-inquiry+AF8-prod, 16)+ADs
+AD4 +- dev-+AD4-transport-+AD4-inquiry+AF8-prod, INQUIRY+AF8-MODEL+AF8-LEN)+ADs
+AD4 +- dev-+AD4-t10+AF8-wwn.model+AFs-INQUIRY+AF8-MODEL+AF8-LEN+AF0 +AD0 '+AFw-0'+ADs
Have you considered to use strlcpy() instead of strncpy() followed by explicit
'+AFw-0'-termination?
+AD4 strncpy(+ACY-dev-+AD4-t10+AF8-wwn.model+AFs-0+AF0,
+AD4 - dev-+AD4-transport-+AD4-inquiry+AF8-prod, 16)+ADs
+AD4 +- dev-+AD4-transport-+AD4-inquiry+AF8-prod, INQUIRY+AF8-MODEL+AF8-LEN)+ADs
+AD4 +- dev-+AD4-t10+AF8-wwn.model+AFs-INQUIRY+AF8-MODEL+AF8-LEN+AF0 +AD0 '+AFw-0'+ADs
Same question here: have you considered to use strlcpy() instead of strncpy()
followed by explicit '+AFw-0'-termination?
+AD4 - memcpy(+ACY-wwn-+AD4-model+AFs-0+AF0, +ACY-buf+AFs-16+AF0, sizeof(wwn-+AD4-model))+ADs
+AD4 +- BUILD+AF8-BUG+AF8-ON(sizeof(wwn-+AD4-model) +ACEAPQ INQUIRY+AF8-MODEL+AF8-LEN +- 1)+ADs
+AD4 +- memcpy(+ACY-wwn-+AD4-model+AFs-0+AF0, +ACY-buf+AFs-16+AF0, INQUIRY+AF8-MODEL+AF8-LEN)+ADs
+AD4 +- wwn-+AD4-model+AFs-INQUIRY+AF8-MODEL+AF8-LEN+AF0 +AD0 '+AFw-0'+ADs
Can the memcpy() and '+AFw-0'-termination be changed into an snprintf(..., +ACIAJQ.+ACo-s+ACI, ...)
call?
Thanks,
Bart.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 3/7] target: consistently null-terminate t10_wwn.model
2018-11-29 1:01 [PATCH v4 3/7] target: consistently null-terminate t10_wwn.model David Disseldorp
` (2 preceding siblings ...)
2018-11-29 16:24 ` Bart Van Assche
@ 2018-11-29 20:31 ` David Disseldorp
2018-11-29 20:46 ` Bart Van Assche
4 siblings, 0 replies; 6+ messages in thread
From: David Disseldorp @ 2018-11-29 20:31 UTC (permalink / raw)
To: target-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v4 3/7] target: consistently null-terminate t10_wwn.model
2018-11-29 1:01 [PATCH v4 3/7] target: consistently null-terminate t10_wwn.model David Disseldorp
` (3 preceding siblings ...)
2018-11-29 20:31 ` David Disseldorp
@ 2018-11-29 20:46 ` Bart Van Assche
4 siblings, 0 replies; 6+ messages in thread
From: Bart Van Assche @ 2018-11-29 20:46 UTC (permalink / raw)
To: target-devel
On Thu, 2018-11-29 at 21:31 +-0100, David Disseldorp wrote:
+AD4 On Thu, 29 Nov 2018 08:24:38 -0800, Bart Van Assche wrote:
+AD4 +AD4 On Thu, 2018-11-29 at 02:01 +-0100, David Disseldorp wrote:
+AD4 +AD4 +AD4 +AFs ... +AF0
+AD4 +AD4 Additionally, have you considered to use strlcpy()
+AD4 +AD4 instead of snprintf()?
+AD4
+AD4 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-11-29 20:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-11-29 1:01 [PATCH v4 3/7] target: consistently null-terminate t10_wwn.model David Disseldorp
2018-11-29 1:19 ` Ly, Bryant
2018-11-29 1:26 ` Lee Duncan
2018-11-29 16:24 ` Bart Van Assche
2018-11-29 20:31 ` David Disseldorp
2018-11-29 20:46 ` Bart Van Assche
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.