Re: [PATCH] bfa: avoid buffer overrun for 12-byte model name

[Jim Meyering <jim@meyering.net>]

Krishna Gudipati wrote:
>> -----Original Message-----
>> From: Jim Meyering [mailto:jim@meyering.net]
>> Sent: Monday, August 20, 2012 9:55 AM
>> To: linux-kernel@vger.kernel.org
>> Cc: Jim Meyering; Jing Huang; Krishna Gudipati; James E.J. Bottomley; linux-
>> scsi@vger.kernel.org
>> Subject: [PATCH] bfa: avoid buffer overrun for 12-byte model name
>>
>> From: Jim Meyering <meyering@redhat.com>
>>
>> we use strncpy to copy a model name of length up to 15 (16, if you count the
>> NUL), into a buffer of size 12 (BFA_FCS_PORT_SYMBNAME_MODEL_SZ).
>> However, strncpy does not always NUL-terminate, so whenever the original
>> model string has strlen >= 12, the following strncat reads beyond end of the -
>> >sym_name buffer as it attempts to find end of string.
>>
>> bfa_fcs_fabric_psymb_init(struct bfa_fcs_fabric_s *fabric) {
>> 	bfa_ioc_get_adapter_model(&fabric->fcs->bfa->ioc, model);
>> 	...
>> 	strncpy((char *)&port_cfg->sym_name, model,
>> 		BFA_FCS_PORT_SYMBNAME_MODEL_SZ);
>> 	strncat((char *)&port_cfg->sym_name,
>> BFA_FCS_PORT_SYMBNAME_SEPARATOR,
>> 		sizeof(BFA_FCS_PORT_SYMBNAME_SEPARATOR));
>> 	...
>>
>> bfa_ioc_get_adapter_model(struct bfa_ioc_s *ioc, char *model) {
>> 	struct bfi_ioc_attr_s	*ioc_attr;
>>
>> 	WARN_ON(!model);
>> 	memset((void *)model, 0, BFA_ADAPTER_MODEL_NAME_LEN);
>>
>> BFA_ADAPTER_MODEL_NAME_LEN = 16
>>
>> Signed-off-by: Jim Meyering <meyering@redhat.com>
>> ---
>>  drivers/scsi/bfa/bfa_fcs.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/scsi/bfa/bfa_fcs.c b/drivers/scsi/bfa/bfa_fcs.c index
>> eaac57e..3329493 100644
>> --- a/drivers/scsi/bfa/bfa_fcs.c
>> +++ b/drivers/scsi/bfa/bfa_fcs.c
>> @@ -713,6 +713,7 @@ bfa_fcs_fabric_psymb_init(struct bfa_fcs_fabric_s
>> *fabric)
>>  	/* Model name/number */
>>  	strncpy((char *)&port_cfg->sym_name, model,
>>  		BFA_FCS_PORT_SYMBNAME_MODEL_SZ);
>> +	port_cfg->sym_name[BFA_FCS_PORT_SYMBNAME_MODEL_SZ - 1]
>> = 0;
>>  	strncat((char *)&port_cfg->sym_name,
>> BFA_FCS_PORT_SYMBNAME_SEPARATOR,
>>  		sizeof(BFA_FCS_PORT_SYMBNAME_SEPARATOR));
>
> Nacked-by: Krishna Gudipati <kgudipat@brocade.com>
>
> Hi Jim,
>
> This model number is of length 12 bytes and the logic added here will
> reset the model last byte.
> In addition strncat does not need the src to be null terminated, the
> change does not compile even.
> NACK to this change.

Hi Krishna,

Thanks for the quick feedback and sorry the patch wasn't quite right.
However, the log is accurate: there is at least a theoretical problem
when the string in "model" (a buffer of size 16 bytes) has strlen >= 12.
While strncat does not require that its second argument be NUL-terminated,
the first one (the destination) must be.  Otherwise, it has no way to
determine the end of the string to which it must append the source bytes.

Here is a v2 patch to which I've added the requisite (char*) cast.
However, this whole function is rather unreadable due to the
repetition (12 times!) of "(char *)&port_cfg->sym_name".
In case someone prefers to factor out that repetition,
I've appended a larger, v3 patch to do that.

>From 4d1ce4e5caf8a5041e5c4f3ae4deddb79c9e247c Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@redhat.com>
Date: Sun, 29 Apr 2012 10:41:05 +0200
Subject: [PATCHv2] bfa: avoid buffer overrun for 12-byte model name

we use strncpy to copy a model name of length up to 15 (16, if you count
the NUL), into a buffer of size 12 (BFA_FCS_PORT_SYMBNAME_MODEL_SZ).
However, strncpy does not always NUL-terminate, so whenever the original
model string has strlen >= 12, the following strncat reads beyond end
of the ->sym_name buffer as it attempts to find end of string.

bfa_fcs_fabric_psymb_init(struct bfa_fcs_fabric_s *fabric)
{
	bfa_ioc_get_adapter_model(&fabric->fcs->bfa->ioc, model);
	...
	strncpy((char *)&port_cfg->sym_name, model,
		BFA_FCS_PORT_SYMBNAME_MODEL_SZ);
	strncat((char *)&port_cfg->sym_name, BFA_FCS_PORT_SYMBNAME_SEPARATOR,
		sizeof(BFA_FCS_PORT_SYMBNAME_SEPARATOR));
	...

bfa_ioc_get_adapter_model(struct bfa_ioc_s *ioc, char *model)
{
	struct bfi_ioc_attr_s	*ioc_attr;

	WARN_ON(!model);
	memset((void *)model, 0, BFA_ADAPTER_MODEL_NAME_LEN);

BFA_ADAPTER_MODEL_NAME_LEN = 16

Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 drivers/scsi/bfa/bfa_fcs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/bfa/bfa_fcs.c b/drivers/scsi/bfa/bfa_fcs.c
index eaac57e..242c37f 100644
--- a/drivers/scsi/bfa/bfa_fcs.c
+++ b/drivers/scsi/bfa/bfa_fcs.c
@@ -713,6 +713,7 @@ bfa_fcs_fabric_psymb_init(struct bfa_fcs_fabric_s *fabric)
 	/* Model name/number */
 	strncpy((char *)&port_cfg->sym_name, model,
 		BFA_FCS_PORT_SYMBNAME_MODEL_SZ);
+	((char *)&port_cfg->sym_name)[BFA_FCS_PORT_SYMBNAME_MODEL_SZ - 1] = 0;
 	strncat((char *)&port_cfg->sym_name, BFA_FCS_PORT_SYMBNAME_SEPARATOR,
 		sizeof(BFA_FCS_PORT_SYMBNAME_SEPARATOR));

--
1.7.12


>From d7f49a0a2f835ec4808772678fe6c4d595ffa8f5 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@redhat.com>
Date: Sun, 29 Apr 2012 10:41:05 +0200
Subject: [PATCHv3] bfa: avoid buffer overrun for 12-byte model name

we use strncpy to copy a model name of length up to 15 (16, if you count
the NUL), into a buffer of size 12 (BFA_FCS_PORT_SYMBNAME_MODEL_SZ).
However, strncpy does not always NUL-terminate, so whenever the original
model string has strlen >= 12, the following strncat reads beyond end
of the ->sym_name buffer as it attempts to find end of string.
Also, factor out the 12 uses of (char *)&port_cfg->sym_name.

bfa_fcs_fabric_psymb_init(struct bfa_fcs_fabric_s *fabric)
{
	bfa_ioc_get_adapter_model(&fabric->fcs->bfa->ioc, model);
	...
	strncpy((char *)&port_cfg->sym_name, model,
		BFA_FCS_PORT_SYMBNAME_MODEL_SZ);
	strncat((char *)&port_cfg->sym_name, BFA_FCS_PORT_SYMBNAME_SEPARATOR,
		sizeof(BFA_FCS_PORT_SYMBNAME_SEPARATOR));
	...

bfa_ioc_get_adapter_model(struct bfa_ioc_s *ioc, char *model)
{
	struct bfi_ioc_attr_s	*ioc_attr;

	WARN_ON(!model);
	memset((void *)model, 0, BFA_ADAPTER_MODEL_NAME_LEN);

BFA_ADAPTER_MODEL_NAME_LEN = 16

Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 drivers/scsi/bfa/bfa_fcs.c | 31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

diff --git a/drivers/scsi/bfa/bfa_fcs.c b/drivers/scsi/bfa/bfa_fcs.c
index eaac57e..77d08f9 100644
--- a/drivers/scsi/bfa/bfa_fcs.c
+++ b/drivers/scsi/bfa/bfa_fcs.c
@@ -707,26 +707,26 @@ bfa_fcs_fabric_psymb_init(struct bfa_fcs_fabric_s *fabric)
 	struct bfa_lport_cfg_s *port_cfg = &fabric->bport.port_cfg;
 	char model[BFA_ADAPTER_MODEL_NAME_LEN] = {0};
 	struct bfa_fcs_driver_info_s *driver_info = &fabric->fcs->driver_info;
+	char *s_name = (char *)&port_cfg->sym_name;

 	bfa_ioc_get_adapter_model(&fabric->fcs->bfa->ioc, model);

 	/* Model name/number */
-	strncpy((char *)&port_cfg->sym_name, model,
-		BFA_FCS_PORT_SYMBNAME_MODEL_SZ);
-	strncat((char *)&port_cfg->sym_name, BFA_FCS_PORT_SYMBNAME_SEPARATOR,
+	strncpy(s_name, model, BFA_FCS_PORT_SYMBNAME_MODEL_SZ);
+	s_name[BFA_FCS_PORT_SYMBNAME_MODEL_SZ - 1] = 0;
+	strncat(s_name, BFA_FCS_PORT_SYMBNAME_SEPARATOR,
 		sizeof(BFA_FCS_PORT_SYMBNAME_SEPARATOR));

 	/* Driver Version */
-	strncat((char *)&port_cfg->sym_name, (char *)driver_info->version,
+	strncat(s_name, (char *)driver_info->version,
 		BFA_FCS_PORT_SYMBNAME_VERSION_SZ);
-	strncat((char *)&port_cfg->sym_name, BFA_FCS_PORT_SYMBNAME_SEPARATOR,
+	strncat(s_name, BFA_FCS_PORT_SYMBNAME_SEPARATOR,
 		sizeof(BFA_FCS_PORT_SYMBNAME_SEPARATOR));

 	/* Host machine name */
-	strncat((char *)&port_cfg->sym_name,
-		(char *)driver_info->host_machine_name,
+	strncat(s_name, (char *)driver_info->host_machine_name,
 		BFA_FCS_PORT_SYMBNAME_MACHINENAME_SZ);
-	strncat((char *)&port_cfg->sym_name, BFA_FCS_PORT_SYMBNAME_SEPARATOR,
+	strncat(s_name, BFA_FCS_PORT_SYMBNAME_SEPARATOR,
 		sizeof(BFA_FCS_PORT_SYMBNAME_SEPARATOR));

 	/*
@@ -735,23 +735,18 @@ bfa_fcs_fabric_psymb_init(struct bfa_fcs_fabric_s *fabric)
 	 * OS name string and instead copy the entire OS info string (64 bytes).
 	 */
 	if (driver_info->host_os_patch[0] == '\0') {
-		strncat((char *)&port_cfg->sym_name,
-			(char *)driver_info->host_os_name,
+		strncat(s_name, (char *)driver_info->host_os_name,
 			BFA_FCS_OS_STR_LEN);
-		strncat((char *)&port_cfg->sym_name,
-			BFA_FCS_PORT_SYMBNAME_SEPARATOR,
+		strncat(s_name, BFA_FCS_PORT_SYMBNAME_SEPARATOR,
 			sizeof(BFA_FCS_PORT_SYMBNAME_SEPARATOR));
 	} else {
-		strncat((char *)&port_cfg->sym_name,
-			(char *)driver_info->host_os_name,
+		strncat(s_name, (char *)driver_info->host_os_name,
 			BFA_FCS_PORT_SYMBNAME_OSINFO_SZ);
-		strncat((char *)&port_cfg->sym_name,
-			BFA_FCS_PORT_SYMBNAME_SEPARATOR,
+		strncat(s_name, BFA_FCS_PORT_SYMBNAME_SEPARATOR,
 			sizeof(BFA_FCS_PORT_SYMBNAME_SEPARATOR));

 		/* Append host OS Patch Info */
-		strncat((char *)&port_cfg->sym_name,
-			(char *)driver_info->host_os_patch,
+		strncat(s_name, (char *)driver_info->host_os_patch,
 			BFA_FCS_PORT_SYMBNAME_OSPATCH_SZ);
 	}

--
1.7.12