| Message ID | 512F1534.9020404@asianux.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
> 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
于 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. :-)
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.
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.
于 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. :-)
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.
于 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. :-)
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.
于 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. :-)
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.
于 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. :-)
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));
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(-)