| 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: <87wqytbic5.fsf@rho.meyering.net>
Date: Sun, 14 Oct 2012 09:53:14 +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
Jim Meyering wrote:
> 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.
Ping?
In case it wasn't clear, there *is* a risk of buffer overflow,
which happens when strncpy makes it so strncat's destination
is not NUL terminated.
If you require support for 12-byte model numbers, then
you'll have to increase the length of that buffer
(BFA_FCS_PORT_SYMBNAME_MODEL_SZ) to at least 13.
I've just rebased, and thus confirmed that the patches still apply.
> 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