| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87d32ll2nk.fsf@rho.meyering.net>
Date: Mon, 20 Aug 2012 22:38:39 +0200
From: Jim Meyering <jim@...ering.net>
To: Krishna Gudipati <kgudipat@...cade.com>
Cc: "linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>,
"James E.J. Bottomley" <JBottomley@...allels.com>,
"linux-scsi\@vger.kernel.org" <linux-scsi@...r.kernel.org>
Subject: Re: [PATCH] bfa: avoid buffer overrun for 12-byte model name
Krishna Gudipati wrote:
>> -----Original Message-----
>> From: Jim Meyering [mailto:jim@...ering.net]
>> Sent: Monday, August 20, 2012 9:55 AM
>> To: linux-kernel@...r.kernel.org
>> Cc: Jim Meyering; Jing Huang; Krishna Gudipati; James E.J. Bottomley; linux-
>> scsi@...r.kernel.org
>> Subject: [PATCH] bfa: avoid buffer overrun for 12-byte model name
>>
>> From: Jim Meyering <meyering@...hat.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@...hat.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@...cade.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@...hat.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@...hat.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@...hat.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@...hat.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists