[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-scsi
Subject:    Re: [PATCH] scsi: ibmvscsi: Improve strings handling
From:       Tyrel Datwyler <tyreld () linux ! vnet ! ibm ! com>
Date:       2018-06-26 22:29:15
Message-ID: abbca8ad-6b59-e701-91d5-a3a7ebb0e31d () linux ! vnet ! ibm ! com
[Download RAW message or body]

On 06/26/2018 01:35 PM, Breno Leitao wrote:

The subject line should have been updated to [PATCH v2] to clue recipients to the \
fact that this is an updated version and not a resend or accidental duplicate send.

> Currently an open firmware property is copied into partition_name variable
> without keeping a room for \0.
> 
> Later one, this variable (partition_name), which is 97 bytes long, is
> strncpyed into ibmvcsci_host_data->madapter_info->partition_name, which
> is 96 bytes long, possibly truncating it 'again' and removing the \0.
> 
> This patch simply decreases the partition name to 96 and just copy using
> strlcpy() which guarantees that the string is \0 terminated. I think there
> is no issue if this there is a truncation in this very first copy, i.e,
> when the open firmware property is read and copied into the driver for the
> very first time;
> 
> This issue also causes the following warning on GCC 8:
> 
> 	drivers/scsi/ibmvscsi/ibmvscsi.c:281:2: warning: ‘strncpy' output may be \
>                 truncated copying 96 bytes from a string of length 96 \
>                 [-Wstringop-truncation]
> 	...
> 	inlined from ‘ibmvscsi_probe' at drivers/scsi/ibmvscsi/ibmvscsi.c:2221:7:
> 	drivers/scsi/ibmvscsi/ibmvscsi.c:265:3: warning: ‘strncpy' specified bound 97 \
> equals destination size [-Wstringop-truncation] 
> CC: Bart Van Assche <bart.vanassche@wdc.com>
> CC: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---

Also, it is generally recommended that you record your revision history here for the \
readers/reviewers to quickly see what changed, and to make sure once the patch is \
pulled this info isn't included in the commit log.

ie.

Changes in v2:
- Addressed Bart's comment by replacing strncpy() with strlcpy()


Otherwise,

Acked-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>

> drivers/scsi/ibmvscsi/ibmvscsi.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
> index 17df76f0be3c..67a2c844e30d 100644
> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
> @@ -93,7 +93,7 @@ static int max_requests = IBMVSCSI_MAX_REQUESTS_DEFAULT;
> static int max_events = IBMVSCSI_MAX_REQUESTS_DEFAULT + 2;
> static int fast_fail = 1;
> static int client_reserve = 1;
> -static char partition_name[97] = "UNKNOWN";
> +static char partition_name[96] = "UNKNOWN";
> static unsigned int partition_number = -1;
> static LIST_HEAD(ibmvscsi_head);
> 
> @@ -262,7 +262,7 @@ static void gather_partition_info(void)
> 
> 	ppartition_name = of_get_property(of_root, "ibm,partition-name", NULL);
> 	if (ppartition_name)
> -		strncpy(partition_name, ppartition_name,
> +		strlcpy(partition_name, ppartition_name,
> 				sizeof(partition_name));
> 	p_number_ptr = of_get_property(of_root, "ibm,partition-no", NULL);
> 	if (p_number_ptr)
> 


[prev in list] [next in list] [prev in thread] [next in thread]