* a few strncpy-related patches
@ 2012-08-20 16:55 Jim Meyering
2012-08-20 16:55 ` [PATCH] ACPI: remove unwarranted use of strncpy Jim Meyering
` (5 more replies)
0 siblings, 6 replies; 21+ messages in thread
From: Jim Meyering @ 2012-08-20 16:55 UTC (permalink / raw)
To: linux-kernel
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(-)
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH] ACPI: remove unwarranted use of strncpy 2012-08-20 16:55 a few strncpy-related patches Jim Meyering @ 2012-08-20 16:55 ` Jim Meyering 2012-08-20 16:55 ` [PATCH] fs/9p: avoid debug OOPS when reading a long symlink Jim Meyering ` (4 subsequent siblings) 5 siblings, 0 replies; 21+ messages in thread From: Jim Meyering @ 2012-08-20 16:55 UTC (permalink / raw) To: linux-kernel; +Cc: Jim Meyering, Len Brown, linux-acpi From: Jim Meyering <meyering@redhat.com> 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 <meyering@redhat.com> --- 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH] fs/9p: avoid debug OOPS when reading a long symlink 2012-08-20 16:55 a few strncpy-related patches Jim Meyering 2012-08-20 16:55 ` [PATCH] ACPI: remove unwarranted use of strncpy Jim Meyering @ 2012-08-20 16:55 ` Jim Meyering 2012-08-21 7:20 ` [PATCHv2] " Jim Meyering 2012-08-20 16:55 ` [PATCH] kmemleak: avoid buffer overrun: NUL-terminate strncpy-copied command Jim Meyering ` (3 subsequent siblings) 5 siblings, 1 reply; 21+ messages in thread From: Jim Meyering @ 2012-08-20 16:55 UTC (permalink / raw) To: linux-kernel Cc: Jim Meyering, Eric Van Hensbergen, Ron Minnich, Latchesar Ionkov, v9fs-developer From: Jim Meyering <meyering@redhat.com> 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 <meyering@redhat.com> --- 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCHv2] fs/9p: avoid debug OOPS when reading a long symlink 2012-08-20 16:55 ` [PATCH] fs/9p: avoid debug OOPS when reading a long symlink Jim Meyering @ 2012-08-21 7:20 ` Jim Meyering 0 siblings, 0 replies; 21+ messages in thread From: Jim Meyering @ 2012-08-21 7:20 UTC (permalink / raw) To: linux-kernel Cc: Eric Van Hensbergen, Ron Minnich, Latchesar Ionkov, v9fs-developer 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 <meyering@redhat.com> --- 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH] kmemleak: avoid buffer overrun: NUL-terminate strncpy-copied command 2012-08-20 16:55 a few strncpy-related patches Jim Meyering 2012-08-20 16:55 ` [PATCH] ACPI: remove unwarranted use of strncpy Jim Meyering 2012-08-20 16:55 ` [PATCH] fs/9p: avoid debug OOPS when reading a long symlink Jim Meyering @ 2012-08-20 16:55 ` Jim Meyering 2012-08-24 10:27 ` Catalin Marinas 2012-08-20 16:55 ` [PATCH] bfa: avoid buffer overrun for 12-byte model name Jim Meyering ` (2 subsequent siblings) 5 siblings, 1 reply; 21+ messages in thread From: Jim Meyering @ 2012-08-20 16:55 UTC (permalink / raw) To: linux-kernel; +Cc: Jim Meyering, Catalin Marinas, linux-mm From: Jim Meyering <meyering@redhat.com> 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 <meyering@redhat.com> --- 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] kmemleak: avoid buffer overrun: NUL-terminate strncpy-copied command 2012-08-20 16:55 ` [PATCH] kmemleak: avoid buffer overrun: NUL-terminate strncpy-copied command Jim Meyering @ 2012-08-24 10:27 ` Catalin Marinas 2012-08-24 11:23 ` Jim Meyering 0 siblings, 1 reply; 21+ messages in thread From: Catalin Marinas @ 2012-08-24 10:27 UTC (permalink / raw) To: Jim Meyering; +Cc: linux-kernel, Jim Meyering, linux-mm On Mon, Aug 20, 2012 at 05:55:22PM +0100, Jim Meyering wrote: > From: Jim Meyering <meyering@redhat.com> > > 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 <meyering@redhat.com> > --- > 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] kmemleak: avoid buffer overrun: NUL-terminate strncpy-copied command 2012-08-24 10:27 ` Catalin Marinas @ 2012-08-24 11:23 ` Jim Meyering 2012-08-28 20:24 ` Dan Carpenter 0 siblings, 1 reply; 21+ messages in thread From: Jim Meyering @ 2012-08-24 11:23 UTC (permalink / raw) To: Catalin Marinas; +Cc: linux-kernel, linux-mm Catalin Marinas wrote: > On Mon, Aug 20, 2012 at 05:55:22PM +0100, Jim Meyering wrote: >> From: Jim Meyering <meyering@redhat.com> >> >> 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 <meyering@redhat.com> >> --- >> 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] kmemleak: avoid buffer overrun: NUL-terminate strncpy-copied command 2012-08-24 11:23 ` Jim Meyering @ 2012-08-28 20:24 ` Dan Carpenter 2012-08-29 6:28 ` Jim Meyering 0 siblings, 1 reply; 21+ messages in thread From: Dan Carpenter @ 2012-08-28 20:24 UTC (permalink / raw) To: Jim Meyering; +Cc: Catalin Marinas, linux-kernel, linux-mm 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] kmemleak: avoid buffer overrun: NUL-terminate strncpy-copied command 2012-08-28 20:24 ` Dan Carpenter @ 2012-08-29 6:28 ` Jim Meyering 2012-08-29 15:56 ` Dan Carpenter 0 siblings, 1 reply; 21+ messages in thread From: Jim Meyering @ 2012-08-29 6:28 UTC (permalink / raw) To: Dan Carpenter; +Cc: Catalin Marinas, linux-kernel, linux-mm 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 <meyering@redhat.com> --- 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] kmemleak: avoid buffer overrun: NUL-terminate strncpy-copied command 2012-08-29 6:28 ` Jim Meyering @ 2012-08-29 15:56 ` Dan Carpenter 0 siblings, 0 replies; 21+ messages in thread From: Dan Carpenter @ 2012-08-29 15:56 UTC (permalink / raw) To: Jim Meyering; +Cc: Catalin Marinas, linux-kernel, linux-mm 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] bfa: avoid buffer overrun for 12-byte model name 2012-08-20 16:55 a few strncpy-related patches Jim Meyering ` (2 preceding siblings ...) 2012-08-20 16:55 ` [PATCH] kmemleak: avoid buffer overrun: NUL-terminate strncpy-copied command Jim Meyering @ 2012-08-20 16:55 ` Jim Meyering 2012-08-20 19:42 ` Krishna Gudipati 2012-08-20 16:55 ` [PATCH] cifs: remove misleading strncpy: each name has length < 16 Jim Meyering 2012-08-20 20:18 ` a few strncpy-related patches Andi Kleen 5 siblings, 1 reply; 21+ messages in thread From: Jim Meyering @ 2012-08-20 16:55 UTC (permalink / raw) To: linux-kernel Cc: Jim Meyering, Jing Huang, Krishna C Gudipati, James E.J. Bottomley, linux-scsi 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)); -- 1.7.12 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* RE: [PATCH] bfa: avoid buffer overrun for 12-byte model name 2012-08-20 16:55 ` [PATCH] bfa: avoid buffer overrun for 12-byte model name Jim Meyering @ 2012-08-20 19:42 ` Krishna Gudipati 2012-08-20 20:38 ` Jim Meyering 0 siblings, 1 reply; 21+ messages in thread From: Krishna Gudipati @ 2012-08-20 19:42 UTC (permalink / raw) To: Jim Meyering, linux-kernel; +Cc: Jim Meyering, James E.J. Bottomley, linux-scsi > -----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. Thanks, Krishna ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] bfa: avoid buffer overrun for 12-byte model name 2012-08-20 19:42 ` Krishna Gudipati @ 2012-08-20 20:38 ` Jim Meyering 2012-10-14 7:53 ` Jim Meyering 0 siblings, 1 reply; 21+ messages in thread From: Jim Meyering @ 2012-08-20 20:38 UTC (permalink / raw) To: Krishna Gudipati; +Cc: linux-kernel, James E.J. Bottomley, linux-scsi 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] bfa: avoid buffer overrun for 12-byte model name 2012-08-20 20:38 ` Jim Meyering @ 2012-10-14 7:53 ` Jim Meyering 2012-10-14 8:20 ` Jim Meyering 0 siblings, 1 reply; 21+ messages in thread From: Jim Meyering @ 2012-10-14 7:53 UTC (permalink / raw) To: Krishna Gudipati; +Cc: linux-kernel, James E.J. Bottomley, linux-scsi Jim Meyering wrote: > 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. 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@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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] bfa: avoid buffer overrun for 12-byte model name 2012-10-14 7:53 ` Jim Meyering @ 2012-10-14 8:20 ` Jim Meyering 2012-12-24 7:43 ` Vijay Mohan Guvva 0 siblings, 1 reply; 21+ messages in thread From: Jim Meyering @ 2012-10-14 8:20 UTC (permalink / raw) To: Krishna Gudipati Cc: Andi Kleen, linux-kernel, James E.J. Bottomley, linux-scsi Jim Meyering wrote: > Jim Meyering wrote: >> 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. > > 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 <jim@meyering.net> --- 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* RE: [PATCH] bfa: avoid buffer overrun for 12-byte model name 2012-10-14 8:20 ` Jim Meyering @ 2012-12-24 7:43 ` Vijay Mohan Guvva 0 siblings, 0 replies; 21+ messages in thread From: Vijay Mohan Guvva @ 2012-12-24 7:43 UTC (permalink / raw) To: Jim Meyering; +Cc: Andi Kleen, linux-kernel, James E.J. Bottomley, linux-scsi Hi Jim, Due to BFA_FCS_PORT_SYMBNAME_MODEL_SZ macro value of 12, we are missing some part of the model name in port/node symbolic name and seeing issues related to null termination. Mismatch between the actual model size and number of bytes copied to symbolic name is a bug. Can you please fix this by changing BFA_FCS_PORT_SYMBNAME_MODEL_SZ to 16 and reduce os_name macro (BFA_FCS_PORT_SYMBNAME_OSINFO_SZ) to 44, so that both the issues i.e symbolic name and null termination will be fixed. Thanks, Vijay -----Original Message----- From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Jim Meyering Sent: Sunday, October 14, 2012 1:51 PM To: Krishna Gudipati Cc: Andi Kleen; linux-kernel@vger.kernel.org; James E.J. Bottomley; linux-scsi@vger.kernel.org Subject: Re: [PATCH] bfa: avoid buffer overrun for 12-byte model name Jim Meyering wrote: > Jim Meyering wrote: >> 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. > > 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 <jim@meyering.net> --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] cifs: remove misleading strncpy: each name has length < 16 2012-08-20 16:55 a few strncpy-related patches Jim Meyering ` (3 preceding siblings ...) 2012-08-20 16:55 ` [PATCH] bfa: avoid buffer overrun for 12-byte model name Jim Meyering @ 2012-08-20 16:55 ` Jim Meyering [not found] ` <CAE2SPAbBmRov9qK2HiBQBQXaZpfJ8pmZJ-PL18FEyoZhzDza4A@mail.gmail.com> 2012-08-20 18:41 ` Jim Meyering 2012-08-20 20:18 ` a few strncpy-related patches Andi Kleen 5 siblings, 2 replies; 21+ messages in thread From: Jim Meyering @ 2012-08-20 16:55 UTC (permalink / raw) To: linux-kernel; +Cc: Jim Meyering, Steve French, linux-cifs, samba-technical From: Jim Meyering <meyering@redhat.com> 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 <meyering@redhat.com> --- 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
[parent not found: <CAE2SPAbBmRov9qK2HiBQBQXaZpfJ8pmZJ-PL18FEyoZhzDza4A@mail.gmail.com>]
* Re: [PATCH] cifs: remove misleading strncpy: each name has length < 16 [not found] ` <CAE2SPAbBmRov9qK2HiBQBQXaZpfJ8pmZJ-PL18FEyoZhzDza4A@mail.gmail.com> @ 2012-08-20 18:40 ` Jim Meyering 0 siblings, 0 replies; 21+ messages in thread From: Jim Meyering @ 2012-08-20 18:40 UTC (permalink / raw) To: Bastien ROUCARIES; +Cc: linux-cifs, linux-kernel, samba-technical, Steve French Bastien ROUCARIES wrote: > Le 20 août 2012 19:29, "Jim Meyering" <jim@meyering.net> a écrit : >> >> From: Jim Meyering <meyering@redhat.com> >> >> 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; ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] cifs: remove misleading strncpy: each name has length < 16 2012-08-20 16:55 ` [PATCH] cifs: remove misleading strncpy: each name has length < 16 Jim Meyering [not found] ` <CAE2SPAbBmRov9qK2HiBQBQXaZpfJ8pmZJ-PL18FEyoZhzDza4A@mail.gmail.com> @ 2012-08-20 18:41 ` Jim Meyering 1 sibling, 0 replies; 21+ messages in thread From: Jim Meyering @ 2012-08-20 18:41 UTC (permalink / raw) To: linux-kernel; +Cc: Steve French, linux-cifs, samba-technical Jim Meyering wrote: > From: Jim Meyering <meyering@redhat.com> > > 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 <meyering@redhat.com> > --- > 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); ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: a few strncpy-related patches 2012-08-20 16:55 a few strncpy-related patches Jim Meyering ` (4 preceding siblings ...) 2012-08-20 16:55 ` [PATCH] cifs: remove misleading strncpy: each name has length < 16 Jim Meyering @ 2012-08-20 20:18 ` Andi Kleen 2012-08-20 20:47 ` Jim Meyering 5 siblings, 1 reply; 21+ messages in thread From: Andi Kleen @ 2012-08-20 20:18 UTC (permalink / raw) To: Jim Meyering; +Cc: linux-kernel Jim Meyering <jim@meyering.net> 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 -- ak@linux.intel.com -- Speaking for myself only ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: a few strncpy-related patches 2012-08-20 20:18 ` a few strncpy-related patches Andi Kleen @ 2012-08-20 20:47 ` Jim Meyering 0 siblings, 0 replies; 21+ messages in thread From: Jim Meyering @ 2012-08-20 20:47 UTC (permalink / raw) To: Andi Kleen; +Cc: linux-kernel Andi Kleen wrote: > Jim Meyering <jim@meyering.net> 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? ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2012-12-24 8:16 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-08-20 16:55 a few strncpy-related patches Jim Meyering
2012-08-20 16:55 ` [PATCH] ACPI: remove unwarranted use of strncpy Jim Meyering
2012-08-20 16:55 ` [PATCH] fs/9p: avoid debug OOPS when reading a long symlink Jim Meyering
2012-08-21 7:20 ` [PATCHv2] " Jim Meyering
2012-08-20 16:55 ` [PATCH] kmemleak: avoid buffer overrun: NUL-terminate strncpy-copied command Jim Meyering
2012-08-24 10:27 ` Catalin Marinas
2012-08-24 11:23 ` Jim Meyering
2012-08-28 20:24 ` Dan Carpenter
2012-08-29 6:28 ` Jim Meyering
2012-08-29 15:56 ` Dan Carpenter
2012-08-20 16:55 ` [PATCH] bfa: avoid buffer overrun for 12-byte model name Jim Meyering
2012-08-20 19:42 ` Krishna Gudipati
2012-08-20 20:38 ` Jim Meyering
2012-10-14 7:53 ` Jim Meyering
2012-10-14 8:20 ` Jim Meyering
2012-12-24 7:43 ` Vijay Mohan Guvva
2012-08-20 16:55 ` [PATCH] cifs: remove misleading strncpy: each name has length < 16 Jim Meyering
[not found] ` <CAE2SPAbBmRov9qK2HiBQBQXaZpfJ8pmZJ-PL18FEyoZhzDza4A@mail.gmail.com>
2012-08-20 18:40 ` Jim Meyering
2012-08-20 18:41 ` Jim Meyering
2012-08-20 20:18 ` a few strncpy-related patches Andi Kleen
2012-08-20 20:47 ` Jim Meyering
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).