| Message ID | 1530045316-22239-1-git-send-email-leitao@debian.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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) >
Breno, > 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; Applied to 4.19/scsi-queue, thanks.
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)
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> --- drivers/scsi/ibmvscsi/ibmvscsi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)