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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 08 Apr 2013 18:16:00 +0800
From:	Chen Gang <gang.chen@...anux.com>
To:	Rusty Russell <rusty@...tcorp.com.au>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] kernel: module: strncpy issue, using strlcpy instead
 of strncpy

On 2013年04月08日 13:30, Rusty Russell wrote:
> Chen Gang <gang.chen@...anux.com> writes:
>> >   ownername and namebuf are all NUL terminated string.
>> >
>> >   need always let them ended by '\0'.
>> >
>> > Signed-off-by: Chen Gang <gang.chen@...anux.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/