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.
