Hi!
Attached is a patch against 2.6.0-test11 to convert all strcpy() calls
to their corresponding strlcpy() for IPv6. Compiled and tested.
Thanks!
In article <[email protected]> (at Thu, 27 Nov 2003 09:14:44 +0100), Felipe Alfaro Solana <[email protected]> says:
> Attached is a patch against 2.6.0-test11 to convert all strcpy() calls
> to their corresponding strlcpy() for IPv6. Compiled and tested.
I don't like this coding style.
diff -uNr linux-2.6.0-test11.orig/net/ipv6/ip6_tunnel.c linux-2.6.0-test11/net/ipv6/ip6_tunnel.c
--- linux-2.6.0-test11.orig/net/ipv6/ip6_tunnel.c 2003-11-26 21:42:56.000000000 +0100
+++ linux-2.6.0-test11/net/ipv6/ip6_tunnel.c 2003-11-27 00:27:09.000000000 +0100
- strcpy(t->parms.name, dev->name);
+ strlcpy(t->parms.name, dev->name, IFNAMSIZ);
sizeof(t->parms.name)
or something like that.
--yoshfuji
On Thu, 27 Nov 2003 17:33:20 +0900 (JST)
YOSHIFUJI Hideaki / _$B5HF#1QL@ <[email protected]> wrote:
> - strcpy(t->parms.name, dev->name);
> + strlcpy(t->parms.name, dev->name, IFNAMSIZ);
> sizeof(t->parms.name)
>
> or something like that.
I agree, using sizeof() is the less error prone way of
doing things like this.
Felipe could you please rewrite your patch like this?
Thank you.
On Thu, 2003-11-27 at 11:59, David S. Miller wrote:
> I agree, using sizeof() is the less error prone way of
> doing things like this.
>
> Felipe could you please rewrite your patch like this?
Done!
In article <[email protected]> (at Thu, 27 Nov 2003 13:04:04 +0100), Felipe Alfaro Solana <[email protected]> says:
> On Thu, 2003-11-27 at 11:59, David S. Miller wrote:
>
> > I agree, using sizeof() is the less error prone way of
> > doing things like this.
> >
> > Felipe could you please rewrite your patch like this?
>
> Done!
Thanks. Ok to me.
--yoshfuji
On Thu, Nov 27, 2003 at 09:09:53PM +0900, YOSHIFUJI Hideaki / ?$B5HF#1QL@?(B wrote:
> In article <[email protected]> (at Thu, 27 Nov 2003 13:04:04 +0100), Felipe Alfaro Solana <[email protected]> says:
>
> > On Thu, 2003-11-27 at 11:59, David S. Miller wrote:
> >
> > > I agree, using sizeof() is the less error prone way of
> > > doing things like this.
> > >
> > > Felipe could you please rewrite your patch like this?
> >
> > Done!
>
> Thanks. Ok to me.
I'm slightly cautious here, although I haven't read the patch yet.
Did anyone consider whether any of these structures were copied to
user space, and whether, as a result of this change, we're now
copying uninitialised data to users?
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 PCMCIA - http://pcmcia.arm.linux.org.uk/
2.6 Serial core
In article <[email protected]> (at Thu, 27 Nov 2003 19:46:02 +0000), Russell King <[email protected]> says:
> > > > I agree, using sizeof() is the less error prone way of
> > > > doing things like this.
> > > >
> > > > Felipe could you please rewrite your patch like this?
> > >
> > > Done!
> >
> > Thanks. Ok to me.
>
> I'm slightly cautious here, although I haven't read the patch yet.
> Did anyone consider whether any of these structures were copied to
> user space, and whether, as a result of this change, we're now
> copying uninitialised data to users?
I believe that it, to change from strcpy() to strlcpy(), just
eliminates possibility of buffer-overrun.
--yoshfuji
On Fri, Nov 28, 2003 at 04:54:13AM +0900, YOSHIFUJI Hideaki / ?$B5HF#1QL@?(B wrote:
> In article <[email protected]> (at Thu, 27 Nov 2003 19:46:02 +0000), Russell King <[email protected]> says:
> > I'm slightly cautious here, although I haven't read the patch yet.
> > Did anyone consider whether any of these structures were copied to
> > user space, and whether, as a result of this change, we're now
> > copying uninitialised data to users?
>
> I believe that it, to change from strcpy() to strlcpy(), just
> eliminates possibility of buffer-overrun.
While this is 100% correct, the bit which raised my attention was the
original message which didn't seem to show that the above had been
considered.
The thing that worries me is that an incorrect strlcpy() conversion
gives the impression that someone has thought about buffer underruns
as well as overruns, and, unless someone /has/ actually thought about
it, there could well still be a security problem lurking there.
I'm just overly wary of all strlcpy() conversions.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 PCMCIA - http://pcmcia.arm.linux.org.uk/
2.6 Serial core
In article <[email protected]> (at Thu, 27 Nov 2003 20:00:41 +0000), Russell King <[email protected]> says:
> The thing that worries me is that an incorrect strlcpy() conversion
> gives the impression that someone has thought about buffer underruns
> as well as overruns, and, unless someone /has/ actually thought about
> it, there could well still be a security problem lurking there.
Hmm, what do you actually mean by "buffer underruns?"
(If I'm correct) do you suggest that we should zero-out rest of
destination buffer?
if so, we may want to have a function, say strlcpy0(), like this:
size_t strlcpy0(char *dst, const char *src, size_t maxlen)
{
size_t len = strlcpy(dst, src, maxlen);
if (maxlen && len < maxlen - 1)
memset(dst + len + 1, 0, maxlen - len - 1);
return len;
}
--yoshfuji
On Fri, Nov 28, 2003 at 05:47:24AM +0900, YOSHIFUJI Hideaki / ?$B5HF#1QL@ wrote:
> In article <[email protected]> (at Thu, 27 Nov 2003 20:00:41 +0000), Russell King <[email protected]> says:
>
> > The thing that worries me is that an incorrect strlcpy() conversion
> > gives the impression that someone has thought about buffer underruns
> > as well as overruns, and, unless someone /has/ actually thought about
> > it, there could well still be a security problem lurking there.
>
> Hmm, what do you actually mean by "buffer underruns?"
>
> (If I'm correct) do you suggest that we should zero-out rest of
> destination buffer?
>
> if so, we may want to have a function, say strlcpy0(), like this:
>
> size_t strlcpy0(char *dst, const char *src, size_t maxlen)
> {
> size_t len = strlcpy(dst, src, maxlen);
> if (maxlen && len < maxlen - 1)
> memset(dst + len + 1, 0, maxlen - len - 1);
> return len;
> }
>
size_t strlcpy0(char *dst, const char *src, size_t maxlen)
{
memset(dst, 0, maxlen);
size_t len = strlcpy(dst, src, maxlen);
return len;
}
--
Murray J. Root
On Thu, 2003-11-27 at 21:00, Russell King wrote:
> >
> > I believe that it, to change from strcpy() to strlcpy(), just
> > eliminates possibility of buffer-overrun.
>
> While this is 100% correct, the bit which raised my attention was the
> original message which didn't seem to show that the above had been
> considered.
Well, I can't see the difference between using strcpy() and strlcpy().
Let be:
char destination[MAX];
char * source;
N = strlen(source);
We could use strlcpy(destination, source, MAX) or strcpy(destination,
source).
We have the following scenarios:
- N < MAX. In this case, both strcpy() and strlcpy() should yield the
same results. No buffer overflows. If the source strings does not
already contain uninitialised data, there's no way for strlcpy() to copy
them.
- N >= MAX. In this case, strlcpy() will copy less bytes than strcpy().
To be exact, strlcpy() will copy N-MAX+1 bytes less than strcpy().
Again, no buffer overflows. Also, it's still impossible to copy
uninitialised data since we just stop at \0 or when we fill up the
destination buffer.
So I don't see how using strlcpy() could copy uninitialised data from
kernel space to user space. If we used memcpy() we could end up copying
uninitialised data, but I can't see how using strlcpy() would do that.
In general terms, strlcpy() will copy *at most* the same number of bytes
as strcpy(), but there is no single case when strlcpy() will copy more
bytes than strcpy().
Can someone throw some light on this?
On Thu, Nov 27, 2003 at 11:06:10PM +0100, Felipe Alfaro Solana wrote:
> On Thu, 2003-11-27 at 21:00, Russell King wrote:
> > >
> > > I believe that it, to change from strcpy() to strlcpy(), just
> > > eliminates possibility of buffer-overrun.
> >
> > While this is 100% correct, the bit which raised my attention was the
> > original message which didn't seem to show that the above had been
> > considered.
>
> Well, I can't see the difference between using strcpy() and strlcpy().
You misunderstand me. Consider the difference between:
strcpy(d, s)
strlcpy(d, s, sizeof(d));
strncpy(d, s, sizeof(d));
strncpy zeros the remainder of d if strlen(s) < sizeof(d), but does not
zero terminate the buffer if strlen(s) == sizeof(d). (Note: this is
how strncpy under the Linux kernel is supposed to work, and yes, the
generic strncpy version in lib/string.c is still buggy.)
strlcpy copies up to the smaller of strlen(s)-1 and sizeof(d)-1, and
ensures that the string is null terminated. If strlen(s) < sizeof(d)-1,
bytes in d will not be written.
Note my final sentence there. Consider the following:
char foo[256];
strlcpy(foo, "hello", sizeof(foo);
copy_to_user(uptr, foo, sizeof(foo));
That ends up writing uninitialised kernel data to (unprivileged) user
space. So would strcpy() used in that situation.
strncpy() on the other hand, will zero the rest of the buffer (on x86
at least) but you'll have to manually ensure that there is a terminator
on the end. Or, you use strlcpy but memset the entire space you're
copying the string into beforehand, which could be wasteful.
Note: we should really fix the generic strncpy() - there are places in
the kernel source which rely on the x86 strncpy() behaviour today (eg,
binfmt_*.c core file generation.)
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 PCMCIA - http://pcmcia.arm.linux.org.uk/
2.6 Serial core
On Thu, Nov 27, 2003 at 10:19:28PM +0000, Russell King wrote:
> Note: we should really fix the generic strncpy() - there are places in
> the kernel source which rely on the x86 strncpy() behaviour today (eg,
> binfmt_*.c core file generation.)
Sorry, bad example. Hmm, from a glance around, it seems that all of
the places which use strncpy() implicitly zero the buffer prior to
using strncpy().
This means that the x86 strncpy is doing unnecessary zeroing. I do
remember Alan complaining about the last set of strlcpy() stuff
introducing information leaks - maybe those got fixed though.
Ok, I don't know where the kernel stands on this issue anymore.
Can someone definitively provide a statement of exactly what the
kernel expects of strncpy() ?
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 PCMCIA - http://pcmcia.arm.linux.org.uk/
2.6 Serial core
On Thu, 2003-11-27 at 23:19, Russell King wrote:
> You misunderstand me. Consider the difference between:
OK, it's perfectly clear now :-)
> Note my final sentence there. Consider the following:
>
> char foo[256];
>
> strlcpy(foo, "hello", sizeof(foo);
>
> copy_to_user(uptr, foo, sizeof(foo));
>
> That ends up writing uninitialised kernel data to (unprivileged) user
> space. So would strcpy() used in that situation.
>
> strncpy() on the other hand, will zero the rest of the buffer (on x86
> at least) but you'll have to manually ensure that there is a terminator
> on the end. Or, you use strlcpy but memset the entire space you're
> copying the string into beforehand, which could be wasteful.
>
> Note: we should really fix the generic strncpy() - there are places in
> the kernel source which rely on the x86 strncpy() behaviour today (eg,
> binfmt_*.c core file generation.)
So, as I see:
1. We should fix strncpy()
2. I should replace strlcpy() with strncpy() in my patches.
In article <[email protected]> (at Fri, 28 Nov 2003 09:23:26 +0900 (JST)), YOSHIFUJI Hideaki / $B5HF#1QL@(B <[email protected]> says:
> 2) memset(dst, 0, len);
> strncpy(dst, src, len);
oops, this should be
memset(dst, 0, len);
if (len > 0)
strncpy(dst, src, len - 1);
> 3) if (len)
> strncpy(dst, src, len - 1);
> dst[len] = 0;
>
> (or, say, strncpy0()).
Note: in this case, we need to fix strncpy() first
to zero-out rest of destination buffer.
--yoshfuji
In article <[email protected]> (at Fri, 28 Nov 2003 00:03:29 +0100), Felipe Alfaro Solana <[email protected]> says:
> So, as I see:
>
> 1. We should fix strncpy()
> 2. I should replace strlcpy() with strncpy() in my patches.
I think it is NOT correct.
It SEEMS unsafe to use strncpy() even if it terminated
string correctly.
So, I'd suggest to replace
strlcpy(dst, src, len);
with
1) strlcpy0(dst, src, len);
where strlcpy0() is provided in my previous mail,
or with
2) memset(dst, 0, len);
strncpy(dst, src, len);
(or say, strncpy0())
or, with
3) if (len)
strncpy(dst, src, len - 1);
dst[len] = 0;
(or, say, strncpy0()).
--yoshfuji
In article <[email protected]> (at Fri, 28 Nov 2003 09:26:42 +0900 (JST)), YOSHIFUJI Hideaki / $B5HF#1QL@(B <[email protected]> says:
> > 3) if (len)
> > strncpy(dst, src, len - 1);
> > dst[len] = 0;
grr, another mistake...:
if (len) {
strncpy(dst, src, len - 1);
dst[len - 1];
}
----------------
1) use strlcpy0(dst, src, len)
size_t strlcpy0(char *dst, const char *src, size_t maxlen)
{
size_t len = strlcpy(dst, src, maxlen);
if (likely(maxlen != 0) && len < maxlen - 1)
memset(dst + len + 1, 0, maxlen - len - 1);
}
2a) use strncpy0(dst, src, len)
char *strncpy0(char *dst, const char *src, size_t maxlen)
{
memset(dst, 0, maxlen);
if (likely(maxlen != 0))
strncpy(dst, src, maxlen - 1);
}
2b) fix strncpy() to zero-out rest of destination buffer
and use strncpy0(dst, src, len)
char *strncpy0(char *dst, const char *src, size_t maxlen)
{
if (likely(maxlen != 0)) {
strncpy(dst, src, maxlen - 1);
dst[maxlen - 1] = 0;
}
}
I prefer 1 > 2b >> 2a.
--yoshfuji
Russell King wrote:
> Sorry, bad example. Hmm, from a glance around, it seems that all of
> the places which use strncpy() implicitly zero the buffer prior to
> using strncpy().
>
> This means that the x86 strncpy is doing unnecessary zeroing. I do
> remember Alan complaining about the last set of strlcpy() stuff
> introducing information leaks - maybe those got fixed though.
The problem is that most places you're filling in an array in a struct.
So even if you use strncpy() everywhere you can still get bitten if the
compiler inserts any padding for alignment on some architecture (since
even if you fully initialize each char[] array in the structure using
strncpy you might still leak info in padding bytes)
The safest thing to do in these cases is:
1. memset() the array before you start
2. strlcpy() for filling each char[] array (since strncpy would just
re-zero those bytes it's wasteful)
Yes, the full memset() is a small waste, but its safe. In 99% of these
cases we're talking about some weird ioctl() or something that's way off
the fast path anyways.
I pointed this out some months ago and someone (forgot who) replied that
there shouldn't be any padding in any struct exported from the kernel.
They added a compiler warning for structure padding in the -mm series for
a few days, but I guess it caused so many warnings that they took it right
out again, so I believe that there ARE plenty of places that user-visible
struct's get padded by the ABI of some platforms. If there's some difinitive
evidence that padding never happens I'd like to see it.
-Mitch
On Fri, 28 November 2003 09:40:22 +0900, YOSHIFUJI Hideaki / ?$B5HF#1QL@ wrote:
> In article <[email protected]> (at Fri, 28 Nov 2003 09:26:42 +0900 (JST)), YOSHIFUJI Hideaki / ?$B5HF#1QL@ <[email protected]> says:
>
> > > 3) if (len)
> > > strncpy(dst, src, len - 1);
> > > dst[len] = 0;
>
> grr, another mistake...:
>
> if (len) {
> strncpy(dst, src, len - 1);
> dst[len - 1];
^
> }
Did you forget ' = 0' there? ;)
J?rn
--
ticks = jiffies;
while (ticks == jiffies);
ticks = jiffies;
-- /usr/src/linux/init/main.c
Russell King wrote:
>
> That ends up writing uninitialised kernel data to (unprivileged) user
> space. So would strcpy() used in that situation.
>
I used to use:
char buf[MAX];
...
buf[MAX-1] = 0; /* zero terminate */
strncpy(buf, src, MAX-1);
This is safe regarding both 0 termination and 0 padding rest of string.
--
Ihar 'Philips' Filipau / with best regards from Saarbruecken.
-- _ _ _
Because the kernel depends on it existing. "init" |_|*|_|
literally _is_ special from a kernel standpoint, |_|_|*|
because its' the "reaper of zombies" (and, may I add, |*|*|*|
that would be a great name for a rock band).
-- Linus Torvalds