[PATCH] net/rds: using strlcpy instead of strncpy

[PATCH] net/rds: using strlcpy instead of strncpy

[Chen Gang]

  when src strlen is sizeof(ctr.name) - 1, the dest str is not '\0' end.
  better use strlcpy instead.

Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 net/rds/stats.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/rds/stats.c b/net/rds/stats.c
index 7be790d..b9ac1df 100644
--- a/net/rds/stats.c
+++ b/net/rds/stats.c
@@ -86,7 +86,7 @@ void rds_stats_info_copy(struct rds_info_iterator *iter,
 
 	for (i = 0; i < nr; i++) {
 		BUG_ON(strlen(names[i]) >= sizeof(ctr.name));
-		strncpy(ctr.name, names[i], sizeof(ctr.name) - 1);
+		strlcpy(ctr.name, names[i], sizeof(ctr.name));
 		ctr.value = values[i];
 
 		rds_info_copy(iter, &ctr, sizeof(ctr));
-- 
1.7.7.6

RE: [PATCH] net/rds: using strlcpy instead of strncpy

[David Laight]

>   when src strlen is sizeof(ctr.name) - 1, the dest str is not '\0' end.
>   better use strlcpy instead.
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  net/rds/stats.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/rds/stats.c b/net/rds/stats.c
> index 7be790d..b9ac1df 100644
> --- a/net/rds/stats.c
> +++ b/net/rds/stats.c
> @@ -86,7 +86,7 @@ void rds_stats_info_copy(struct rds_info_iterator *iter,
> 
>  	for (i = 0; i < nr; i++) {
>  		BUG_ON(strlen(names[i]) >= sizeof(ctr.name));
> -		strncpy(ctr.name, names[i], sizeof(ctr.name) - 1);
> +		strlcpy(ctr.name, names[i], sizeof(ctr.name));
>  		ctr.value = values[i];
> 

If the target buffer ends up being copied to userspace that
might lead to random kernel memory being leaked.

	David

Re: [PATCH] net/rds: using strlcpy instead of strncpy

[Chen Gang]

于 2013年02月28日 17:36, David Laight 写道:
>> > diff --git a/net/rds/stats.c b/net/rds/stats.c
>> > index 7be790d..b9ac1df 100644
>> > --- a/net/rds/stats.c
>> > +++ b/net/rds/stats.c
>> > @@ -86,7 +86,7 @@ void rds_stats_info_copy(struct rds_info_iterator *iter,
>> > 
>> >  	for (i = 0; i < nr; i++) {
>> >  		BUG_ON(strlen(names[i]) >= sizeof(ctr.name));
>> > -		strncpy(ctr.name, names[i], sizeof(ctr.name) - 1);
>> > +		strlcpy(ctr.name, names[i], sizeof(ctr.name));
>> >  		ctr.value = values[i];
>> > 
> If the target buffer ends up being copied to userspace that
> might lead to random kernel memory being leaked.

  excuse me, I do not quite understand what your meaning.

  I guess what you said is:
    if crt.name is not ended by '\0', it will cause issue.
    the issue is:
      "If the target buffer ends up being copied to userspace that
       might lead to random kernel memory being leaked."

  is it correct ?
    if it is correct, need I add them as patch comments, too ?
    else (incorrect), please give more descriptions, thanks.


  :-)

-- 
Chen Gang

Asianux Corporation

RE: [PATCH] net/rds: using strlcpy instead of strncpy

[Ben Hutchings]

On Thu, 2013-02-28 at 09:36 +0000, David Laight wrote:
> >   when src strlen is sizeof(ctr.name) - 1, the dest str is not '\0' end.
> >   better use strlcpy instead.
> > 
> > Signed-off-by: Chen Gang <gang.chen@asianux.com>
> > ---
> >  net/rds/stats.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/net/rds/stats.c b/net/rds/stats.c
> > index 7be790d..b9ac1df 100644
> > --- a/net/rds/stats.c
> > +++ b/net/rds/stats.c
> > @@ -86,7 +86,7 @@ void rds_stats_info_copy(struct rds_info_iterator *iter,
> > 
> >  	for (i = 0; i < nr; i++) {
> >  		BUG_ON(strlen(names[i]) >= sizeof(ctr.name));
> > -		strncpy(ctr.name, names[i], sizeof(ctr.name) - 1);
> > +		strlcpy(ctr.name, names[i], sizeof(ctr.name));
> >  		ctr.value = values[i];
> > 
> 
> If the target buffer ends up being copied to userspace that
> might lead to random kernel memory being leaked.

Seems it is.  The last byte of 'name' is not currently initialised and
therefore is already leaked to userland.

But it's OK because rds_info_copy() uses memcpy() not __copy_to_user(),
so SMAP will block this leak. :-)

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

RE: [PATCH] net/rds: using strlcpy instead of strncpy

[Ben Hutchings]

On Mon, 2013-03-04 at 18:32 +0000, Ben Hutchings wrote:
> On Thu, 2013-02-28 at 09:36 +0000, David Laight wrote:
> > >   when src strlen is sizeof(ctr.name) - 1, the dest str is not '\0' end.
> > >   better use strlcpy instead.
> > > 
> > > Signed-off-by: Chen Gang <gang.chen@asianux.com>
> > > ---
> > >  net/rds/stats.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/net/rds/stats.c b/net/rds/stats.c
> > > index 7be790d..b9ac1df 100644
> > > --- a/net/rds/stats.c
> > > +++ b/net/rds/stats.c
> > > @@ -86,7 +86,7 @@ void rds_stats_info_copy(struct rds_info_iterator *iter,
> > > 
> > >  	for (i = 0; i < nr; i++) {
> > >  		BUG_ON(strlen(names[i]) >= sizeof(ctr.name));
> > > -		strncpy(ctr.name, names[i], sizeof(ctr.name) - 1);
> > > +		strlcpy(ctr.name, names[i], sizeof(ctr.name));
> > >  		ctr.value = values[i];
> > > 
> > 
> > If the target buffer ends up being copied to userspace that
> > might lead to random kernel memory being leaked.
> 
> Seems it is.  The last byte of 'name' is not currently initialised and
> therefore is already leaked to userland.
> 
> But it's OK because rds_info_copy() uses memcpy() not __copy_to_user(),
> so SMAP will block this leak. :-)

Or not, as kmap() presumably evades that.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] net/rds: using strlcpy instead of strncpy

[Chen Gang]

于 2013年03月05日 02:34, Ben Hutchings 写道:
> On Mon, 2013-03-04 at 18:32 +0000, Ben Hutchings wrote:
>> > On Thu, 2013-02-28 at 09:36 +0000, David Laight wrote:
>>> > > 
>>> > > If the target buffer ends up being copied to userspace that
>>> > > might lead to random kernel memory being leaked.
>> > 
>> > Seems it is.  The last byte of 'name' is not currently initialised and
>> > therefore is already leaked to userland.
>> > 
>> > But it's OK because rds_info_copy() uses memcpy() not __copy_to_user(),
>> > so SMAP will block this leak. :-)
> Or not, as kmap() presumably evades that.

 is this patch ok, or need improving ?

BTW:
  excuse me, maybe my reply will be late during this week.
  the reason:
    my father had a serious heart disease, and is in hospital.
    during these days, most of my time has to be in hospital.
      (God Bless, and thank Jesus Christ, my father is safe, now).
    within my company (Asianux), I also have something to do.


 :-)

-- 
Chen Gang

Asianux Corporation

Re: [PATCH] net/rds: using strlcpy instead of strncpy

[Ben Hutchings]

On Tue, 2013-03-05 at 10:32 +0800, Chen Gang wrote:
> 于 2013年03月05日 02:34, Ben Hutchings 写道:
> > On Mon, 2013-03-04 at 18:32 +0000, Ben Hutchings wrote:
> >> > On Thu, 2013-02-28 at 09:36 +0000, David Laight wrote:
> >>> > > 
> >>> > > If the target buffer ends up being copied to userspace that
> >>> > > might lead to random kernel memory being leaked.
> >> > 
> >> > Seems it is.  The last byte of 'name' is not currently initialised and
> >> > therefore is already leaked to userland.
> >> > 
> >> > But it's OK because rds_info_copy() uses memcpy() not __copy_to_user(),
> >> > so SMAP will block this leak. :-)
> > Or not, as kmap() presumably evades that.
> 
>  is this patch ok, or need improving ?

I think it is wrong, and the code should be changed to do either:

1. Zero-initialise the whole of the name, then use strlcpy().
2. Keep using strncpy(), and also set the last byte of name to 0.

> BTW:
>   excuse me, maybe my reply will be late during this week.
>   the reason:
>     my father had a serious heart disease, and is in hospital.
>     during these days, most of my time has to be in hospital.
>       (God Bless, and thank Jesus Christ, my father is safe, now).
>     within my company (Asianux), I also have something to do.

Best wishes to you both.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] net/rds: using strlcpy instead of strncpy

[Chen Gang]

于 2013年03月05日 11:16, Ben Hutchings 写道:
> I think it is wrong, and the code should be changed to do either:
> 
> 1. Zero-initialise the whole of the name, then use strlcpy().
> 2. Keep using strncpy(), and also set the last byte of name to 0.
> 

  I think what I have done is just like your choice "2."
  for me, I think they are equal:

-		strncpy(ctr.name, names[i], sizeof(ctr.name) - 1);
+		strlcpy(ctr.name, names[i], sizeof(ctr.name));


		strncpy(ctr.name, names[i], sizeof(ctr.name) - 1);
+ 		ctr.name[sizeof(ctr.name) - 1] = '\0';



>> > BTW:
>> >   excuse me, maybe my reply will be late during this week.
>> >   the reason:
>> >     my father had a serious heart disease, and is in hospital.
>> >     during these days, most of my time has to be in hospital.
>> >       (God Bless, and thank Jesus Christ, my father is safe, now).
>> >     within my company (Asianux), I also have something to do.
> Best wishes to you both.

  thank you.

  :-)

-- 
Chen Gang

Asianux Corporation

Re: [PATCH] net/rds: using strlcpy instead of strncpy

[Ben Hutchings]

On Tue, 2013-03-05 at 11:32 +0800, Chen Gang wrote:
> 于 2013年03月05日 11:16, Ben Hutchings 写道:
> > I think it is wrong, and the code should be changed to do either:
> > 
> > 1. Zero-initialise the whole of the name, then use strlcpy().
> > 2. Keep using strncpy(), and also set the last byte of name to 0.
> > 
> 
>   I think what I have done is just like your choice "2."
>   for me, I think they are equal:
> 
> -		strncpy(ctr.name, names[i], sizeof(ctr.name) - 1);
> +		strlcpy(ctr.name, names[i], sizeof(ctr.name));
> 
> 
> 		strncpy(ctr.name, names[i], sizeof(ctr.name) - 1);
> + 		ctr.name[sizeof(ctr.name) - 1] = '\0';

They are not.  strncpy() pads with zeroes to the end of the given buffer
whereas strlcpy() adds only a single zero byte (and truncates if
necessary to fit the zero byte).

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] net/rds: using strlcpy instead of strncpy

[Chen Gang]

于 2013年03月05日 11:37, Ben Hutchings 写道:
>>   I think what I have done is just like your choice "2."
>> >   for me, I think they are equal:
>> > 
>> > -		strncpy(ctr.name, names[i], sizeof(ctr.name) - 1);
>> > +		strlcpy(ctr.name, names[i], sizeof(ctr.name));
>> > 
>> > 
>> > 		strncpy(ctr.name, names[i], sizeof(ctr.name) - 1);
>> > + 		ctr.name[sizeof(ctr.name) - 1] = '\0';
> They are not.  strncpy() pads with zeroes to the end of the given buffer
> whereas strlcpy() adds only a single zero byte (and truncates if
> necessary to fit the zero byte).

  ok, thank you. they are really not the same (originally, I did not
notice it)

  could you supply the reason:
    why need we zero all of ctr.name ?
    (for me, I think, keeping ctr.name just a zero-based string is ok)

  thanks.

  :-)

-- 
Chen Gang

Asianux Corporation

Re: [PATCH] net/rds: using strlcpy instead of strncpy

[Ben Hutchings]

On Tue, 2013-03-05 at 12:09 +0800, Chen Gang wrote:
> 于 2013年03月05日 11:37, Ben Hutchings 写道:
> >>   I think what I have done is just like your choice "2."
> >> >   for me, I think they are equal:
> >> > 
> >> > -		strncpy(ctr.name, names[i], sizeof(ctr.name) - 1);
> >> > +		strlcpy(ctr.name, names[i], sizeof(ctr.name));
> >> > 
> >> > 
> >> > 		strncpy(ctr.name, names[i], sizeof(ctr.name) - 1);
> >> > + 		ctr.name[sizeof(ctr.name) - 1] = '\0';
> > They are not.  strncpy() pads with zeroes to the end of the given buffer
> > whereas strlcpy() adds only a single zero byte (and truncates if
> > necessary to fit the zero byte).
> 
>   ok, thank you. they are really not the same (originally, I did not
> notice it)
> 
>   could you supply the reason:
>     why need we zero all of ctr.name ?
>     (for me, I think, keeping ctr.name just a zero-based string is ok)

This function calls rds_copy_info() to copy the whole of ctr into
userland.

If ctr is not completely initialised, then the values of the
uninitialised bytes are left over from the local variables of an earlier
system call.  If an attacker knows enough about the stack layout (easy
if this is a distribution kernel), they can make a series of system
calls that leak information about heap-allocated objects.  That can help
them to exploit other kernel bugs for privilege escalation.  So we
should initialise every bit of memory that is going to be copied to
userland.

(In fact, in general it's not even enough to initialise all fields of
the structure, because there may be padding bytes between them.  In this
case we know there isn't, because it's declared as packed.)

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] net/rds: using strlcpy instead of strncpy

[Chen Gang]

于 2013年03月06日 01:00, Ben Hutchings 写道:
> This function calls rds_copy_info() to copy the whole of ctr into
> userland.
> 
> If ctr is not completely initialised, then the values of the
> uninitialised bytes are left over from the local variables of an earlier
> system call.  If an attacker knows enough about the stack layout (easy
> if this is a distribution kernel), they can make a series of system
> calls that leak information about heap-allocated objects.  That can help
> them to exploit other kernel bugs for privilege escalation.  So we
> should initialise every bit of memory that is going to be copied to
> userland.
> 
> (In fact, in general it's not even enough to initialise all fields of
> the structure, because there may be padding bytes between them.  In this
> case we know there isn't, because it's declared as packed.)
> 
> Ben.

  thank you for your information.

  I should send patch v2 for it.

  :-)


-- 
Chen Gang

Asianux Corporation

[PATCH] net/rds: zero last byte for strncpy

[Chen Gang]

  for NUL terminated string, need be always sure '\0' in the end.

additional info:
  strncpy will pads with zeroes to the end of the given buffer.
  should initialise every bit of memory that is going to be copied to userland


Signed-off-by: Chen Gang <gang.chen@asianux.com>
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 net/rds/stats.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/net/rds/stats.c b/net/rds/stats.c
index 7be790d..73be187 100644
--- a/net/rds/stats.c
+++ b/net/rds/stats.c
@@ -87,6 +87,7 @@ void rds_stats_info_copy(struct rds_info_iterator *iter,
 	for (i = 0; i < nr; i++) {
 		BUG_ON(strlen(names[i]) >= sizeof(ctr.name));
 		strncpy(ctr.name, names[i], sizeof(ctr.name) - 1);
+		ctr.name[sizeof(ctr.name) - 1] = '\0';
 		ctr.value = values[i];
 
 		rds_info_copy(iter, &ctr, sizeof(ctr));
-- 
1.7.7.6

Re: [PATCH] net/rds: zero last byte for strncpy

[David Miller]

From: Chen Gang <gang.chen@asianux.com>
Date: Fri, 08 Mar 2013 11:19:32 +0800

> 
>   for NUL terminated string, need be always sure '\0' in the end.
> 
> additional info:
>   strncpy will pads with zeroes to the end of the given buffer.
>   should initialise every bit of memory that is going to be copied to userland
> 
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>

Applied.

Re: [PATCH] net/rds: zero last byte for strncpy

[Chen Gang]

于 2013年03月08日 13:36, David Miller 写道:
> From: Chen Gang <gang.chen@asianux.com>
> Date: Fri, 08 Mar 2013 11:19:32 +0800
> 
>>
>>   for NUL terminated string, need be always sure '\0' in the end.
>>
>> additional info:
>>   strncpy will pads with zeroes to the end of the given buffer.
>>   should initialise every bit of memory that is going to be copied to userland
>>
>>
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> 
> Applied.
> 
> 

  thanks.

-- 
Chen Gang

Asianux Corporation