netdev - Re: [PATCH] net/rds: 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:	Tue, 5 Mar 2013 17:00:44 +0000
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Chen Gang <gang.chen@...anux.com>
CC:	David Laight <David.Laight@...LAB.COM>,
	<venkat.x.venkatsubra@...cle.com>,
	David Miller <davem@...emloft.net>, <rds-devel@....oracle.com>,
	netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH] net/rds: using strlcpy instead of strncpy

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.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html