I've seen a few too many cases of strncpy misuse.
Looking through linux sources, I spotted/fixed these:
[PATCH] ACPI: remove unwarranted use of strncpy
[PATCH] fs/9p: avoid debug OOPS when reading a long symlink
[PATCH] kmemleak: avoid buffer overrun: NUL-terminate strncpy-copied
[PATCH] bfa: avoid buffer overrun for 12-byte model name
[PATCH] cifs: remove misleading strncpy: each name has length < 16
drivers/acpi/sysfs.c | 3 +--
drivers/scsi/bfa/bfa_fcs.c | 1 +
fs/9p/vfs_inode.c | 8 ++++----
fs/cifs/cifssmb.c | 6 ++++--
mm/kmemleak.c | 1 +
5 files changed, 11 insertions(+), 8 deletions(-)
From: Jim Meyering <[email protected]>
strncpy is best avoided in general. Here, using strcpy would have
been clearer and semantically equivalent, but we can do better still
by removing it: i.e., use kstrdup in place of kzalloc+strncpy.
Signed-off-by: Jim Meyering <[email protected]>
---
drivers/acpi/sysfs.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
index 7c3f98b..20cc627 100644
--- a/drivers/acpi/sysfs.c
+++ b/drivers/acpi/sysfs.c
@@ -674,10 +674,9 @@ void acpi_irq_stats_init(void)
else
sprintf(buffer, "bug%02X", i);
- name = kzalloc(strlen(buffer) + 1, GFP_KERNEL);
+ name = kstrdup(buffer, GFP_KERNEL);
if (name == NULL)
goto fail;
- strncpy(name, buffer, strlen(buffer) + 1);
sysfs_attr_init(&counter_attrs[i].attr);
counter_attrs[i].attr.name = name;
--
1.7.12
From: Jim Meyering <[email protected]>
strncpy NUL-terminates only when the length of the source string
is smaller than the size of the destination buffer.
The two other strncpy uses (just preceding) happen to be ok
with the current TASK_COMM_LEN (16), because the literals
"hardirq" and "softirq" are both shorter than 16. However,
technically it'd be better to use strcpy along with a
compile-time assertion that they fit in the buffer.
Signed-off-by: Jim Meyering <[email protected]>
---
mm/kmemleak.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 45eb621..947257f 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -555,6 +555,7 @@ static struct kmemleak_object *create_object(unsigned long ptr, size_t size,
* case, the command line is not correct.
*/
strncpy(object->comm, current->comm, sizeof(object->comm));
+ object->comm[sizeof(object->comm) - 1] = 0;
}
/* kernel backtrace */
--
1.7.12
From: Jim Meyering <[email protected]>
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 <[email protected]>
---
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));
--
1.7.12
From: Jim Meyering <[email protected]>
Each of the protocols[i].name strings (statically declared above)
has length less than 16, so this use of strncpy is misleading:
strncpy(pSMB->DialectsArray+count, protocols[i].name, 16);
Besides, if a new name were added with length N >= 16, the existing
strncpy-using code would be buggy, creating a ->DialectsArray buffer
containing N-16+1 unset bytes where the NUL terminator should have
been. Instead, traverse the name only once go get its length,
use a BUG_ON assertion to enforce the length restriction
and use memcpy to perform the copy.
Signed-off-by: Jim Meyering <[email protected]>
---
fs/cifs/cifssmb.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 074923c..16a9018 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -441,8 +441,10 @@ CIFSSMBNegotiate(const unsigned int xid, struct cifs_ses *ses)
count = 0;
for (i = 0; i < CIFS_NUM_PROT; i++) {
- strncpy(pSMB->DialectsArray+count, protocols[i].name, 16);
- count += strlen(protocols[i].name) + 1;
+ size_t len = strlen(protocols[i].name);
+ BUG_ON(len >= 16);
+ memcpy(pSMB->DialectsArray+count, protocols[i].name, len + 1);
+ count += len + 1;
/* null at end of source and target buffers anyway */
}
inc_rfc1001_len(pSMB, count);
--
1.7.12
From: Jim Meyering <[email protected]>
Reading a symlink longer than the given buffer, a p9_debug use would
try to print the link name (not NUL-terminated) using a %s format.
Use %.*s instead, and replace the strncpy+strnlen with functionally
equivalent strlen+memcpy.
Signed-off-by: Jim Meyering <[email protected]>
---
fs/9p/vfs_inode.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index cbf9dbb..16ed405 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -1276,12 +1276,12 @@ static int v9fs_readlink(struct dentry *dentry, char *buffer, int buflen)
}
/* copy extension buffer into buffer */
- strncpy(buffer, st->extension, buflen);
+ retval = min(strlen(st->extension)+1, buflen);
+ memcpy(buffer, st->extension, retval);
- p9_debug(P9_DEBUG_VFS, "%s -> %s (%s)\n",
- dentry->d_name.name, st->extension, buffer);
+ p9_debug(P9_DEBUG_VFS, "%s -> %s (%.*s)\n",
+ dentry->d_name.name, st->extension, buflen, buffer);
- retval = strnlen(buffer, buflen);
done:
p9stat_free(st);
kfree(st);
--
1.7.12
Bastien ROUCARIES wrote:
> Le 20 ao?t 2012 19:29, "Jim Meyering" <[email protected]> a ?crit?:
>>
>> From: Jim Meyering <[email protected]>
>>
>> Each of the protocols[i].name strings (statically declared above)
>> has length less than 16, so this use of strncpy is misleading:
>> ? strncpy(pSMB->DialectsArray+count, protocols[i].name, 16);
>> Besides, if a new name were added with length N >= 16, the existing
>> strncpy-using code would be buggy, creating a ->DialectsArray buffer
>> containing N-16+1 unset bytes where the NUL terminator should have
>> been. ?Instead, traverse the name only once go get its length,
>> use a BUG_ON assertion to enforce the length restriction
>> and use memcpy to perform the copy.
>
> Could you use ARRAY_SIZE instead of hard coding 16?
Not that I can see: the DialectsArray member is declared like this:
typedef struct negotiate_req {
struct smb_hdr hdr; /* wct = 0 */
__le16 ByteCount;
unsigned char DialectsArray[1];
} __attribute__((packed)) NEGOTIATE_REQ;
...
>> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
>> index 074923c..16a9018 100644
>> --- a/fs/cifs/cifssmb.c
>> +++ b/fs/cifs/cifssmb.c
>> @@ -441,8 +441,10 @@ CIFSSMBNegotiate(const unsigned int xid, struct cifs_ses
> *ses)
>>
>> ? ? ? ? count = 0;
>> ? ? ? ? for (i = 0; i < CIFS_NUM_PROT; i++) {
>> - ? ? ? ? ? ? ? strncpy(pSMB->DialectsArray+count, protocols[i].name, 16);
>> - ? ? ? ? ? ? ? count += strlen(protocols[i].name) + 1;
>> + ? ? ? ? ? ? ? size_t len = strlen(protocols[i].name);
>> + ? ? ? ? ? ? ? BUG_ON(len >= 16);
>> + ? ? ? ? ? ? ? memcpy(pSMB->DialectsArray+count, protocols[i].name, len + 1);
>> + ? ? ? ? ? ? ? count += len + 1;
Jim Meyering wrote:
> From: Jim Meyering <[email protected]>
>
> Each of the protocols[i].name strings (statically declared above)
> has length less than 16, so this use of strncpy is misleading:
> strncpy(pSMB->DialectsArray+count, protocols[i].name, 16);
> Besides, if a new name were added with length N >= 16, the existing
> strncpy-using code would be buggy, creating a ->DialectsArray buffer
> containing N-16+1 unset bytes where the NUL terminator should have
> been. Instead, traverse the name only once go get its length,
s/go/to/, of course.
> use a BUG_ON assertion to enforce the length restriction
> and use memcpy to perform the copy.
>
> Signed-off-by: Jim Meyering <[email protected]>
> ---
> fs/cifs/cifssmb.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index 074923c..16a9018 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -441,8 +441,10 @@ CIFSSMBNegotiate(const unsigned int xid, struct cifs_ses *ses)
>
> count = 0;
> for (i = 0; i < CIFS_NUM_PROT; i++) {
> - strncpy(pSMB->DialectsArray+count, protocols[i].name, 16);
> - count += strlen(protocols[i].name) + 1;
> + size_t len = strlen(protocols[i].name);
> + BUG_ON(len >= 16);
> + memcpy(pSMB->DialectsArray+count, protocols[i].name, len + 1);
> + count += len + 1;
> /* null at end of source and target buffers anyway */
> }
> inc_rfc1001_len(pSMB, count);
Jim Meyering <[email protected]> writes:
> I've seen a few too many cases of strncpy misuse.
> Looking through linux sources, I spotted/fixed these:
>
> [PATCH] ACPI: remove unwarranted use of strncpy
> [PATCH] fs/9p: avoid debug OOPS when reading a long symlink
> [PATCH] kmemleak: avoid buffer overrun: NUL-terminate strncpy-copied
> [PATCH] bfa: avoid buffer overrun for 12-byte model name
> [PATCH] cifs: remove misleading strncpy: each name has length < 16
Better to convert tham all to strlcpy?
The kernel has it.
-Andi
--
[email protected] -- Speaking for myself only
> -----Original Message-----
> From: Jim Meyering [mailto:[email protected]]
> Sent: Monday, August 20, 2012 9:55 AM
> To: [email protected]
> Cc: Jim Meyering; Jing Huang; Krishna Gudipati; James E.J. Bottomley; linux-
> [email protected]
> Subject: [PATCH] bfa: avoid buffer overrun for 12-byte model name
>
> From: Jim Meyering <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
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.
Thanks,
Krishna
Krishna Gudipati wrote:
>> -----Original Message-----
>> From: Jim Meyering [mailto:[email protected]]
>> Sent: Monday, August 20, 2012 9:55 AM
>> To: [email protected]
>> Cc: Jim Meyering; Jing Huang; Krishna Gudipati; James E.J. Bottomley; linux-
>> [email protected]
>> Subject: [PATCH] bfa: avoid buffer overrun for 12-byte model name
>>
>> From: Jim Meyering <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
>
> 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 <[email protected]>
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 <[email protected]>
---
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 <[email protected]>
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 <[email protected]>
---
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
Andi Kleen wrote:
> Jim Meyering <[email protected]> writes:
>
>> I've seen a few too many cases of strncpy misuse.
>> Looking through linux sources, I spotted/fixed these:
>>
>> [PATCH] ACPI: remove unwarranted use of strncpy
>> [PATCH] fs/9p: avoid debug OOPS when reading a long symlink
>> [PATCH] kmemleak: avoid buffer overrun: NUL-terminate strncpy-copied
>> [PATCH] bfa: avoid buffer overrun for 12-byte model name
>> [PATCH] cifs: remove misleading strncpy: each name has length < 16
>
> Better to convert tham all to strlcpy?
>
> The kernel has it.
Anything is better than strncpy.
Is there consensus here that strlcpy is preferred?
Would the integrator(s) even require consensus?
$ git grep -w strncpy|wc -l
987
$ git grep -w strlcpy|wc -l
1345
In any case, shouldn't fixing bugs and obvious misuse be separate
from any global NSC conversion?
Reading a symlink longer than the given buffer, a p9_debug use would
try to print the link name (not NUL-terminated) using a %s format.
Use %.*s instead, and replace the strncpy+strnlen with functionally
equivalent strlen+memcpy.
Signed-off-by: Jim Meyering <[email protected]>
---
V1 provoked a warning due to strlen/buflen type differences (size_t/int)
and the "min" macro's type equality requirement. This adds a cast to
avoid that warning:
- retval = min(strlen(st->extension)+1, buflen);
+ retval = min(strlen(st->extension)+1, (size_t)buflen);
fs/9p/vfs_inode.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index cbf9dbb..890bed5 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -1276,12 +1276,12 @@ static int v9fs_readlink(struct dentry *dentry, char *buffer, int buflen)
}
/* copy extension buffer into buffer */
- strncpy(buffer, st->extension, buflen);
+ retval = min(strlen(st->extension)+1, (size_t)buflen);
+ memcpy(buffer, st->extension, retval);
- p9_debug(P9_DEBUG_VFS, "%s -> %s (%s)\n",
- dentry->d_name.name, st->extension, buffer);
+ p9_debug(P9_DEBUG_VFS, "%s -> %s (%.*s)\n",
+ dentry->d_name.name, st->extension, buflen, buffer);
- retval = strnlen(buffer, buflen);
done:
p9stat_free(st);
kfree(st);
--
1.7.12
On Mon, Aug 20, 2012 at 05:55:22PM +0100, Jim Meyering wrote:
> From: Jim Meyering <[email protected]>
>
> strncpy NUL-terminates only when the length of the source string
> is smaller than the size of the destination buffer.
> The two other strncpy uses (just preceding) happen to be ok
> with the current TASK_COMM_LEN (16), because the literals
> "hardirq" and "softirq" are both shorter than 16. However,
> technically it'd be better to use strcpy along with a
> compile-time assertion that they fit in the buffer.
>
> Signed-off-by: Jim Meyering <[email protected]>
> ---
> mm/kmemleak.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> index 45eb621..947257f 100644
> --- a/mm/kmemleak.c
> +++ b/mm/kmemleak.c
> @@ -555,6 +555,7 @@ static struct kmemleak_object *create_object(unsigned long ptr, size_t size,
> * case, the command line is not correct.
> */
> strncpy(object->comm, current->comm, sizeof(object->comm));
> + object->comm[sizeof(object->comm) - 1] = 0;
Does it really matter here? object->comm[] and current->comm[] have the
same size, TASK_COMM_LEN.
--
Catalin
Catalin Marinas wrote:
> On Mon, Aug 20, 2012 at 05:55:22PM +0100, Jim Meyering wrote:
>> From: Jim Meyering <[email protected]>
>>
>> strncpy NUL-terminates only when the length of the source string
>> is smaller than the size of the destination buffer.
>> The two other strncpy uses (just preceding) happen to be ok
>> with the current TASK_COMM_LEN (16), because the literals
>> "hardirq" and "softirq" are both shorter than 16. However,
>> technically it'd be better to use strcpy along with a
>> compile-time assertion that they fit in the buffer.
>>
>> Signed-off-by: Jim Meyering <[email protected]>
>> ---
>> mm/kmemleak.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
>> index 45eb621..947257f 100644
>> --- a/mm/kmemleak.c
>> +++ b/mm/kmemleak.c
>> @@ -555,6 +555,7 @@ static struct kmemleak_object *create_object(unsigned long ptr, size_t size,
>> * case, the command line is not correct.
>> */
>> strncpy(object->comm, current->comm, sizeof(object->comm));
>> + object->comm[sizeof(object->comm) - 1] = 0;
>
> Does it really matter here? object->comm[] and current->comm[] have the
> same size, TASK_COMM_LEN.
TL;DR: either it may matter, and the patch is useful,
or else that use of strncpy is unwarranted.
----------------
Can we certify that those lengths will always be the same, i.e.,
by adding something like this ?
static_assert (sizeof (object->comm) != sizeof(current->comm));
[I know we can't rely on this C11 syntax. see below]
There are two reasons one might use strncpy:
1) to truncate, when strlen(src) >= dest_buf_len
2) to NUL-pad out to the length of dest_buf_len
The only uses of ->comm are to print that name, so (2) appears not to be
a concern. Hence, if we are confident that the buffers will always have
the same length, then there is no reason to use strncpy in the first place.
In that case, what would you think of a patch to use strcpy instead?
- strncpy(object->comm, current->comm, sizeof(object->comm));
+ strcpy(object->comm, current->comm);
Is there a preferred method of adding a static_assert-like statement?
I see compile_time_assert and a few similar macros, but I haven't
spotted anything that is used project-wide.
On Fri, Aug 24, 2012 at 01:23:29PM +0200, Jim Meyering wrote:
> In that case, what would you think of a patch to use strcpy instead?
>
> - strncpy(object->comm, current->comm, sizeof(object->comm));
> + strcpy(object->comm, current->comm);
Another option would be to use strlcpy(). It's slightly neater than
the strncpy() followed by a NUL assignment.
>
> Is there a preferred method of adding a static_assert-like statement?
> I see compile_time_assert and a few similar macros, but I haven't
> spotted anything that is used project-wide.
BUILD_BUG_ON().
regards,
dan carpenter
Dan Carpenter wrote:
> On Fri, Aug 24, 2012 at 01:23:29PM +0200, Jim Meyering wrote:
>> In that case, what would you think of a patch to use strcpy instead?
>>
>> - strncpy(object->comm, current->comm, sizeof(object->comm));
>> + strcpy(object->comm, current->comm);
>
> Another option would be to use strlcpy(). It's slightly neater than
> the strncpy() followed by a NUL assignment.
>
>> Is there a preferred method of adding a static_assert-like statement?
>> I see compile_time_assert and a few similar macros, but I haven't
>> spotted anything that is used project-wide.
>
> BUILD_BUG_ON().
Hi Dan,
Thanks for the feedback and tip. How about this patch?
-- >8 --
Subject: [PATCH] kmemleak: remove unwarranted uses of strncpy
Use of strncpy was not justified -- was misleading, in fact, since
none of the three uses could trigger strncpy's truncation feature,
nor did they require the NUL-padding it can provide. Replace each
use with a BUG_ON_BUILD to ensure that the existing constraint
(source string is no larger than the size of the destination buffer)
and a use of strcpy. With the literals, it's easy to see that each
is shorter than TASK_COMM_LEN (aka, 16). In the third case, the
source and destination buffer have the same length, so there is no
possibility of truncation.
Signed-off-by: Jim Meyering <[email protected]>
---
mm/kmemleak.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 45eb621..7359ffa 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -542,10 +542,12 @@ static struct kmemleak_object *create_object(unsigned long ptr, size_t size,
/* task information */
if (in_irq()) {
object->pid = 0;
- strncpy(object->comm, "hardirq", sizeof(object->comm));
+ BUILD_BUG_ON(sizeof "hardirq" > sizeof(current->comm));
+ strcpy(object->comm, "hardirq");
} else if (in_softirq()) {
object->pid = 0;
- strncpy(object->comm, "softirq", sizeof(object->comm));
+ BUILD_BUG_ON(sizeof "softirq" > sizeof(current->comm));
+ strcpy(object->comm, "softirq");
} else {
object->pid = current->pid;
/*
@@ -554,7 +556,8 @@ static struct kmemleak_object *create_object(unsigned long ptr, size_t size,
* dependency issues with current->alloc_lock. In the worst
* case, the command line is not correct.
*/
- strncpy(object->comm, current->comm, sizeof(object->comm));
+ BUILD_BUG_ON(sizeof (object->comm) > sizeof(current->comm));
+ strcpy(object->comm, current->comm);
}
/* kernel backtrace */
--
1.7.12.116.g31e0100
On Wed, Aug 29, 2012 at 08:28:47AM +0200, Jim Meyering wrote:
> Dan Carpenter wrote:
> > On Fri, Aug 24, 2012 at 01:23:29PM +0200, Jim Meyering wrote:
> >> In that case, what would you think of a patch to use strcpy instead?
> >>
> >> - strncpy(object->comm, current->comm, sizeof(object->comm));
> >> + strcpy(object->comm, current->comm);
> >
> > Another option would be to use strlcpy(). It's slightly neater than
> > the strncpy() followed by a NUL assignment.
> >
> >> Is there a preferred method of adding a static_assert-like statement?
> >> I see compile_time_assert and a few similar macros, but I haven't
> >> spotted anything that is used project-wide.
> >
> > BUILD_BUG_ON().
>
> Hi Dan,
>
> Thanks for the feedback and tip. How about this patch?
>
I'm not someone who can approve kmemleak patches, but that's
horrible. I'm not sure we need a BUILD_BUG_ON(), I was just telling
you the standard way to do a build time assert. If we did put the
assert in then it should only be in one place in the header file
where the data is decared instead of repeated over and over.
I like strlcpy(). Both strcpy() and strlcpy() will silence your
static checker tool. strcpy() may be better, but strlcpy() feels
very safe so that might be preferred.
regards,
dan carpenter
Jim Meyering wrote:
> Krishna Gudipati wrote:
>>> -----Original Message-----
>>> From: Jim Meyering [mailto:[email protected]]
>>> Sent: Monday, August 20, 2012 9:55 AM
>>> To: [email protected]
>>> Cc: Jim Meyering; Jing Huang; Krishna Gudipati; James E.J. Bottomley; linux-
>>> [email protected]
>>> Subject: [PATCH] bfa: avoid buffer overrun for 12-byte model name
>>>
>>> From: Jim Meyering <[email protected]>
>>>
>>> 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 <[email protected]>
>>> ---
>>> 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 <[email protected]>
>>
>> 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 <[email protected]>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
> 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 <[email protected]>
> ---
> 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
Jim Meyering wrote:
> Jim Meyering wrote:
>> Krishna Gudipati wrote:
>>>> -----Original Message-----
>>>> From: Jim Meyering [mailto:[email protected]]
>>>> Sent: Monday, August 20, 2012 9:55 AM
>>>> To: [email protected]
>>>> Cc: Jim Meyering; Jing Huang; Krishna Gudipati; James E.J. Bottomley; linux-
>>>> [email protected]
>>>> Subject: [PATCH] bfa: avoid buffer overrun for 12-byte model name
>>>>
>>>> From: Jim Meyering <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> ---
>>>> 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 <[email protected]>
>>>
>>> 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.
Taking Andi's advice, I've made the offending code use
strlcpy in place of strncpy. More importantly, I've fixed
the same bug also in the following, nearly identical function.
-- >8 --
Two functions have this problem:
bfa_fcs_fabric_psymb_init
bfa_fcs_fabric_nsymb_init
They 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.
Instead, use strlcpy, which does guarantee NUL-termination.
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 <[email protected]>
---
drivers/scsi/bfa/bfa_fcs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/bfa/bfa_fcs.c b/drivers/scsi/bfa/bfa_fcs.c
index d428808..c7f476c 100644
--- a/drivers/scsi/bfa/bfa_fcs.c
+++ b/drivers/scsi/bfa/bfa_fcs.c
@@ -837,7 +837,7 @@ bfa_fcs_fabric_psymb_init(struct bfa_fcs_fabric_s *fabric)
bfa_ioc_get_adapter_model(&fabric->fcs->bfa->ioc, model);
/* Model name/number */
- strncpy((char *)&port_cfg->sym_name, model,
+ strlcpy((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));
@@ -898,7 +898,7 @@ bfa_fcs_fabric_nsymb_init(struct bfa_fcs_fabric_s *fabric)
bfa_ioc_get_adapter_model(&fabric->fcs->bfa->ioc, model);
/* Model name/number */
- strncpy((char *)&port_cfg->node_sym_name, model,
+ strlcpy((char *)&port_cfg->node_sym_name, model,
BFA_FCS_PORT_SYMBNAME_MODEL_SZ);
strncat((char *)&port_cfg->node_sym_name,
BFA_FCS_PORT_SYMBNAME_SEPARATOR,
--
1.8.0.rc2