Re: revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy"
On Wed, Apr 29, 2015 at 2:35 AM, Al Viro wrote:
> strncpy() has another use, though, and it can't be replaced by strlcpy() -
> see the commits that had started this thread. IMO they (and anything
> else of the same nature) really need to be reverted; using strlcpy() on
> something that isn't guaranteed to be NUL-terminated is a serious bug.
To be honest, I never considered the "source is not NUL-terminated" case.
Strings in C (if they are strings, and not "buffers"; with buffers you
should know how many bytes are valid anyway) are always terminated ;-)
The other case we sometimes do want strncpy() for is the "crazy "fill with
NUL" at the end" feature, to avoid leaking sensitive data. The alternative
is to clear the target first, or the remainder afterwards.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: revert fs/befs/linuxvfs.c: replace strncpy by strlcpy
On Wed, Apr 29, 2015 at 2:35 AM, Al Viro v...@zeniv.linux.org.uk wrote:
strncpy() has another use, though, and it can't be replaced by strlcpy() -
see the commits that had started this thread. IMO they (and anything
else of the same nature) really need to be reverted; using strlcpy() on
something that isn't guaranteed to be NUL-terminated is a serious bug.
To be honest, I never considered the source is not NUL-terminated case.
Strings in C (if they are strings, and not buffers; with buffers you
should know how many bytes are valid anyway) are always terminated ;-)
The other case we sometimes do want strncpy() for is the crazy fill with
NUL at the end feature, to avoid leaking sensitive data. The alternative
is to clear the target first, or the remainder afterwards.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy"
On Tue, Apr 28, 2015 at 02:48:45PM -0700, Linus Torvalds wrote: > I suspect you could take that lib/strncpy_from_user.c and massage it > reasonably trivially to be a good function. > > That said, I can't think of a single strncpy (or strlcpy) in kernel > space that is actually worth even optimizing for. They just don't tend > to be in the critical path. So correctness is likely *much* more > important than worrying about performance. Indeed. As it is, I suspect that strlcpy() use should be uniformly discouraged; if nothing else, snprintf() gives the same semantics, is less likely to cause confusion regardling the expected return value and none of those paths are performance-critical. strncpy() has another use, though, and it can't be replaced by strlcpy() - see the commits that had started this thread. IMO they (and anything else of the same nature) really need to be reverted; using strlcpy() on something that isn't guaranteed to be NUL-terminated is a serious bug. And catching all such places is going to be quite a work - there are too many strlcpy() callers out there. Frankly, looking through call sites in fs... * two callers in fs/9p - strlcpy() + sscanf(), both should've been plain sscanf() (and the second should've been "HARDLINKCOUNT%u" instead of "%13s %u" + comparison of string with "HARDLINKCOUNT" - sscanf() is quite capable of matching explicit string literals) * affs one: match_strdup + strlcpy + kfree. Should just use match_strlcpy instead (BTW, despite the name, it does *not* use strclpy() internally). * afs: might be correct. * two in befs: both broken. * binfmt_misc: fishy; load_misc_binary() finds an object under rwlock, copies one of its fields (->interpreter), drops rwlock and proceeds to do various blocking operations (including open_exec()). Using another field of the same object (->interp_flags) all along. If it gets freed and reused, well... let's hope we won't fuck up too badly. IMO we'd be better off if we added a refcount to that sucker and used it to control the freeing. * btrfs: undefined behaviour - potentially overlapping source and destination. * another btrfs one: char b[BDEVNAME_SIZE]; strlcpy(s->s_id, bdevname(bdev, b), sizeof(s->s_id)); complete garbage; might as well do bdevname(bdev, s->s_id) and be done with that. ... and so on; this stuff is misused more often than not. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy"
On Tue, Apr 28, 2015 at 2:38 PM, Chris Metcalf wrote: > > To do that we'd probably want to provide a generic version that > just copied byte-by-byte, and encourage architecture variants > that were more efficient. Our generic "strncpy_from_user()" is actually fairly efficient, and reasonably portable. It almost has to be, since this is actually a much more common - and much more critical - load than any regular strncpy I know of in the kernel. I suspect you could take that lib/strncpy_from_user.c and massage it reasonably trivially to be a good function. That said, I can't think of a single strncpy (or strlcpy) in kernel space that is actually worth even optimizing for. They just don't tend to be in the critical path. So correctness is likely *much* more important than worrying about performance. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy"
On 04/28/2015 04:51 PM, Linus Torvalds wrote:
On Tue, Apr 28, 2015 at 12:48 PM, Chris Metcalf wrote:
FWIW, I wanted to deal with some strncpy/strlcpy API issues last year
and just put a "strscpy()" function in arch/tile/gxio/mpipe.c,
So quite frankly, I don't like that one either.
Some people really *do* want truncation, and your strscpy() makes that
impossible.
Perhaps the answer is to provide a strscpy() and a separate
strscpy_truncate() that explicitly enables truncation semantics.
I remain skeptical of providing a handy function with an error
return that people are free to ignore. If one wants truncating
semantics, one should be obliged to say so.
Also, your strscpy() implementation is actually not thread-safe: it
can return an non-terminated string if the source string isn't stable.
That can certainly be a design issue ("don't do that then"), but it
*can* be a possible source of security issues, so it's a bad idea in
something that is supposed to be secure.
To do that we'd probably want to provide a generic version that
just copied byte-by-byte, and encourage architecture variants
that were more efficient. This would leave it less efficient in
general than strncpy/strlcpy (the former typically has efficient
arch versions already, and the latter, although not thread-safe
by your definition, is built on strlen/memcpy, which typically
have efficient arch versions).
I don't see a way to leverage existing efficient implementations,
since only strncpy likely has pre-existing efficient implementations,
and then we'd have to call strlen() on the destination just to
return the total length (after already calling strnlen() on the
source to figure out how long we thought it was).
I suppose if we want to just return a boolean saying "it fit" or
"it didn't fit" we could leverage strncpy, though if the buffer
changed out from under us to be just a single NUL instead of
a long string, we'd still do the silly NUL-fill behavior. So maybe
it's not worth the contortions.
And quite frankly, I think that the *only* valid reason to add another
random string copy function is that you actually get it right. We
don't need yet another half-arsed routine that can be easily misused.
We have too many of those.
For sure. Something like this untested code in lib/string.c, to be
concrete?
#ifndef __HAVE_ARCH_STRSCPY
/**
* strscpy_truncate - Copy a C-string into a sized buffer and return whether it
fit
* @dest: Where to copy the string to
* @src: Where to copy the string from
* @count: Size of destination buffer
*
* Copy the string, or as much of it as fits, into the dest buffer.
* The routine returns the total number of bytes copied
* (including the trailing NUL) or zero if the buffer wasn't
* big enough. The dest buffer is always NUL-terminated.
*/
static size_t strscpy_truncate(char *dest, const char *src, size_t count)
{
char *tmp = dest;
if (count == 0)
return 0; /* no NUL-termination possible */
while ((*tmp++ = *src++) != '\0') {
if (--count == 0) {
*--tmp = '\0';
return 0;
}
}
return tmp - dest;
}
EXPORT_SYMBOL(strscpy_truncate);
/**
* strscpy - Copy a C-string into a sized buffer, but only if it fits
* @dest: Where to copy the string to
* @src: Where to copy the string from
* @count: Size of destination buffer
*
* Use this routine to avoid copying too-long strings.
* The routine returns the total number of bytes copied
* (including the trailing NUL) or zero if the buffer wasn't
* big enough. To ensure that programmers pay attention
* to the return code, the destination has a single NUL
* written at the front (if count is non-zero) when the
* buffer is not big enough.
*/
static size_t strscpy(char *dest, const char *src, size_t count)
{
size_t res = strscpy_truncate(dest, src, count);
if (res == 0 && count != 0)
dest[0] = '\0';
return res;
}
EXPORT_SYMBOL(strscpy);
#endif
--
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy"
On Tue, Apr 28, 2015 at 12:48 PM, Chris Metcalf wrote:
>
> FWIW, I wanted to deal with some strncpy/strlcpy API issues last year
> and just put a "strscpy()" function in arch/tile/gxio/mpipe.c,
So quite frankly, I don't like that one either.
Some people really *do* want truncation, and your strscpy() makes that
impossible.
Also, your strscpy() implementation is actually not thread-safe: it
can return an non-terminated string if the source string isn't stable.
That can certainly be a design issue ("don't do that then"), but it
*can* be a possible source of security issues, so it's a bad idea in
something that is supposed to be secure.
And quite frankly, I think that the *only* valid reason to add another
random string copy function is that you actually get it right. We
don't need yet another half-arsed routine that can be easily misused.
We have too many of those.
Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy"
> On 28 April 2015 at 19:39 Al Viro wrote:
>
>
> On Tue, Apr 28, 2015 at 06:42:10PM +0200, Fabian Frederick wrote:
> >
> >
> > > On 28 April 2015 at 18:05 Al Viro wrote:
> > >
> > >
> > > On Tue, Apr 28, 2015 at 07:35:10AM +0200, Fabian Frederick wrote:
> > >
> > > > > Al, very unhappy about the prospect of looking through ~2000 calls of
> > > > > strlcpy()
> > > > > we have in the tree...
> > > >
> > > > Sorry Al, I thought it was more secure.
> > >
> > > It's not just you, unfortunately, and dumping all that annoyance on you
> > > as a proxy for everyone who does that kind of thing had been unfair.
> > > My apologies...
> >
> > No problem Al :) but why can't we harden strlcpy at first with
> > something like a strlen limited to max char.
> > (I don't know if it's already in kernel libs).
> >
> > size_t strlenl(const char *s, size_t maxlen)
>
> aka strnlen()
>
> > const char *sc = s;
> > size_t i = 0;
> >
> > while (*sc != '\0' && (i < maxlen)) {
> > i++;
> > sc++;
> > }
> > return sc - s;
> > }
> >
> > Then we could solve problems downstream ...
>
> Can't. Seriously, look what strlcpy() is supposed to return; it's pretty
> much a microoptimized snprintf(dst, size, "%s", src). It's certainly
> been patterned after snprintf(3) - "don't exceed that size, NUL-terminate
> unless the size is zero, return the number of characters (excluding NUL)
> that would've been written if the size had been large enough".
>
> The following is a legitimate use of strlcpy():
>
> int foo(char *); /* modifies string */
>
> int const_foo(const char *s)
> {
> int res;
> char buf[32], *p = buf;
> size_t wanted = strlcpy(buf, sizeof(buf), s);
> if (wanted >= sizeof(buf)) {
> p = malloc(wanted + 1);
> if (!p)
> return -ENOMEM;
> memcpy(p, s, wanted + 1);
> }
> res = foo(p);
> if (p != buf)
> free(p);
> return res;
> }
>
> None of the kernel callers are of exactly that form (and most ignore the
> return value completely), but if we make that sucker return something
> different from what strlcpy(3) would return, we'd damn better _not_ keep
> the name; there's enough confusion in that area as it is.
Of course with another function name. There's no other way to do it ...
strlncpy/strlncat ? :)
Regards,
Fabian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy"
On 04/28/2015 12:42 PM, Linus Torvalds wrote: On Tue, Apr 28, 2015 at 9:05 AM, Al Viro wrote: Unfortunately, we _can't_ make strlcpy() never look past src + size - 1 - not without changing its semantics. Yeah, strlcpy is actually nasty. I don't understand why people like it so much. It's a crap interface, and completely unsuitable for untrusted sources because of the overrun issue. FWIW, I wanted to deal with some strncpy/strlcpy API issues last year and just put a "strscpy()" function in arch/tile/gxio/mpipe.c, with a comment saying it might be worth making it global at a later date, but at the time only the reviewers seemed interested in making it happen, so I let that possibility die on the vine. I argue that truncated strings are often pretty nasty, so you don't want to just silently say "Sure, I made it fit!" but instead make that be a failure case. In principle you could keep the return code of my proposed strscpy() and still do the truncated-string copy, but I think it's a mistake in almost every case, and if you really want that semantics, you should be forced to use something harder (e.g. some combination of explicit strlen and memcpy) so it's clear you know what you're doing. commit bceb7efa6a7e656bfaa67b6f54925e7db75bcd52 Author: Chris Metcalf Date: Tue Sep 2 16:25:22 2014 -0400 tile gxio: use better string copy primitive Both strncpy and strlcpy suffer from the fact that they do partial copies of strings into the destination when the target buffer is too small. This is frequently pointless since an overflow of the target buffer may make the result invalid. strncpy() makes it relatively hard to even detect the error condition, and with strlcpy() you have to duplicate the buffer size parameter to test to see if the result exceeds it. By returning zero in the failure case, we both make testing for it easy, and by simply not copying anything in that case, we make it mandatory for callers to test the error code. To catch lazy programmers who don't check, we also place a NUL at the start of the destination buffer (if there is space) to ensure that the result is an invalid string. At some point it may make sense to promote strscpy() to a global platform-independent function, but other than the reviewers, no one was interested on LKML, so for now leave the strscpy() function as file-static. Reviewed-by: Randy Dunlap Reviewed-by: Rickard Strandqvist Signed-off-by: Chris Metcalf -- Chris Metcalf, EZChip Semiconductor http://www.ezchip.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy"
On Tue, Apr 28, 2015 at 06:42:10PM +0200, Fabian Frederick wrote:
>
>
> > On 28 April 2015 at 18:05 Al Viro wrote:
> >
> >
> > On Tue, Apr 28, 2015 at 07:35:10AM +0200, Fabian Frederick wrote:
> >
> > > > Al, very unhappy about the prospect of looking through ~2000 calls of
> > > > strlcpy()
> > > > we have in the tree...
> > >
> > > Sorry Al, I thought it was more secure.
> >
> > It's not just you, unfortunately, and dumping all that annoyance on you
> > as a proxy for everyone who does that kind of thing had been unfair.
> > My apologies...
>
> No problem Al :) but why can't we harden strlcpy at first with
> something like a strlen limited to max char.
> (I don't know if it's already in kernel libs).
>
> size_t strlenl(const char *s, size_t maxlen)
aka strnlen()
> const char *sc = s;
> size_t i = 0;
>
> while (*sc != '\0' && (i < maxlen)) {
> i++;
> sc++;
> }
> return sc - s;
> }
>
> Then we could solve problems downstream ...
Can't. Seriously, look what strlcpy() is supposed to return; it's pretty
much a microoptimized snprintf(dst, size, "%s", src). It's certainly
been patterned after snprintf(3) - "don't exceed that size, NUL-terminate
unless the size is zero, return the number of characters (excluding NUL)
that would've been written if the size had been large enough".
The following is a legitimate use of strlcpy():
int foo(char *);/* modifies string */
int const_foo(const char *s)
{
int res;
char buf[32], *p = buf;
size_t wanted = strlcpy(buf, sizeof(buf), s);
if (wanted >= sizeof(buf)) {
p = malloc(wanted + 1);
if (!p)
return -ENOMEM;
memcpy(p, s, wanted + 1);
}
res = foo(p);
if (p != buf)
free(p);
return res;
}
None of the kernel callers are of exactly that form (and most ignore the
return value completely), but if we make that sucker return something
different from what strlcpy(3) would return, we'd damn better _not_ keep
the name; there's enough confusion in that area as it is.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy"
On Tue, Apr 28, 2015 at 9:05 AM, Al Viro wrote: > > Unfortunately, we _can't_ make strlcpy() never look past src + size - 1 - > not without changing its semantics. Yeah, strlcpy is actually nasty. I don't understand why people like it so much. It's a crap interface, and completely unsuitable for untrusted sources because of the overrun issue. Now, strncpy is nasty too, because of the two main flaws: - no guaranteed NUL at the end - crazy "fill with NUL" at the end the first of which causes security issues, and the second of which causes performance issues when you have small strings and size your buffers generously. Generally, what we want is "strncpy()" that forces a _single_ NUL at the end. Basically, what we want is that good_strncpy(dst, src, n); assert(strlen(dst) < n); always just works, but it doesn't try to pad the 'dst' to zero. Alternatively, the return value should be really obvious and unambiguous. That's what our "strncpy_from_user()" does: it is a but more complex because it needs to show three cases (ok, too long, and EFAULT), so the semantics for 'strncpy_from_user() is that you have to just always check the return value, but at least it's simple: - negative means error - >= n means "too long" - 0..n-1 means "ok" and is the size of the string. for a normal in-kernel strncpy(), we'd likely be better off just returing "we truncated" as an error. Then you could just do mystrncpy(dst, src, sizeof(dst)); and the result would be a valid string (possibly truncated), and if you care about truncation you just do if (good_strncpy(dst, src, sizeof(dst)) < 0) return -ETOOLONG; both of which are just very *obvious* things, and neither of which leans a possibly unsafe string in "dst", nor look past the end of 'src'. Oh well. I can certainly imagine other more complex forms too (max length of destination _and_ separately max length of source). But strncpy and strlcpy are both horrible nasty error-prone crap. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy"
> On 28 April 2015 at 18:05 Al Viro wrote:
>
>
> On Tue, Apr 28, 2015 at 07:35:10AM +0200, Fabian Frederick wrote:
>
> > > Al, very unhappy about the prospect of looking through ~2000 calls of
> > > strlcpy()
> > > we have in the tree...
> >
> > Sorry Al, I thought it was more secure.
>
> It's not just you, unfortunately, and dumping all that annoyance on you
> as a proxy for everyone who does that kind of thing had been unfair.
> My apologies...
No problem Al :) but why can't we harden strlcpy at first with
something like a strlen limited to max char.
(I don't know if it's already in kernel libs).
size_t strlenl(const char *s, size_t maxlen)
{
const char *sc = s;
size_t i = 0;
while (*sc != '\0' && (i < maxlen)) {
i++;
sc++;
}
return sc - s;
}
Then we could solve problems downstream ...
Regards,
Fabian
>
> > I guess the 2 following patches should be reversed as well :
> >
> > 6cb103b6f45a
> > "fs/befs/btree.c: replace strncpy by strlcpy + coding style fixing"
> >
> > 69201bb11327
> > "fs/ocfs2/super.c: use OCFS2_MAX_VOL_LABEL_LEN and strlcpy"
>
> AFAICS, they should.
>
> Unfortunately, we _can't_ make strlcpy() never look past src + size - 1 -
> not without changing its semantics. Its callers expect it to return
> the length of source; one of the intended uses is
> wanted = strlcpy(dst, src, dst_size);
> if (wanted >= dst_size) {
> p = realloc(dst, wanted + 1);
> if (!p) {
> // too bad
> } else {
> dst = p;
> memcpy(dst, src, wanted + 1);
> }
> }
> and that really wants the accurate length. Now, the absolute majority of
> strlcpy() users in the kernel completely ignore the return value, i.e. go for
> silent truncation. About 1% do not.
>
> Out of those, some are correctly implemented "fail if truncated" uses.
> The rest... Some are weirdly open-coded snprintf() (series of strlcpy and
> strlcat) and some are _very_ dubious. Either they really never get
> truncated, or we have a problem. For example, this
> kernel/module.c:2349: s += strlcpy(s,
> >strtab[src[i].st_name],
> smells really bad. We are truncating a bunch of strings dowsn to
> KSYM_NAME_LEN
> there and storing them in a buffer, with spacing that matches _untruncated_
> strings.
>
> drivers/s390/scsi/zfcp_fc.c:825: len = strlcpy(rspn_req->rspn.fr_name,
> fc_host_symbolic_name(shost),
> also looks fishy - what happens there is
> len = strlcpy(rspn_req->rspn.fr_name, fc_host_symbolic_name(shost),
> FC_SYMBOLIC_NAME_SIZE);
> rspn_req->rspn.fr_name_len = len;
>
> drivers/usb/gadget/function/f_midi.c:976: result = strlcpy(page, opts->id,
> PAGE_SIZE);
> drivers/usb/gadget/function/f_printer.c:1172: result = strlcpy(page,
> opts->pnp_string + 2, PNP_STRING_LEN - 2);
> drivers/usb/gadget/function/f_printer.c:1184: result =
> strlcpy(opts->pnp_string + 2, page, PNP_STRING_LEN - 2);
>
> are also strange...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy"
On Tue, Apr 28, 2015 at 07:35:10AM +0200, Fabian Frederick wrote:
> > Al, very unhappy about the prospect of looking through ~2000 calls of
> > strlcpy()
> > we have in the tree...
>
> Sorry Al, I thought it was more secure.
It's not just you, unfortunately, and dumping all that annoyance on you
as a proxy for everyone who does that kind of thing had been unfair.
My apologies...
> I guess the 2 following patches should be reversed as well :
>
> 6cb103b6f45a
> "fs/befs/btree.c: replace strncpy by strlcpy + coding style fixing"
>
> 69201bb11327
> "fs/ocfs2/super.c: use OCFS2_MAX_VOL_LABEL_LEN and strlcpy"
AFAICS, they should.
Unfortunately, we _can't_ make strlcpy() never look past src + size - 1 -
not without changing its semantics. Its callers expect it to return
the length of source; one of the intended uses is
wanted = strlcpy(dst, src, dst_size);
if (wanted >= dst_size) {
p = realloc(dst, wanted + 1);
if (!p) {
// too bad
} else {
dst = p;
memcpy(dst, src, wanted + 1);
}
}
and that really wants the accurate length. Now, the absolute majority of
strlcpy() users in the kernel completely ignore the return value, i.e. go for
silent truncation. About 1% do not.
Out of those, some are correctly implemented "fail if truncated" uses.
The rest... Some are weirdly open-coded snprintf() (series of strlcpy and
strlcat) and some are _very_ dubious. Either they really never get
truncated, or we have a problem. For example, this
kernel/module.c:2349: s += strlcpy(s,
>strtab[src[i].st_name],
smells really bad. We are truncating a bunch of strings dowsn to KSYM_NAME_LEN
there and storing them in a buffer, with spacing that matches _untruncated_
strings.
drivers/s390/scsi/zfcp_fc.c:825:len = strlcpy(rspn_req->rspn.fr_name,
fc_host_symbolic_name(shost),
also looks fishy - what happens there is
len = strlcpy(rspn_req->rspn.fr_name, fc_host_symbolic_name(shost),
FC_SYMBOLIC_NAME_SIZE);
rspn_req->rspn.fr_name_len = len;
drivers/usb/gadget/function/f_midi.c:976: result = strlcpy(page,
opts->id, PAGE_SIZE);
drivers/usb/gadget/function/f_printer.c:1172: result = strlcpy(page,
opts->pnp_string + 2, PNP_STRING_LEN - 2);
drivers/usb/gadget/function/f_printer.c:1184: result =
strlcpy(opts->pnp_string + 2, page, PNP_STRING_LEN - 2);
are also strange...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: revert fs/befs/linuxvfs.c: replace strncpy by strlcpy
On Tue, Apr 28, 2015 at 02:48:45PM -0700, Linus Torvalds wrote: I suspect you could take that lib/strncpy_from_user.c and massage it reasonably trivially to be a good function. That said, I can't think of a single strncpy (or strlcpy) in kernel space that is actually worth even optimizing for. They just don't tend to be in the critical path. So correctness is likely *much* more important than worrying about performance. Indeed. As it is, I suspect that strlcpy() use should be uniformly discouraged; if nothing else, snprintf() gives the same semantics, is less likely to cause confusion regardling the expected return value and none of those paths are performance-critical. strncpy() has another use, though, and it can't be replaced by strlcpy() - see the commits that had started this thread. IMO they (and anything else of the same nature) really need to be reverted; using strlcpy() on something that isn't guaranteed to be NUL-terminated is a serious bug. And catching all such places is going to be quite a work - there are too many strlcpy() callers out there. Frankly, looking through call sites in fs... * two callers in fs/9p - strlcpy() + sscanf(), both should've been plain sscanf() (and the second should've been HARDLINKCOUNT%u instead of %13s %u + comparison of string with HARDLINKCOUNT - sscanf() is quite capable of matching explicit string literals) * affs one: match_strdup + strlcpy + kfree. Should just use match_strlcpy instead (BTW, despite the name, it does *not* use strclpy() internally). * afs: might be correct. * two in befs: both broken. * binfmt_misc: fishy; load_misc_binary() finds an object under rwlock, copies one of its fields (-interpreter), drops rwlock and proceeds to do various blocking operations (including open_exec()). Using another field of the same object (-interp_flags) all along. If it gets freed and reused, well... let's hope we won't fuck up too badly. IMO we'd be better off if we added a refcount to that sucker and used it to control the freeing. * btrfs: undefined behaviour - potentially overlapping source and destination. * another btrfs one: char b[BDEVNAME_SIZE]; strlcpy(s-s_id, bdevname(bdev, b), sizeof(s-s_id)); complete garbage; might as well do bdevname(bdev, s-s_id) and be done with that. ... and so on; this stuff is misused more often than not. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: revert fs/befs/linuxvfs.c: replace strncpy by strlcpy
On Tue, Apr 28, 2015 at 07:35:10AM +0200, Fabian Frederick wrote:
Al, very unhappy about the prospect of looking through ~2000 calls of
strlcpy()
we have in the tree...
Sorry Al, I thought it was more secure.
It's not just you, unfortunately, and dumping all that annoyance on you
as a proxy for everyone who does that kind of thing had been unfair.
My apologies...
I guess the 2 following patches should be reversed as well :
6cb103b6f45a
fs/befs/btree.c: replace strncpy by strlcpy + coding style fixing
69201bb11327
fs/ocfs2/super.c: use OCFS2_MAX_VOL_LABEL_LEN and strlcpy
AFAICS, they should.
Unfortunately, we _can't_ make strlcpy() never look past src + size - 1 -
not without changing its semantics. Its callers expect it to return
the length of source; one of the intended uses is
wanted = strlcpy(dst, src, dst_size);
if (wanted = dst_size) {
p = realloc(dst, wanted + 1);
if (!p) {
// too bad
} else {
dst = p;
memcpy(dst, src, wanted + 1);
}
}
and that really wants the accurate length. Now, the absolute majority of
strlcpy() users in the kernel completely ignore the return value, i.e. go for
silent truncation. About 1% do not.
Out of those, some are correctly implemented fail if truncated uses.
The rest... Some are weirdly open-coded snprintf() (series of strlcpy and
strlcat) and some are _very_ dubious. Either they really never get
truncated, or we have a problem. For example, this
kernel/module.c:2349: s += strlcpy(s,
mod-strtab[src[i].st_name],
smells really bad. We are truncating a bunch of strings dowsn to KSYM_NAME_LEN
there and storing them in a buffer, with spacing that matches _untruncated_
strings.
drivers/s390/scsi/zfcp_fc.c:825:len = strlcpy(rspn_req-rspn.fr_name,
fc_host_symbolic_name(shost),
also looks fishy - what happens there is
len = strlcpy(rspn_req-rspn.fr_name, fc_host_symbolic_name(shost),
FC_SYMBOLIC_NAME_SIZE);
rspn_req-rspn.fr_name_len = len;
drivers/usb/gadget/function/f_midi.c:976: result = strlcpy(page,
opts-id, PAGE_SIZE);
drivers/usb/gadget/function/f_printer.c:1172: result = strlcpy(page,
opts-pnp_string + 2, PNP_STRING_LEN - 2);
drivers/usb/gadget/function/f_printer.c:1184: result =
strlcpy(opts-pnp_string + 2, page, PNP_STRING_LEN - 2);
are also strange...
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: revert fs/befs/linuxvfs.c: replace strncpy by strlcpy
On 28 April 2015 at 18:05 Al Viro v...@zeniv.linux.org.uk wrote:
On Tue, Apr 28, 2015 at 07:35:10AM +0200, Fabian Frederick wrote:
Al, very unhappy about the prospect of looking through ~2000 calls of
strlcpy()
we have in the tree...
Sorry Al, I thought it was more secure.
It's not just you, unfortunately, and dumping all that annoyance on you
as a proxy for everyone who does that kind of thing had been unfair.
My apologies...
No problem Al :) but why can't we harden strlcpy at first with
something like a strlen limited to max char.
(I don't know if it's already in kernel libs).
size_t strlenl(const char *s, size_t maxlen)
{
const char *sc = s;
size_t i = 0;
while (*sc != '\0' (i maxlen)) {
i++;
sc++;
}
return sc - s;
}
Then we could solve problems downstream ...
Regards,
Fabian
I guess the 2 following patches should be reversed as well :
6cb103b6f45a
fs/befs/btree.c: replace strncpy by strlcpy + coding style fixing
69201bb11327
fs/ocfs2/super.c: use OCFS2_MAX_VOL_LABEL_LEN and strlcpy
AFAICS, they should.
Unfortunately, we _can't_ make strlcpy() never look past src + size - 1 -
not without changing its semantics. Its callers expect it to return
the length of source; one of the intended uses is
wanted = strlcpy(dst, src, dst_size);
if (wanted = dst_size) {
p = realloc(dst, wanted + 1);
if (!p) {
// too bad
} else {
dst = p;
memcpy(dst, src, wanted + 1);
}
}
and that really wants the accurate length. Now, the absolute majority of
strlcpy() users in the kernel completely ignore the return value, i.e. go for
silent truncation. About 1% do not.
Out of those, some are correctly implemented fail if truncated uses.
The rest... Some are weirdly open-coded snprintf() (series of strlcpy and
strlcat) and some are _very_ dubious. Either they really never get
truncated, or we have a problem. For example, this
kernel/module.c:2349: s += strlcpy(s,
mod-strtab[src[i].st_name],
smells really bad. We are truncating a bunch of strings dowsn to
KSYM_NAME_LEN
there and storing them in a buffer, with spacing that matches _untruncated_
strings.
drivers/s390/scsi/zfcp_fc.c:825: len = strlcpy(rspn_req-rspn.fr_name,
fc_host_symbolic_name(shost),
also looks fishy - what happens there is
len = strlcpy(rspn_req-rspn.fr_name, fc_host_symbolic_name(shost),
FC_SYMBOLIC_NAME_SIZE);
rspn_req-rspn.fr_name_len = len;
drivers/usb/gadget/function/f_midi.c:976: result = strlcpy(page, opts-id,
PAGE_SIZE);
drivers/usb/gadget/function/f_printer.c:1172: result = strlcpy(page,
opts-pnp_string + 2, PNP_STRING_LEN - 2);
drivers/usb/gadget/function/f_printer.c:1184: result =
strlcpy(opts-pnp_string + 2, page, PNP_STRING_LEN - 2);
are also strange...
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: revert fs/befs/linuxvfs.c: replace strncpy by strlcpy
On Tue, Apr 28, 2015 at 9:05 AM, Al Viro v...@zeniv.linux.org.uk wrote: Unfortunately, we _can't_ make strlcpy() never look past src + size - 1 - not without changing its semantics. Yeah, strlcpy is actually nasty. I don't understand why people like it so much. It's a crap interface, and completely unsuitable for untrusted sources because of the overrun issue. Now, strncpy is nasty too, because of the two main flaws: - no guaranteed NUL at the end - crazy fill with NUL at the end the first of which causes security issues, and the second of which causes performance issues when you have small strings and size your buffers generously. Generally, what we want is strncpy() that forces a _single_ NUL at the end. Basically, what we want is that good_strncpy(dst, src, n); assert(strlen(dst) n); always just works, but it doesn't try to pad the 'dst' to zero. Alternatively, the return value should be really obvious and unambiguous. That's what our strncpy_from_user() does: it is a but more complex because it needs to show three cases (ok, too long, and EFAULT), so the semantics for 'strncpy_from_user() is that you have to just always check the return value, but at least it's simple: - negative means error - = n means too long - 0..n-1 means ok and is the size of the string. for a normal in-kernel strncpy(), we'd likely be better off just returing we truncated as an error. Then you could just do mystrncpy(dst, src, sizeof(dst)); and the result would be a valid string (possibly truncated), and if you care about truncation you just do if (good_strncpy(dst, src, sizeof(dst)) 0) return -ETOOLONG; both of which are just very *obvious* things, and neither of which leans a possibly unsafe string in dst, nor look past the end of 'src'. Oh well. I can certainly imagine other more complex forms too (max length of destination _and_ separately max length of source). But strncpy and strlcpy are both horrible nasty error-prone crap. Linus -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: revert fs/befs/linuxvfs.c: replace strncpy by strlcpy
On 28 April 2015 at 19:39 Al Viro v...@zeniv.linux.org.uk wrote:
On Tue, Apr 28, 2015 at 06:42:10PM +0200, Fabian Frederick wrote:
On 28 April 2015 at 18:05 Al Viro v...@zeniv.linux.org.uk wrote:
On Tue, Apr 28, 2015 at 07:35:10AM +0200, Fabian Frederick wrote:
Al, very unhappy about the prospect of looking through ~2000 calls of
strlcpy()
we have in the tree...
Sorry Al, I thought it was more secure.
It's not just you, unfortunately, and dumping all that annoyance on you
as a proxy for everyone who does that kind of thing had been unfair.
My apologies...
No problem Al :) but why can't we harden strlcpy at first with
something like a strlen limited to max char.
(I don't know if it's already in kernel libs).
size_t strlenl(const char *s, size_t maxlen)
aka strnlen()
const char *sc = s;
size_t i = 0;
while (*sc != '\0' (i maxlen)) {
i++;
sc++;
}
return sc - s;
}
Then we could solve problems downstream ...
Can't. Seriously, look what strlcpy() is supposed to return; it's pretty
much a microoptimized snprintf(dst, size, %s, src). It's certainly
been patterned after snprintf(3) - don't exceed that size, NUL-terminate
unless the size is zero, return the number of characters (excluding NUL)
that would've been written if the size had been large enough.
The following is a legitimate use of strlcpy():
int foo(char *); /* modifies string */
int const_foo(const char *s)
{
int res;
char buf[32], *p = buf;
size_t wanted = strlcpy(buf, sizeof(buf), s);
if (wanted = sizeof(buf)) {
p = malloc(wanted + 1);
if (!p)
return -ENOMEM;
memcpy(p, s, wanted + 1);
}
res = foo(p);
if (p != buf)
free(p);
return res;
}
None of the kernel callers are of exactly that form (and most ignore the
return value completely), but if we make that sucker return something
different from what strlcpy(3) would return, we'd damn better _not_ keep
the name; there's enough confusion in that area as it is.
Of course with another function name. There's no other way to do it ...
strlncpy/strlncat ? :)
Regards,
Fabian
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: revert fs/befs/linuxvfs.c: replace strncpy by strlcpy
On Tue, Apr 28, 2015 at 2:38 PM, Chris Metcalf cmetc...@ezchip.com wrote: To do that we'd probably want to provide a generic version that just copied byte-by-byte, and encourage architecture variants that were more efficient. Our generic strncpy_from_user() is actually fairly efficient, and reasonably portable. It almost has to be, since this is actually a much more common - and much more critical - load than any regular strncpy I know of in the kernel. I suspect you could take that lib/strncpy_from_user.c and massage it reasonably trivially to be a good function. That said, I can't think of a single strncpy (or strlcpy) in kernel space that is actually worth even optimizing for. They just don't tend to be in the critical path. So correctness is likely *much* more important than worrying about performance. Linus -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: revert fs/befs/linuxvfs.c: replace strncpy by strlcpy
On Tue, Apr 28, 2015 at 12:48 PM, Chris Metcalf cmetc...@ezchip.com wrote: FWIW, I wanted to deal with some strncpy/strlcpy API issues last year and just put a strscpy() function in arch/tile/gxio/mpipe.c, So quite frankly, I don't like that one either. Some people really *do* want truncation, and your strscpy() makes that impossible. Also, your strscpy() implementation is actually not thread-safe: it can return an non-terminated string if the source string isn't stable. That can certainly be a design issue (don't do that then), but it *can* be a possible source of security issues, so it's a bad idea in something that is supposed to be secure. And quite frankly, I think that the *only* valid reason to add another random string copy function is that you actually get it right. We don't need yet another half-arsed routine that can be easily misused. We have too many of those. Linus -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: revert fs/befs/linuxvfs.c: replace strncpy by strlcpy
On 04/28/2015 04:51 PM, Linus Torvalds wrote:
On Tue, Apr 28, 2015 at 12:48 PM, Chris Metcalf cmetc...@ezchip.com wrote:
FWIW, I wanted to deal with some strncpy/strlcpy API issues last year
and just put a strscpy() function in arch/tile/gxio/mpipe.c,
So quite frankly, I don't like that one either.
Some people really *do* want truncation, and your strscpy() makes that
impossible.
Perhaps the answer is to provide a strscpy() and a separate
strscpy_truncate() that explicitly enables truncation semantics.
I remain skeptical of providing a handy function with an error
return that people are free to ignore. If one wants truncating
semantics, one should be obliged to say so.
Also, your strscpy() implementation is actually not thread-safe: it
can return an non-terminated string if the source string isn't stable.
That can certainly be a design issue (don't do that then), but it
*can* be a possible source of security issues, so it's a bad idea in
something that is supposed to be secure.
To do that we'd probably want to provide a generic version that
just copied byte-by-byte, and encourage architecture variants
that were more efficient. This would leave it less efficient in
general than strncpy/strlcpy (the former typically has efficient
arch versions already, and the latter, although not thread-safe
by your definition, is built on strlen/memcpy, which typically
have efficient arch versions).
I don't see a way to leverage existing efficient implementations,
since only strncpy likely has pre-existing efficient implementations,
and then we'd have to call strlen() on the destination just to
return the total length (after already calling strnlen() on the
source to figure out how long we thought it was).
I suppose if we want to just return a boolean saying it fit or
it didn't fit we could leverage strncpy, though if the buffer
changed out from under us to be just a single NUL instead of
a long string, we'd still do the silly NUL-fill behavior. So maybe
it's not worth the contortions.
And quite frankly, I think that the *only* valid reason to add another
random string copy function is that you actually get it right. We
don't need yet another half-arsed routine that can be easily misused.
We have too many of those.
For sure. Something like this untested code in lib/string.c, to be
concrete?
#ifndef __HAVE_ARCH_STRSCPY
/**
* strscpy_truncate - Copy a C-string into a sized buffer and return whether it
fit
* @dest: Where to copy the string to
* @src: Where to copy the string from
* @count: Size of destination buffer
*
* Copy the string, or as much of it as fits, into the dest buffer.
* The routine returns the total number of bytes copied
* (including the trailing NUL) or zero if the buffer wasn't
* big enough. The dest buffer is always NUL-terminated.
*/
static size_t strscpy_truncate(char *dest, const char *src, size_t count)
{
char *tmp = dest;
if (count == 0)
return 0; /* no NUL-termination possible */
while ((*tmp++ = *src++) != '\0') {
if (--count == 0) {
*--tmp = '\0';
return 0;
}
}
return tmp - dest;
}
EXPORT_SYMBOL(strscpy_truncate);
/**
* strscpy - Copy a C-string into a sized buffer, but only if it fits
* @dest: Where to copy the string to
* @src: Where to copy the string from
* @count: Size of destination buffer
*
* Use this routine to avoid copying too-long strings.
* The routine returns the total number of bytes copied
* (including the trailing NUL) or zero if the buffer wasn't
* big enough. To ensure that programmers pay attention
* to the return code, the destination has a single NUL
* written at the front (if count is non-zero) when the
* buffer is not big enough.
*/
static size_t strscpy(char *dest, const char *src, size_t count)
{
size_t res = strscpy_truncate(dest, src, count);
if (res == 0 count != 0)
dest[0] = '\0';
return res;
}
EXPORT_SYMBOL(strscpy);
#endif
--
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: revert fs/befs/linuxvfs.c: replace strncpy by strlcpy
On Tue, Apr 28, 2015 at 06:42:10PM +0200, Fabian Frederick wrote:
On 28 April 2015 at 18:05 Al Viro v...@zeniv.linux.org.uk wrote:
On Tue, Apr 28, 2015 at 07:35:10AM +0200, Fabian Frederick wrote:
Al, very unhappy about the prospect of looking through ~2000 calls of
strlcpy()
we have in the tree...
Sorry Al, I thought it was more secure.
It's not just you, unfortunately, and dumping all that annoyance on you
as a proxy for everyone who does that kind of thing had been unfair.
My apologies...
No problem Al :) but why can't we harden strlcpy at first with
something like a strlen limited to max char.
(I don't know if it's already in kernel libs).
size_t strlenl(const char *s, size_t maxlen)
aka strnlen()
const char *sc = s;
size_t i = 0;
while (*sc != '\0' (i maxlen)) {
i++;
sc++;
}
return sc - s;
}
Then we could solve problems downstream ...
Can't. Seriously, look what strlcpy() is supposed to return; it's pretty
much a microoptimized snprintf(dst, size, %s, src). It's certainly
been patterned after snprintf(3) - don't exceed that size, NUL-terminate
unless the size is zero, return the number of characters (excluding NUL)
that would've been written if the size had been large enough.
The following is a legitimate use of strlcpy():
int foo(char *);/* modifies string */
int const_foo(const char *s)
{
int res;
char buf[32], *p = buf;
size_t wanted = strlcpy(buf, sizeof(buf), s);
if (wanted = sizeof(buf)) {
p = malloc(wanted + 1);
if (!p)
return -ENOMEM;
memcpy(p, s, wanted + 1);
}
res = foo(p);
if (p != buf)
free(p);
return res;
}
None of the kernel callers are of exactly that form (and most ignore the
return value completely), but if we make that sucker return something
different from what strlcpy(3) would return, we'd damn better _not_ keep
the name; there's enough confusion in that area as it is.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: revert fs/befs/linuxvfs.c: replace strncpy by strlcpy
On 04/28/2015 12:42 PM, Linus Torvalds wrote: On Tue, Apr 28, 2015 at 9:05 AM, Al Viro v...@zeniv.linux.org.uk wrote: Unfortunately, we _can't_ make strlcpy() never look past src + size - 1 - not without changing its semantics. Yeah, strlcpy is actually nasty. I don't understand why people like it so much. It's a crap interface, and completely unsuitable for untrusted sources because of the overrun issue. FWIW, I wanted to deal with some strncpy/strlcpy API issues last year and just put a strscpy() function in arch/tile/gxio/mpipe.c, with a comment saying it might be worth making it global at a later date, but at the time only the reviewers seemed interested in making it happen, so I let that possibility die on the vine. I argue that truncated strings are often pretty nasty, so you don't want to just silently say Sure, I made it fit! but instead make that be a failure case. In principle you could keep the return code of my proposed strscpy() and still do the truncated-string copy, but I think it's a mistake in almost every case, and if you really want that semantics, you should be forced to use something harder (e.g. some combination of explicit strlen and memcpy) so it's clear you know what you're doing. commit bceb7efa6a7e656bfaa67b6f54925e7db75bcd52 Author: Chris Metcalf cmetc...@tilera.com Date: Tue Sep 2 16:25:22 2014 -0400 tile gxio: use better string copy primitive Both strncpy and strlcpy suffer from the fact that they do partial copies of strings into the destination when the target buffer is too small. This is frequently pointless since an overflow of the target buffer may make the result invalid. strncpy() makes it relatively hard to even detect the error condition, and with strlcpy() you have to duplicate the buffer size parameter to test to see if the result exceeds it. By returning zero in the failure case, we both make testing for it easy, and by simply not copying anything in that case, we make it mandatory for callers to test the error code. To catch lazy programmers who don't check, we also place a NUL at the start of the destination buffer (if there is space) to ensure that the result is an invalid string. At some point it may make sense to promote strscpy() to a global platform-independent function, but other than the reviewers, no one was interested on LKML, so for now leave the strscpy() function as file-static. Reviewed-by: Randy Dunlap rdun...@infradead.org Reviewed-by: Rickard Strandqvist rickard_strandqv...@spectrumdigital.se Signed-off-by: Chris Metcalf cmetc...@tilera.com -- Chris Metcalf, EZChip Semiconductor http://www.ezchip.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy"
> On 28 April 2015 at 05:48 Al Viro wrote: > > > commit 39d7a29f867bd5a4a551fad6bb3812ceddb0bce1 > Author: Fabian Frederick > Date: Fri Jun 6 14:36:15 2014 -0700 > > fs/befs/linuxvfs.c: replace strncpy by strlcpy > > strncpy + end of string assignment replaced by strlcpy > > replaces perfectly safe code with undefined behaviour. All in the name > of "security hardening", presumably. Folks, seeing the words "designed to be > safer, more consistent, and less error prone replacement" in a manpage does > *NOT* mean "OK, quit reading it - no need to go further, not even to the end > of the paragraph". Because in the end of that paragraph it says "This means > that for strlcpy() src must be NUL-terminated". And sure enough, our > implementation relies on that - it starts with strlen(). > > strncpy() is guaranteed not to look further than size. strlcpy() is *NOT*. > strncpy() on unvalidated source is safe, provided that you sanitize the copy; > strlcpy() on anything like that is an invitation for nasal daemons. > > Sure, we can (and probably should) make strlcpy(dst, src, n) never access > memory beyond src + n - 1, but this kind of cargo-culting is a Bad Thing(tm); > mindless "security improvements" without so much as bothering to RTFM are > asking for trouble. And in userland code anything like that _can't_ be > papered over afterwards - not unless you can patch every libc implementation > out there. > > This particular code is completely pointless - if anything, it should've been > memcpy() + nd_terminate_link()... > > Al, very unhappy about the prospect of looking through ~2000 calls of > strlcpy() > we have in the tree... Sorry Al, I thought it was more secure. I guess the 2 following patches should be reversed as well : 6cb103b6f45a "fs/befs/btree.c: replace strncpy by strlcpy + coding style fixing" 69201bb11327 "fs/ocfs2/super.c: use OCFS2_MAX_VOL_LABEL_LEN and strlcpy" Regards, Fabian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: revert fs/befs/linuxvfs.c: replace strncpy by strlcpy
On 28 April 2015 at 05:48 Al Viro v...@zeniv.linux.org.uk wrote: commit 39d7a29f867bd5a4a551fad6bb3812ceddb0bce1 Author: Fabian Frederick f...@skynet.be Date: Fri Jun 6 14:36:15 2014 -0700 fs/befs/linuxvfs.c: replace strncpy by strlcpy strncpy + end of string assignment replaced by strlcpy replaces perfectly safe code with undefined behaviour. All in the name of security hardening, presumably. Folks, seeing the words designed to be safer, more consistent, and less error prone replacement in a manpage does *NOT* mean OK, quit reading it - no need to go further, not even to the end of the paragraph. Because in the end of that paragraph it says This means that for strlcpy() src must be NUL-terminated. And sure enough, our implementation relies on that - it starts with strlen(). strncpy() is guaranteed not to look further than size. strlcpy() is *NOT*. strncpy() on unvalidated source is safe, provided that you sanitize the copy; strlcpy() on anything like that is an invitation for nasal daemons. Sure, we can (and probably should) make strlcpy(dst, src, n) never access memory beyond src + n - 1, but this kind of cargo-culting is a Bad Thing(tm); mindless security improvements without so much as bothering to RTFM are asking for trouble. And in userland code anything like that _can't_ be papered over afterwards - not unless you can patch every libc implementation out there. This particular code is completely pointless - if anything, it should've been memcpy() + nd_terminate_link()... Al, very unhappy about the prospect of looking through ~2000 calls of strlcpy() we have in the tree... Sorry Al, I thought it was more secure. I guess the 2 following patches should be reversed as well : 6cb103b6f45a fs/befs/btree.c: replace strncpy by strlcpy + coding style fixing 69201bb11327 fs/ocfs2/super.c: use OCFS2_MAX_VOL_LABEL_LEN and strlcpy Regards, Fabian -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
