[PATCH] kmemleak: avoid buffer overrun: NUL-terminate strncpy-copied command

[PATCH] kmemleak: avoid buffer overrun: NUL-terminate strncpy-copied command

[Jim Meyering]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] kmemleak: avoid buffer overrun: NUL-terminate strncpy-copied command

[Catalin Marinas]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] kmemleak: avoid buffer overrun: NUL-terminate strncpy-copied command

[Jim Meyering]

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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] kmemleak: avoid buffer overrun: NUL-terminate strncpy-copied command

[Dan Carpenter]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] kmemleak: avoid buffer overrun: NUL-terminate strncpy-copied command

[Jim Meyering]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] kmemleak: avoid buffer overrun: NUL-terminate strncpy-copied command

[Dan Carpenter]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>