[PATCH] kernel: module: strncpy issue, using strlcpy instead of strncpy

[PATCH] kernel: module: strncpy issue, using strlcpy instead of strncpy

[Chen Gang]

  ownername and namebuf are all NUL terminated string.

  need always let them ended by '\0'.

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

diff --git a/kernel/module.c b/kernel/module.c
index 3c2c72d..597efd8 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1283,7 +1283,7 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod,
 
 getname:
 	/* We must make copy under the lock if we failed to get ref. */
-	strncpy(ownername, module_name(owner), MODULE_NAME_LEN);
+	strlcpy(ownername, module_name(owner), MODULE_NAME_LEN);
 unlock:
 	mutex_unlock(&module_mutex);
 	return sym;
@@ -3464,7 +3464,7 @@ const char *module_address_lookup(unsigned long addr,
 	}
 	/* Make a copy in here where it's safe */
 	if (ret) {
-		strncpy(namebuf, ret, KSYM_NAME_LEN - 1);
+		strlcpy(namebuf, ret, KSYM_NAME_LEN);
 		ret = namebuf;
 	}
 	preempt_enable();
-- 
1.7.7.6

Re: [PATCH] kernel: module: strncpy issue, using strlcpy instead of strncpy

[Geert Uytterhoeven]

On Sun, Apr 7, 2013 at 1:38 PM, Chen Gang <gang.chen@asianux.com> wrote:
>   ownername and namebuf are all NUL terminated string.
>
>   need always let them ended by '\0'.
>
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  kernel/module.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index 3c2c72d..597efd8 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c

> @@ -3464,7 +3464,7 @@ const char *module_address_lookup(unsigned long addr,
>         }
>         /* Make a copy in here where it's safe */
>         if (ret) {
> -               strncpy(namebuf, ret, KSYM_NAME_LEN - 1);
> +               strlcpy(namebuf, ret, KSYM_NAME_LEN);
>                 ret = namebuf;
>         }
>         preempt_enable();

Is this buffer ever copied to userspace?
If yes, it may leak innocent kernel stack to userspace, as strlcpy()
doesn't fill
the remaining of the buffer with zeroes, while strncpy() does.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] kernel: module: strncpy issue, using strlcpy instead of strncpy

[Chen Gang]

On 2013年04月07日 22:28, Geert Uytterhoeven wrote:
> On Sun, Apr 7, 2013 at 1:38 PM, Chen Gang <gang.chen@asianux.com> wrote:
>> >   ownername and namebuf are all NUL terminated string.
>> >
>> >   need always let them ended by '\0'.
>> >
>> > Signed-off-by: Chen Gang <gang.chen@asianux.com>
>> > ---
>> >  kernel/module.c |    4 ++--
>> >  1 files changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/kernel/module.c b/kernel/module.c
>> > index 3c2c72d..597efd8 100644
>> > --- a/kernel/module.c
>> > +++ b/kernel/module.c
>> > @@ -3464,7 +3464,7 @@ const char *module_address_lookup(unsigned long addr,
>> >         }
>> >         /* Make a copy in here where it's safe */
>> >         if (ret) {
>> > -               strncpy(namebuf, ret, KSYM_NAME_LEN - 1);
>> > +               strlcpy(namebuf, ret, KSYM_NAME_LEN);
>> >                 ret = namebuf;
>> >         }
>> >         preempt_enable();
> Is this buffer ever copied to userspace?


at lease now:
  I think, it is not, the reason is:
    it is only a tool function for kallsyms using.
    it has no duty to let namebuf initialized.

  please reference the related comments in include/linux/module.h

493 /* For kallsyms to ask for address resolution.  namebuf should be at
494  * least KSYM_NAME_LEN long: a pointer to namebuf is returned if
495  * found, otherwise NULL. */
496 const char *module_address_lookup(unsigned long addr,
497                             unsigned long *symbolsize,
498                             unsigned long *offset,
499                             char **modname,
500                             char *namebuf);


originally:
  it will not cause issue (the upper caller has noticed it).
  but we really need let it '\0' ended within module_address_lookup.
    (so, maybe for subject: "strncpy issue" need be deleted)


in the future:
  since it is an extern function, it can be used by others.
  since it is a tool function, it can not be used directly by user mode.
  according to the api definition:
    if it is necessary to initialize (such as return to user mode)
      the caller should perform it.
    if it is not necessary to initialize (not return to user mode)
      still prefer the caller to initialize it.
      but should understand if the caller will not initialize it.
        (if caller does not initialize it, it should not cause issue)


  thanks.

  :-)


-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel: module: strncpy issue, using strlcpy instead of strncpy

[Chen Gang]

On 2013年04月08日 10:48, Chen Gang wrote:
> On 2013年04月07日 22:28, Geert Uytterhoeven wrote:
>> On Sun, Apr 7, 2013 at 1:38 PM, Chen Gang <gang.chen@asianux.com> wrote:
>>>>   ownername and namebuf are all NUL terminated string.
>>>>
>>>>   need always let them ended by '\0'.
>>>>
>>>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>>>> ---
>>>>  kernel/module.c |    4 ++--
>>>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/kernel/module.c b/kernel/module.c
>>>> index 3c2c72d..597efd8 100644
>>>> --- a/kernel/module.c
>>>> +++ b/kernel/module.c
>>>> @@ -3464,7 +3464,7 @@ const char *module_address_lookup(unsigned long addr,
>>>>         }
>>>>         /* Make a copy in here where it's safe */
>>>>         if (ret) {
>>>> -               strncpy(namebuf, ret, KSYM_NAME_LEN - 1);
>>>> +               strlcpy(namebuf, ret, KSYM_NAME_LEN);
>>>>                 ret = namebuf;
>>>>         }
>>>>         preempt_enable();
>> Is this buffer ever copied to userspace?
> 
> 

  the key words is:
    if it knows the length of namebuf, it may need initialized namebuf.
    if it can not know the length of namebuf
      it can not initialize namebuf.
      (in our case, it only know, the namebuf length >= KSYM_NAME_LEN)

  so, it need not initialize it.

  :-)

  thanks.

gchen.


> at lease now:
>   I think, it is not, the reason is:
>     it is only a tool function for kallsyms using.
>     it has no duty to let namebuf initialized.
> 
>   please reference the related comments in include/linux/module.h
> 
> 493 /* For kallsyms to ask for address resolution.  namebuf should be at
> 494  * least KSYM_NAME_LEN long: a pointer to namebuf is returned if
> 495  * found, otherwise NULL. */
> 496 const char *module_address_lookup(unsigned long addr,
> 497                             unsigned long *symbolsize,
> 498                             unsigned long *offset,
> 499                             char **modname,
> 500                             char *namebuf);
> 
> 
> originally:
>   it will not cause issue (the upper caller has noticed it).
>   but we really need let it '\0' ended within module_address_lookup.
>     (so, maybe for subject: "strncpy issue" need be deleted)
> 
> 
> in the future:
>   since it is an extern function, it can be used by others.
>   since it is a tool function, it can not be used directly by user mode.
>   according to the api definition:
>     if it is necessary to initialize (such as return to user mode)
>       the caller should perform it.
>     if it is not necessary to initialize (not return to user mode)
>       still prefer the caller to initialize it.
>       but should understand if the caller will not initialize it.
>         (if caller does not initialize it, it should not cause issue)
> 
> 
>   thanks.
> 
>   :-)
> 
> 


-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel: module: strncpy issue, using strlcpy instead of strncpy

[Rusty Russell]

Chen Gang <gang.chen@asianux.com> writes:
>   ownername and namebuf are all NUL terminated string.
>
>   need always let them ended by '\0'.
>
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  kernel/module.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index 3c2c72d..597efd8 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1283,7 +1283,7 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod,
>  
>  getname:
>  	/* We must make copy under the lock if we failed to get ref. */
> -	strncpy(ownername, module_name(owner), MODULE_NAME_LEN);
> +	strlcpy(ownername, module_name(owner), MODULE_NAME_LEN);

This should just be strcpy().

>  unlock:
>  	mutex_unlock(&module_mutex);
>  	return sym;
> @@ -3464,7 +3464,7 @@ const char *module_address_lookup(unsigned long addr,
>  	}
>  	/* Make a copy in here where it's safe */
>  	if (ret) {
> -		strncpy(namebuf, ret, KSYM_NAME_LEN - 1);
> +		strlcpy(namebuf, ret, KSYM_NAME_LEN);

This isn't a bug, because the caller (kallsyms_lookup) puts a NUL in
ret[KSYM_NAME_LEN].

However, kallsyms_lookup also calls kallsyms_expand_symbol, which
doesn't stop at KSYM_NAME_LEN, so if a name was longer than that we'd
have a real bug.

Would you like to take a look at that, too?

Thanks,
Rusty.

Re: [PATCH] kernel: module: strncpy issue, using strlcpy instead of strncpy

[Chen Gang]

On 2013年04月08日 13:30, Rusty Russell wrote:
> Chen Gang <gang.chen@asianux.com> writes:
>> >   ownername and namebuf are all NUL terminated string.
>> >
>> >   need always let them ended by '\0'.
>> >
>> > Signed-off-by: Chen Gang <gang.chen@asianux.com>
>> > ---
>> >  kernel/module.c |    4 ++--
>> >  1 files changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/kernel/module.c b/kernel/module.c
>> > index 3c2c72d..597efd8 100644
>> > --- a/kernel/module.c
>> > +++ b/kernel/module.c
>> > @@ -1283,7 +1283,7 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod,
>> >  
>> >  getname:
>> >  	/* We must make copy under the lock if we failed to get ref. */
>> > -	strncpy(ownername, module_name(owner), MODULE_NAME_LEN);
>> > +	strlcpy(ownername, module_name(owner), MODULE_NAME_LEN);
> This should just be strcpy().
> 

  for me, either strcpy or strlcpy are ok.
    strcpy is quicker than strlcpy (in our case, it seems not quite important).
    strlcpy is more clearer to readers (they do not care about the buffer length again).

  since you prefer strcpy, I need respect your (the original maintainer's) willing.
  so I need change to strcpy.

  :-)


>> >  unlock:
>> >  	mutex_unlock(&module_mutex);
>> >  	return sym;
>> > @@ -3464,7 +3464,7 @@ const char *module_address_lookup(unsigned long addr,
>> >  	}
>> >  	/* Make a copy in here where it's safe */
>> >  	if (ret) {
>> > -		strncpy(namebuf, ret, KSYM_NAME_LEN - 1);
>> > +		strlcpy(namebuf, ret, KSYM_NAME_LEN);
> This isn't a bug, because the caller (kallsyms_lookup) puts a NUL in
> ret[KSYM_NAME_LEN].
> 

  originally, it is really not a bug (so subject need delete "strncpy issue").
  now, I still prefer to set tail '\0' in function module_address_lookup.
  future, if it is used by others, it is necessary to set tail '\0' in this function.



and for this patch, is it suitable to send patch v2 ?




> However, kallsyms_lookup also calls kallsyms_expand_symbol, which
> doesn't stop at KSYM_NAME_LEN, so if a name was longer than that we'd
> have a real bug.
> 
> Would you like to take a look at that, too?
> 

  it looks like a bug.  for me, I prefer to give length check for it.

  but I am sorry, now, I can not be sure whether it is really a bug.

    I have to need additional time resources to make sure about it.
      I am not quite familiar with kernel (especially kernel of kernel).
      I even do not know about kallsyms_names (it seems not a normal variable)
      I have to spend time resources to process other things within company.

  so I think I should make sure whether it is a bug, before 2013-4-20, is it ok ?

  welcome any suggestions or completions.


> Thanks,
> Rusty.
> 
> 


-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel: module: strncpy issue, using strlcpy instead of strncpy

[Rusty Russell]

Chen Gang <gang.chen@asianux.com> writes:
> On 2013年04月08日 13:30, Rusty Russell wrote:
>> Chen Gang <gang.chen@asianux.com> writes:
>>> >   ownername and namebuf are all NUL terminated string.
>>> >
>>> >   need always let them ended by '\0'.
>>> >
>>> > Signed-off-by: Chen Gang <gang.chen@asianux.com>
>>> > ---
>>> >  kernel/module.c |    4 ++--
>>> >  1 files changed, 2 insertions(+), 2 deletions(-)
>>> >
>>> > diff --git a/kernel/module.c b/kernel/module.c
>>> > index 3c2c72d..597efd8 100644
>>> > --- a/kernel/module.c
>>> > +++ b/kernel/module.c
>>> > @@ -1283,7 +1283,7 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod,
>>> >  
>>> >  getname:
>>> >  	/* We must make copy under the lock if we failed to get ref. */
>>> > -	strncpy(ownername, module_name(owner), MODULE_NAME_LEN);
>>> > +	strlcpy(ownername, module_name(owner), MODULE_NAME_LEN);
>> This should just be strcpy().
>> 
>
>   for me, either strcpy or strlcpy are ok.
>     strcpy is quicker than strlcpy (in our case, it seems not quite important).
>     strlcpy is more clearer to readers (they do not care about the buffer length again).
>
>   since you prefer strcpy, I need respect your (the original maintainer's) willing.
>   so I need change to strcpy.
>
>   :-)
>
>
>>> >  unlock:
>>> >  	mutex_unlock(&module_mutex);
>>> >  	return sym;
>>> > @@ -3464,7 +3464,7 @@ const char *module_address_lookup(unsigned long addr,
>>> >  	}
>>> >  	/* Make a copy in here where it's safe */
>>> >  	if (ret) {
>>> > -		strncpy(namebuf, ret, KSYM_NAME_LEN - 1);
>>> > +		strlcpy(namebuf, ret, KSYM_NAME_LEN);
>> This isn't a bug, because the caller (kallsyms_lookup) puts a NUL in
>> ret[KSYM_NAME_LEN].
>> 
>
>   originally, it is really not a bug (so subject need delete "strncpy issue").
>   now, I still prefer to set tail '\0' in function module_address_lookup.
>   future, if it is used by others, it is necessary to set tail '\0' in this function.
>
>
>
> and for this patch, is it suitable to send patch v2 ?

Yes, that's fine.

>> However, kallsyms_lookup also calls kallsyms_expand_symbol, which
>> doesn't stop at KSYM_NAME_LEN, so if a name was longer than that we'd
>> have a real bug.
>> 
>> Would you like to take a look at that, too?
>> 
>
>   it looks like a bug.  for me, I prefer to give length check for it.
>
>   but I am sorry, now, I can not be sure whether it is really a bug.

It really is.  We don't export any symbols > 128 characters, but if we
did then kallsyms_expand_symbol() would overflow the buffer handed to
it.

Your suggestion about an explicit length for kallsyms_expand_symbol() is
the correct one.

(This is a separate patch to the cleanup above).

Thanks,
Rusty.

Re: [PATCH] kernel: module: strncpy issue, using strlcpy instead of strncpy

[Chen Gang]

On 2013年04月08日 21:45, Rusty Russell wrote:
>> >
>> > and for this patch, is it suitable to send patch v2 ?
> Yes, that's fine.
> 

  thanks, I will send patch v2 (the subject need delete "strncpy issue")


>>> >> However, kallsyms_lookup also calls kallsyms_expand_symbol, which
>>> >> doesn't stop at KSYM_NAME_LEN, so if a name was longer than that we'd
>>> >> have a real bug.
>>> >> 
>>> >> Would you like to take a look at that, too?
>>> >> 
>> >
>> >   it looks like a bug.  for me, I prefer to give length check for it.
>> >
>> >   but I am sorry, now, I can not be sure whether it is really a bug.
> It really is.  We don't export any symbols > 128 characters, but if we
> did then kallsyms_expand_symbol() would overflow the buffer handed to
> it.
> 
> Your suggestion about an explicit length for kallsyms_expand_symbol() is
> the correct one.


  thank you to give me additional chance to send a new patch,
  I will send another patch for it.

  since you find it, firstly, and also give a confirmation.
  can I add you as "Signed-of-by", too ?

  and excuse me:
    I think I should spend time resources to have a test for new patch.
    so I will finish it within this weekend (2013-4-14), is it OK ?


  thanks.


-- 
Chen Gang

Asianux Corporation

[PATCH v2] kernel: module: using strlcpy and strcpy instead of strncpy

[Chen Gang]

  namebuf is NUL terminated string.  better always let it ended by '\0'.
  ownername and module_name(owner) are the same buf len. strcpy is better.


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

diff --git a/kernel/module.c b/kernel/module.c
index 3c2c72d..09aeefd 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1283,7 +1283,7 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod,
 
 getname:
 	/* We must make copy under the lock if we failed to get ref. */
-	strncpy(ownername, module_name(owner), MODULE_NAME_LEN);
+	strcpy(ownername, module_name(owner));
 unlock:
 	mutex_unlock(&module_mutex);
 	return sym;
@@ -3464,7 +3464,7 @@ const char *module_address_lookup(unsigned long addr,
 	}
 	/* Make a copy in here where it's safe */
 	if (ret) {
-		strncpy(namebuf, ret, KSYM_NAME_LEN - 1);
+		strlcpy(namebuf, ret, KSYM_NAME_LEN);
 		ret = namebuf;
 	}
 	preempt_enable();
-- 
1.7.7.6

Re: [PATCH] kernel: module: strncpy issue, using strlcpy instead of strncpy

[Chen Gang]

On 2013年04月09日 09:52, Chen Gang wrote:
>>>>
>>>>   it looks like a bug.  for me, I prefer to give length check for it.
>>>>
>>>>   but I am sorry, now, I can not be sure whether it is really a bug.
>> It really is.  We don't export any symbols > 128 characters, but if we
>> did then kallsyms_expand_symbol() would overflow the buffer handed to
>> it.
>>
>> Your suggestion about an explicit length for kallsyms_expand_symbol() is
>> the correct one.
> 
> 

  oh, sorry, after read the related source code, I think:

    for kernel/kallsyms.c, it is no issue
      (although I still also prefer to give a length checking).

    the reason is:
      scripts/kallsyms.c does not check the 128 limitation.
      if compiler limits it with 128 automatically, we will have no issue.
      else the scripts/kallsyms will cause issue.

    so whether what happens, kernel/kallsyms.c will not cause issue.

    :-)


  the related patch for scripts/kallsyms.c is below, please check, thanks.
    (now, it is just for a reference, after have a test, I will send).


-----------------------patch begin--------------------------------------

diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
index 487ac6f..9ec6d1f 100644
--- a/scripts/kallsyms.c
+++ b/scripts/kallsyms.c
@@ -145,13 +145,15 @@ static int read_symbol(FILE *in, struct sym_entry *s)
 	/* include the type field in the symbol name, so that it gets
 	 * compressed together */
 	s->len = strlen(str) + 1;
+	if (s->len > KSYM_NAME_LEN)
+		s->len = KSYM_NAME_LEN;
 	s->sym = malloc(s->len + 1);
 	if (!s->sym) {
 		fprintf(stderr, "kallsyms failure: "
 			"unable to allocate required amount of memory\n");
 		exit(EXIT_FAILURE);
 	}
-	strcpy((char *)s->sym + 1, str);
+	strlcpy((char *)s->sym + 1, str, KSYM_NAME_LEN);
 	s->sym[0] = stype;
 
 	return 0;
@@ -290,7 +292,7 @@ static void write_src(void)
 	unsigned int i, k, off;
 	unsigned int best_idx[256];
 	unsigned int *markers;
-	char buf[KSYM_NAME_LEN];
+	char buf[KSYM_NAME_LEN + 1];
 
 	printf("#include <asm/types.h>\n");
 	printf("#if BITS_PER_LONG == 64\n");
-- 
1.7.7.6


-----------------------patch end----------------------------------------



>   thank you to give me additional chance to send a new patch,
>   I will send another patch for it.
> 
>   since you find it, firstly, and also give a confirmation.
>   can I add you as "Signed-of-by", too ?
> 
>   and excuse me:
>     I think I should spend time resources to have a test for new patch.
>     so I will finish it within this weekend (2013-4-14), is it OK ?
> 
> 
>   thanks.
> 
> 


-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel: module: strncpy issue, using strlcpy instead of strncpy

[Chen Gang]

On 2013年04月09日 17:36, Chen Gang wrote:
> diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
> index 487ac6f..9ec6d1f 100644
> --- a/scripts/kallsyms.c
> +++ b/scripts/kallsyms.c
> @@ -145,13 +145,15 @@ static int read_symbol(FILE *in, struct sym_entry *s)
>  	/* include the type field in the symbol name, so that it gets
>  	 * compressed together */
>  	s->len = strlen(str) + 1;
> +	if (s->len > KSYM_NAME_LEN)
> +		s->len = KSYM_NAME_LEN;
>  	s->sym = malloc(s->len + 1);
>  	if (!s->sym) {
>  		fprintf(stderr, "kallsyms failure: "
>  			"unable to allocate required amount of memory\n");
>  		exit(EXIT_FAILURE);
>  	}
> -	strcpy((char *)s->sym + 1, str);
> +	strlcpy((char *)s->sym + 1, str, KSYM_NAME_LEN);
>  	s->sym[0] = stype;
>  

  oh... it is a user mode program which no strlcpy in lib C !!

  (so this diff is just really for a reference, not for the real using)

  :-)

-- 
Chen Gang

Asianux Corporation

Re: [PATCH v2] kernel: module: using strlcpy and strcpy instead of strncpy

[Rusty Russell]

Chen Gang <gang.chen@asianux.com> writes:
>   namebuf is NUL terminated string.  better always let it ended by '\0'.
>   ownername and module_name(owner) are the same buf len. strcpy is better.
>
>
> Signed-off-by: Chen Gang <gang.chen@asianux.com>

Would be better to describe the justificaiton for strcpy in
resolve_symbol(), eg.

For resolve_symbol() we just use strcpy: the module_name() is always the
name field of struct module (which is a fixed array), or a literal "kernel".

Cheers,
Rusty.

> ---
>  kernel/module.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index 3c2c72d..09aeefd 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1283,7 +1283,7 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod,
>  
>  getname:
>  	/* We must make copy under the lock if we failed to get ref. */
> -	strncpy(ownername, module_name(owner), MODULE_NAME_LEN);
> +	strcpy(ownername, module_name(owner));
>  unlock:
>  	mutex_unlock(&module_mutex);
>  	return sym;
> @@ -3464,7 +3464,7 @@ const char *module_address_lookup(unsigned long addr,
>  	}
>  	/* Make a copy in here where it's safe */
>  	if (ret) {
> -		strncpy(namebuf, ret, KSYM_NAME_LEN - 1);
> +		strlcpy(namebuf, ret, KSYM_NAME_LEN);
>  		ret = namebuf;
>  	}
>  	preempt_enable();
> -- 
> 1.7.7.6

[PATCH v3] kernel: module: using strlcpy and strcpy instead of strncpy

[Chen Gang]

  namebuf is NUL terminated string.  better always let it ended by '\0'.

  the module_name() is always the name field of struct module (which is
  a fixed array), or a literal "kernel", so strcpy is better.


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

diff --git a/kernel/module.c b/kernel/module.c
index 3c2c72d..09aeefd 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1283,7 +1283,7 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod,
 
 getname:
 	/* We must make copy under the lock if we failed to get ref. */
-	strncpy(ownername, module_name(owner), MODULE_NAME_LEN);
+	strcpy(ownername, module_name(owner));
 unlock:
 	mutex_unlock(&module_mutex);
 	return sym;
@@ -3464,7 +3464,7 @@ const char *module_address_lookup(unsigned long addr,
 	}
 	/* Make a copy in here where it's safe */
 	if (ret) {
-		strncpy(namebuf, ret, KSYM_NAME_LEN - 1);
+		strlcpy(namebuf, ret, KSYM_NAME_LEN);
 		ret = namebuf;
 	}
 	preempt_enable();
-- 
1.7.7.6

Re: [PATCH] kernel: module: strncpy issue, using strlcpy instead of strncpy

[Chen Gang]

On 2013年04月09日 17:36, Chen Gang wrote:
> On 2013年04月09日 09:52, Chen Gang wrote:
>>>>>
>>>>>   it looks like a bug.  for me, I prefer to give length check for it.
>>>>>
>>>>>   but I am sorry, now, I can not be sure whether it is really a bug.
>>> It really is.  We don't export any symbols > 128 characters, but if we
>>> did then kallsyms_expand_symbol() would overflow the buffer handed to
>>> it.
>>>
>>> Your suggestion about an explicit length for kallsyms_expand_symbol() is
>>> the correct one.
>>
>>

  after have a test, what you said above is correct.


  it is my fault:
    the scripts/kallsyms.c use KSYM_NAME_LEN (also defined as 128).
    but this macro is not for symbol name (it is for token name !!).
    its symbol max length is 500 (use hard code number, no comments)

  if symbol name length is not less than 128, kernel need check it.
    I made a driver to call function kallsyms_on_each_symbol.
    also add a patch to check the length in kernel.
    it really has effect.
      System.map still has the full symbol name when len >= 128.
      kallsyms_on_each_symbol truncates the name to 127, when len >= 128

  I will send the related patch.
    can I mark you as Signed-of-by too (you find and be sure about it) ?


  :-)


gchen.

> 
>   oh, sorry, after read the related source code, I think:
> 
>     for kernel/kallsyms.c, it is no issue
>       (although I still also prefer to give a length checking).
> 
>     the reason is:
>       scripts/kallsyms.c does not check the 128 limitation.
>       if compiler limits it with 128 automatically, we will have no issue.
>       else the scripts/kallsyms will cause issue.
> 
>     so whether what happens, kernel/kallsyms.c will not cause issue.
> 
>     :-)
> 
> 
>   the related patch for scripts/kallsyms.c is below, please check, thanks.
>     (now, it is just for a reference, after have a test, I will send).
> 
> 
> -----------------------patch begin--------------------------------------
> 
> diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
> index 487ac6f..9ec6d1f 100644
> --- a/scripts/kallsyms.c
> +++ b/scripts/kallsyms.c
> @@ -145,13 +145,15 @@ static int read_symbol(FILE *in, struct sym_entry *s)
>  	/* include the type field in the symbol name, so that it gets
>  	 * compressed together */
>  	s->len = strlen(str) + 1;
> +	if (s->len > KSYM_NAME_LEN)
> +		s->len = KSYM_NAME_LEN;
>  	s->sym = malloc(s->len + 1);
>  	if (!s->sym) {
>  		fprintf(stderr, "kallsyms failure: "
>  			"unable to allocate required amount of memory\n");
>  		exit(EXIT_FAILURE);
>  	}
> -	strcpy((char *)s->sym + 1, str);
> +	strlcpy((char *)s->sym + 1, str, KSYM_NAME_LEN);
>  	s->sym[0] = stype;
>  
>  	return 0;
> @@ -290,7 +292,7 @@ static void write_src(void)
>  	unsigned int i, k, off;
>  	unsigned int best_idx[256];
>  	unsigned int *markers;
> -	char buf[KSYM_NAME_LEN];
> +	char buf[KSYM_NAME_LEN + 1];
>  
>  	printf("#include <asm/types.h>\n");
>  	printf("#if BITS_PER_LONG == 64\n");
> 


-- 
Chen Gang

Asianux Corporation

Re: [PATCH v3] kernel: module: using strlcpy and strcpy instead of strncpy

[Rusty Russell]

Chen Gang <gang.chen@asianux.com> writes:
>   namebuf is NUL terminated string.  better always let it ended by '\0'.
>
>   the module_name() is always the name field of struct module (which is
>   a fixed array), or a literal "kernel", so strcpy is better.

Thanks, applied.

Cheers,
Rusty.