revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy"

revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy"

[Al Viro]

commit 39d7a29f867bd5a4a551fad6bb3812ceddb0bce1
Author: Fabian Frederick <fabf@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...

Re: revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy"

[Fabian Frederick]

> On 28 April 2015 at 05:48 Al Viro <viro@ZenIV.linux.org.uk> wrote:
>
>
> commit 39d7a29f867bd5a4a551fad6bb3812ceddb0bce1
> Author: Fabian Frederick <fabf@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

Re: revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy"

[Al Viro]

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...

Re: revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy"

[Fabian Frederick]

> On 28 April 2015 at 18:05 Al Viro <viro@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...

Re: revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy"

[Al Viro]

On Tue, Apr 28, 2015 at 06:42:10PM +0200, Fabian Frederick wrote:
> 
> 
> > On 28 April 2015 at 18:05 Al Viro <viro@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.

Re: revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy"

[Fabian Frederick]

> On 28 April 2015 at 19:39 Al Viro <viro@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 <viro@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

Re: revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy"

[Linus Torvalds]

On Tue, Apr 28, 2015 at 9:05 AM, Al Viro <viro@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

Re: revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy"

[Chris Metcalf]

On 04/28/2015 12:42 PM, Linus Torvalds wrote:
> On Tue, Apr 28, 2015 at 9:05 AM, Al Viro <viro@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 <cmetcalf@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 <rdunlap@infradead.org>
     Reviewed-by: Rickard Strandqvist 
<rickard_strandqvist@spectrumdigital.se>
     Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>


-- 
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com

Re: revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy"

[Linus Torvalds]

On Tue, Apr 28, 2015 at 12:48 PM, Chris Metcalf <cmetcalf@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

Re: revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy"

[Chris Metcalf]

On 04/28/2015 04:51 PM, Linus Torvalds wrote:
> On Tue, Apr 28, 2015 at 12:48 PM, Chris Metcalf <cmetcalf@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

Re: revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy"

[Linus Torvalds]

On Tue, Apr 28, 2015 at 2:38 PM, Chris Metcalf <cmetcalf@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

Re: revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy"

[Al Viro]

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.

Re: revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy"

[Geert Uytterhoeven]

On Wed, Apr 29, 2015 at 2:35 AM, Al Viro <viro@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 -- 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

[PATCH 0/3] add new strscpy() API for string copy

[Chris Metcalf]

This patch series addresses limitations in strncpy() and strlcpy();
both the old APIs are unpleasant, as Linus nicely summarized here
a couple of days ago:

  https://lkml.org/lkml/2015/4/28/570

and of course as other folks (Greg K-H and Linus again) said last year:

  https://plus.google.com/+gregkroahhartman/posts/1amLbuhWbh5

The proposed new API (strscpy(), for "s"afe string copy) has an
easy-to-use API for detecting buffer overflow, avoids unsafe truncation
by default, and isn't subject to thread-safety attacks like the current
strlcpy implementation.  See patch 2/3 for more on why strscpy() is a
good thing.

To make strscpy() work more efficiently I did the minimum tweaking
necessary to allow <asm/word-at-a-time.h> to work on all architectures,
though of course individual maintainers can still make their versions
more efficient as needed.

It's likely not necessary for per-architecture implementations of
strscpy() to be written, but I stuck with the standard __HAVE_ARCH_XXX
model just for consistency with the rest of <linux/string.h>.

I tested the implementation with a simple user-space harness, so I
believe it is correct for the corner cases I could think of.  In
particular I pairwise-tested all the unaligned values of source and
dest, and tested the restriction on src page-crossing at all
unaligned offsets approaching the page boundary.

This builds on an earlier version of strscpy() submitted as
a file-static method in the arch/tile/gxio tree last year, after
an attempt to gather interest in a new generic strscpy failed:

  https://lkml.org/lkml/2014/8/7/368

The patch series is available to be pulled from

  git://git.kernel.org/pub/scm/linux/kernel/git/cmetcalf/linux-tile.git strscpy

Chris Metcalf (3):
  Make asm/word-at-a-time.h available on all architectures
  string: provide strscpy() and strscpy_truncate()
  tile: use global strscpy() rather than private copy

 arch/arc/include/asm/Kbuild          |   1 +
 arch/avr32/include/asm/Kbuild        |   1 +
 arch/blackfin/include/asm/Kbuild     |   1 +
 arch/c6x/include/asm/Kbuild          |   1 +
 arch/cris/include/asm/Kbuild         |   1 +
 arch/frv/include/asm/Kbuild          |   1 +
 arch/hexagon/include/asm/Kbuild      |   1 +
 arch/ia64/include/asm/Kbuild         |   1 +
 arch/m32r/include/asm/Kbuild         |   1 +
 arch/metag/include/asm/Kbuild        |   1 +
 arch/microblaze/include/asm/Kbuild   |   1 +
 arch/mips/include/asm/Kbuild         |   1 +
 arch/mn10300/include/asm/Kbuild      |   1 +
 arch/nios2/include/asm/Kbuild        |   1 +
 arch/powerpc/include/asm/Kbuild      |   1 +
 arch/s390/include/asm/Kbuild         |   1 +
 arch/score/include/asm/Kbuild        |   1 +
 arch/tile/gxio/mpipe.c               |  33 ++---------
 arch/tile/include/asm/Kbuild         |   1 +
 arch/um/include/asm/Kbuild           |   1 +
 arch/unicore32/include/asm/Kbuild    |   1 +
 arch/xtensa/include/asm/Kbuild       |   1 +
 include/asm-generic/word-at-a-time.h |  80 ++++++++++++++++++++++---
 include/linux/string.h               |   6 ++
 lib/string.c                         | 109 +++++++++++++++++++++++++++++++++++
 25 files changed, 212 insertions(+), 37 deletions(-)

-- 
2.1.2

[PATCH 1/3] Make asm/word-at-a-time.h available on all architectures

[Chris Metcalf]

Added the x86 implementation of word-at-a-time to the
generic version, which previously only supported big-endian.

Omitted the x86-specific load_unaligned_zeropad(), which in
any case is also not present for the existing BE-only
implementation of a word-at-a-time, and is only used under
CONFIG_DCACHE_WORD_ACCESS.

Added as a "generic-y" to the Kbuilds of all architectures
that didn't previously have it.

Signed-off-by: Chris Metcalf <cmetcalf@ezchip.com>
---
 arch/arc/include/asm/Kbuild          |  1 +
 arch/avr32/include/asm/Kbuild        |  1 +
 arch/blackfin/include/asm/Kbuild     |  1 +
 arch/c6x/include/asm/Kbuild          |  1 +
 arch/cris/include/asm/Kbuild         |  1 +
 arch/frv/include/asm/Kbuild          |  1 +
 arch/hexagon/include/asm/Kbuild      |  1 +
 arch/ia64/include/asm/Kbuild         |  1 +
 arch/m32r/include/asm/Kbuild         |  1 +
 arch/metag/include/asm/Kbuild        |  1 +
 arch/microblaze/include/asm/Kbuild   |  1 +
 arch/mips/include/asm/Kbuild         |  1 +
 arch/mn10300/include/asm/Kbuild      |  1 +
 arch/nios2/include/asm/Kbuild        |  1 +
 arch/powerpc/include/asm/Kbuild      |  1 +
 arch/s390/include/asm/Kbuild         |  1 +
 arch/score/include/asm/Kbuild        |  1 +
 arch/tile/include/asm/Kbuild         |  1 +
 arch/um/include/asm/Kbuild           |  1 +
 arch/unicore32/include/asm/Kbuild    |  1 +
 arch/xtensa/include/asm/Kbuild       |  1 +
 include/asm-generic/word-at-a-time.h | 80 ++++++++++++++++++++++++++++++++----
 22 files changed, 93 insertions(+), 8 deletions(-)

diff --git a/arch/arc/include/asm/Kbuild b/arch/arc/include/asm/Kbuild
index be0c39e76f7c..be895ef03a51 100644
--- a/arch/arc/include/asm/Kbuild
+++ b/arch/arc/include/asm/Kbuild
@@ -49,4 +49,5 @@ generic-y += types.h
 generic-y += ucontext.h
 generic-y += user.h
 generic-y += vga.h
+generic-y += word-at-a-time.h
 generic-y += xor.h
diff --git a/arch/avr32/include/asm/Kbuild b/arch/avr32/include/asm/Kbuild
index 528d70d47a54..10dd6e8470bd 100644
--- a/arch/avr32/include/asm/Kbuild
+++ b/arch/avr32/include/asm/Kbuild
@@ -20,4 +20,5 @@ generic-y += sections.h
 generic-y += topology.h
 generic-y += trace_clock.h
 generic-y += vga.h
+generic-y += word-at-a-time.h
 generic-y += xor.h
diff --git a/arch/blackfin/include/asm/Kbuild b/arch/blackfin/include/asm/Kbuild
index 4bd3c3cfc9ab..9705a527b7b0 100644
--- a/arch/blackfin/include/asm/Kbuild
+++ b/arch/blackfin/include/asm/Kbuild
@@ -46,4 +46,5 @@ generic-y += types.h
 generic-y += ucontext.h
 generic-y += unaligned.h
 generic-y += user.h
+generic-y += word-at-a-time.h
 generic-y += xor.h
diff --git a/arch/c6x/include/asm/Kbuild b/arch/c6x/include/asm/Kbuild
index ae0a51f5376c..535efdb0692d 100644
--- a/arch/c6x/include/asm/Kbuild
+++ b/arch/c6x/include/asm/Kbuild
@@ -59,4 +59,5 @@ generic-y += types.h
 generic-y += ucontext.h
 generic-y += user.h
 generic-y += vga.h
+generic-y += word-at-a-time.h
 generic-y += xor.h
diff --git a/arch/cris/include/asm/Kbuild b/arch/cris/include/asm/Kbuild
index 057e51859b0a..4991539a25f6 100644
--- a/arch/cris/include/asm/Kbuild
+++ b/arch/cris/include/asm/Kbuild
@@ -26,4 +26,5 @@ generic-y += sections.h
 generic-y += topology.h
 generic-y += trace_clock.h
 generic-y += vga.h
+generic-y += word-at-a-time.h
 generic-y += xor.h
diff --git a/arch/frv/include/asm/Kbuild b/arch/frv/include/asm/Kbuild
index e3f81b53578e..ca34d1cf2cdf 100644
--- a/arch/frv/include/asm/Kbuild
+++ b/arch/frv/include/asm/Kbuild
@@ -7,3 +7,4 @@ generic-y += mcs_spinlock.h
 generic-y += preempt.h
 generic-y += scatterlist.h
 generic-y += trace_clock.h
+generic-y += word-at-a-time.h
diff --git a/arch/hexagon/include/asm/Kbuild b/arch/hexagon/include/asm/Kbuild
index c7a99f860b40..872920fba0a6 100644
--- a/arch/hexagon/include/asm/Kbuild
+++ b/arch/hexagon/include/asm/Kbuild
@@ -58,4 +58,5 @@ generic-y += types.h
 generic-y += ucontext.h
 generic-y += unaligned.h
 generic-y += vga.h
+generic-y += word-at-a-time.h
 generic-y += xor.h
diff --git a/arch/ia64/include/asm/Kbuild b/arch/ia64/include/asm/Kbuild
index 9b41b4bcc073..614579fa1e49 100644
--- a/arch/ia64/include/asm/Kbuild
+++ b/arch/ia64/include/asm/Kbuild
@@ -8,3 +8,4 @@ generic-y += preempt.h
 generic-y += scatterlist.h
 generic-y += trace_clock.h
 generic-y += vtime.h
+generic-y += word-at-a-time.h
diff --git a/arch/m32r/include/asm/Kbuild b/arch/m32r/include/asm/Kbuild
index 2edc793372fc..36f850dd4a1e 100644
--- a/arch/m32r/include/asm/Kbuild
+++ b/arch/m32r/include/asm/Kbuild
@@ -9,3 +9,4 @@ generic-y += preempt.h
 generic-y += scatterlist.h
 generic-y += sections.h
 generic-y += trace_clock.h
+generic-y += word-at-a-time.h
diff --git a/arch/metag/include/asm/Kbuild b/arch/metag/include/asm/Kbuild
index 0bf5d525b945..a291b3b2529e 100644
--- a/arch/metag/include/asm/Kbuild
+++ b/arch/metag/include/asm/Kbuild
@@ -54,4 +54,5 @@ generic-y += ucontext.h
 generic-y += unaligned.h
 generic-y += user.h
 generic-y += vga.h
+generic-y += word-at-a-time.h
 generic-y += xor.h
diff --git a/arch/microblaze/include/asm/Kbuild b/arch/microblaze/include/asm/Kbuild
index ab564a6db5c3..966f221b2d67 100644
--- a/arch/microblaze/include/asm/Kbuild
+++ b/arch/microblaze/include/asm/Kbuild
@@ -10,3 +10,4 @@ generic-y += preempt.h
 generic-y += scatterlist.h
 generic-y += syscalls.h
 generic-y += trace_clock.h
+generic-y += word-at-a-time.h
diff --git a/arch/mips/include/asm/Kbuild b/arch/mips/include/asm/Kbuild
index 526539cbc99f..c3cfb83aca1d 100644
--- a/arch/mips/include/asm/Kbuild
+++ b/arch/mips/include/asm/Kbuild
@@ -18,4 +18,5 @@ generic-y += serial.h
 generic-y += trace_clock.h
 generic-y += ucontext.h
 generic-y += user.h
+generic-y += word-at-a-time.h
 generic-y += xor.h
diff --git a/arch/mn10300/include/asm/Kbuild b/arch/mn10300/include/asm/Kbuild
index f892d9de47d9..30a06e2d6e42 100644
--- a/arch/mn10300/include/asm/Kbuild
+++ b/arch/mn10300/include/asm/Kbuild
@@ -9,3 +9,4 @@ generic-y += preempt.h
 generic-y += scatterlist.h
 generic-y += sections.h
 generic-y += trace_clock.h
+generic-y += word-at-a-time.h
diff --git a/arch/nios2/include/asm/Kbuild b/arch/nios2/include/asm/Kbuild
index 24b3d8999ac7..bc87081c0669 100644
--- a/arch/nios2/include/asm/Kbuild
+++ b/arch/nios2/include/asm/Kbuild
@@ -61,4 +61,5 @@ generic-y += types.h
 generic-y += unaligned.h
 generic-y += user.h
 generic-y += vga.h
+generic-y += word-at-a-time.h
 generic-y += xor.h
diff --git a/arch/powerpc/include/asm/Kbuild b/arch/powerpc/include/asm/Kbuild
index 4b87205c230c..dfc61a151890 100644
--- a/arch/powerpc/include/asm/Kbuild
+++ b/arch/powerpc/include/asm/Kbuild
@@ -9,3 +9,4 @@ generic-y += rwsem.h
 generic-y += scatterlist.h
 generic-y += trace_clock.h
 generic-y += vtime.h
+generic-y += word-at-a-time.h
diff --git a/arch/s390/include/asm/Kbuild b/arch/s390/include/asm/Kbuild
index c631f98fd524..27c37b3d1dad 100644
--- a/arch/s390/include/asm/Kbuild
+++ b/arch/s390/include/asm/Kbuild
@@ -6,3 +6,4 @@ generic-y += mcs_spinlock.h
 generic-y += preempt.h
 generic-y += scatterlist.h
 generic-y += trace_clock.h
+generic-y += word-at-a-time.h
diff --git a/arch/score/include/asm/Kbuild b/arch/score/include/asm/Kbuild
index 83ed116d414c..16958f529aa7 100644
--- a/arch/score/include/asm/Kbuild
+++ b/arch/score/include/asm/Kbuild
@@ -13,3 +13,4 @@ generic-y += sections.h
 generic-y += trace_clock.h
 generic-y += xor.h
 generic-y += serial.h
+generic-y += word-at-a-time.h
diff --git a/arch/tile/include/asm/Kbuild b/arch/tile/include/asm/Kbuild
index f5433e0e34e0..0804122e579a 100644
--- a/arch/tile/include/asm/Kbuild
+++ b/arch/tile/include/asm/Kbuild
@@ -39,4 +39,5 @@ generic-y += termbits.h
 generic-y += termios.h
 generic-y += trace_clock.h
 generic-y += types.h
+generic-y += word-at-a-time.h
 generic-y += xor.h
diff --git a/arch/um/include/asm/Kbuild b/arch/um/include/asm/Kbuild
index 9176fa11d49b..ee02364d8922 100644
--- a/arch/um/include/asm/Kbuild
+++ b/arch/um/include/asm/Kbuild
@@ -26,4 +26,5 @@ generic-y += sections.h
 generic-y += switch_to.h
 generic-y += topology.h
 generic-y += trace_clock.h
+generic-y += word-at-a-time.h
 generic-y += xor.h
diff --git a/arch/unicore32/include/asm/Kbuild b/arch/unicore32/include/asm/Kbuild
index 3e0c19d0f4c5..f456bfa1dd54 100644
--- a/arch/unicore32/include/asm/Kbuild
+++ b/arch/unicore32/include/asm/Kbuild
@@ -62,4 +62,5 @@ generic-y += ucontext.h
 generic-y += unaligned.h
 generic-y += user.h
 generic-y += vga.h
+generic-y += word-at-a-time.h
 generic-y += xor.h
diff --git a/arch/xtensa/include/asm/Kbuild b/arch/xtensa/include/asm/Kbuild
index 86a9ab2e2ca9..6ac0a9ab3ea3 100644
--- a/arch/xtensa/include/asm/Kbuild
+++ b/arch/xtensa/include/asm/Kbuild
@@ -29,4 +29,5 @@ generic-y += statfs.h
 generic-y += termios.h
 generic-y += topology.h
 generic-y += trace_clock.h
+generic-y += word-at-a-time.h
 generic-y += xor.h
diff --git a/include/asm-generic/word-at-a-time.h b/include/asm-generic/word-at-a-time.h
index 94f9ea8abcae..011dde083f23 100644
--- a/include/asm-generic/word-at-a-time.h
+++ b/include/asm-generic/word-at-a-time.h
@@ -1,15 +1,10 @@
 #ifndef _ASM_WORD_AT_A_TIME_H
 #define _ASM_WORD_AT_A_TIME_H
 
-/*
- * This says "generic", but it's actually big-endian only.
- * Little-endian can use more efficient versions of these
- * interfaces, see for example
- *	 arch/x86/include/asm/word-at-a-time.h
- * for those.
- */
-
 #include <linux/kernel.h>
+#include <asm/byteorder.h>
+
+#ifdef __BIG_ENDIAN
 
 struct word_at_a_time {
 	const unsigned long high_bits, low_bits;
@@ -53,4 +48,73 @@ static inline bool has_zero(unsigned long val, unsigned long *data, const struct
 #define zero_bytemask(mask) (~1ul << __fls(mask))
 #endif
 
+#else
+
+/*
+ * The optimal byte mask counting is probably going to be something
+ * that is architecture-specific. If you have a reliably fast
+ * bit count instruction, that might be better than the multiply
+ * and shift, for example.
+ */
+struct word_at_a_time {
+	const unsigned long one_bits, high_bits;
+};
+
+#define WORD_AT_A_TIME_CONSTANTS { REPEAT_BYTE(0x01), REPEAT_BYTE(0x80) }
+
+#ifdef CONFIG_64BIT
+
+/*
+ * Jan Achrenius on G+: microoptimized version of
+ * the simpler "(mask & ONEBYTES) * ONEBYTES >> 56"
+ * that works for the bytemasks without having to
+ * mask them first.
+ */
+static inline long count_masked_bytes(unsigned long mask)
+{
+	return mask*0x0001020304050608ul >> 56;
+}
+
+#else	/* 32-bit case */
+
+/* Carl Chatfield / Jan Achrenius G+ version for 32-bit */
+static inline long count_masked_bytes(long mask)
+{
+	/* (000000 0000ff 00ffff ffffff) -> ( 1 1 2 3 ) */
+	long a = (0x0ff0001+mask) >> 23;
+	/* Fix the 1 for 00 case */
+	return a & mask;
+}
+
+#endif
+
+/* Return nonzero if it has a zero */
+static inline unsigned long has_zero(unsigned long a, unsigned long *bits, const struct word_at_a_time *c)
+{
+	unsigned long mask = ((a - c->one_bits) & ~a) & c->high_bits;
+	*bits = mask;
+	return mask;
+}
+
+static inline unsigned long prep_zero_mask(unsigned long a, unsigned long bits, const struct word_at_a_time *c)
+{
+	return bits;
+}
+
+static inline unsigned long create_zero_mask(unsigned long bits)
+{
+	bits = (bits - 1) & ~bits;
+	return bits >> 7;
+}
+
+/* The mask we created is directly usable as a bytemask */
+#define zero_bytemask(mask) (mask)
+
+static inline unsigned long find_zero(unsigned long mask)
+{
+	return count_masked_bytes(mask);
+}
+
+#endif /* __BIG_ENDIAN */
+
 #endif /* _ASM_WORD_AT_A_TIME_H */
-- 
2.1.2

[PATCH 2/3] string: provide strscpy() and strscpy_truncate()

[Chris Metcalf]

The strscpy() API is intended to be used instead of strlcpy(),
and instead of most uses of strncpy().

- The API provides an easy way to check for destination buffer overflow:
  a -E2BIG error return value.

- By default, truncation causes the destination buffer to be the
  empty string, so users don't blindly assume a truncated string is
  valid.  If you know a truncated string still has valid semantics,
  you can use the provided strscpy_truncate(), which has the same
  return value semantics but does not make the destination an empty
  string on error.

- The provided implementation is robust in the face of the source
  buffer being asynchronously changed during the copy, unlike the
  current implementation of strlcpy().

- The implementation should be reasonably performant on all
  platforms since it uses the asm/word-at-a-time.h API rather than
  simple byte copy.  Kernel-to-kernel string copy is not considered
  to be performance critical in any case.

- If the remainder of the destination buffer must be zeroed for some
  reason, strscpy() + error check + memset() is probably the easiest
  pattern, but using strncpy() in a pattern where the last byte of
  the buffer is first set non-NUL, then tested for NUL afterwards,
  can also be safely used.

Signed-off-by: Chris Metcalf <cmetcalf@ezchip.com>
---
 include/linux/string.h |   6 +++
 lib/string.c           | 109 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 115 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index e40099e585c9..7942944f3abb 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -25,6 +25,12 @@ extern char * strncpy(char *,const char *, __kernel_size_t);
 #ifndef __HAVE_ARCH_STRLCPY
 size_t strlcpy(char *, const char *, size_t);
 #endif
+#ifndef __HAVE_ARCH_STRSCPY
+ssize_t strscpy(char *, const char *, size_t);
+#endif
+#ifndef __HAVE_ARCH_STRSCPY_TRUNCATE
+ssize_t strscpy_truncate(char *, const char *, size_t);
+#endif
 #ifndef __HAVE_ARCH_STRCAT
 extern char * strcat(char *, const char *);
 #endif
diff --git a/lib/string.c b/lib/string.c
index a5792019193c..84d5b6eceb74 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -27,6 +27,9 @@
 #include <linux/bug.h>
 #include <linux/errno.h>
 
+#include <asm/byteorder.h>
+#include <asm/word-at-a-time.h>
+
 #ifndef __HAVE_ARCH_STRNCASECMP
 /**
  * strncasecmp - Case insensitive, length-limited string comparison
@@ -146,6 +149,112 @@ size_t strlcpy(char *dest, const char *src, size_t size)
 EXPORT_SYMBOL(strlcpy);
 #endif
 
+#ifndef __HAVE_ARCH_STRSCPY_TRUNCATE
+/**
+ * strscpy_truncate - Copy a C-string into a sized buffer
+ * @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 number of characters copied (not including
+ * the trailing NUL) or -E2BIG if the destination buffer wasn't big enough.
+ * The behavior is undefined if the string buffers overlap.
+ *
+ * Note that the implementation is robust to the string changing out
+ * from underneath it, unlike the current strlcpy() implementation,
+ * and it is easier to check overflow than with strlcpy()'s API.
+ *
+ * strscpy() is preferred over this function unless a truncated string
+ * provides some valid semantics in the destination buffer.
+ */
+ssize_t strscpy_truncate(char *dest, const char *src, size_t count)
+{
+	const struct word_at_a_time constants = WORD_AT_A_TIME_CONSTANTS;
+	size_t max = count;
+	long res = 0;
+
+	if (count == 0)
+		return -E2BIG;
+
+#ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
+	/*
+	 * If src is unaligned, don't cross a page boundary,
+	 * since we don't know if the next page is mapped.
+	 */
+	if ((long)src & (sizeof(long) - 1)) {
+		size_t limit = PAGE_SIZE - ((long)src & (PAGE_SIZE - 1));
+		if (limit < max)
+			max = limit;
+	}
+#else
+	/* If src or dest is unaligned, don't do word-at-a-time. */
+	if (((long) dest | (long) src) & (sizeof(long) - 1))
+		max = 0;
+#endif
+
+	while (max >= sizeof(unsigned long)) {
+		unsigned long c, data;
+
+		c = *(unsigned long *)(src+res);
+		*(unsigned long *)(dest+res) = c;
+		if (has_zero(c, &data, &constants)) {
+			data = prep_zero_mask(c, data, &constants);
+			data = create_zero_mask(data);
+			return res + find_zero(data);
+		}
+		res += sizeof(unsigned long);
+		count -= sizeof(unsigned long);
+		max -= sizeof(unsigned long);
+	}
+
+	while (count) {
+		char c;
+
+		c = src[res];
+		dest[res] = c;
+		if (!c)
+			return res;
+		res++;
+		count--;
+	}
+
+	/* Hit buffer length without finding a NUL; force NUL-termination. */
+	if (res)
+		dest[res-1] = '\0';
+
+	return -E2BIG;
+}
+EXPORT_SYMBOL(strscpy_truncate);
+#endif
+
+#ifndef __HAVE_ARCH_STRSCPY
+/**
+ * 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
+ *
+ * Copy the string into the dest buffer.  The routine returns the
+ * number of characters copied (not including the trailing NUL) or
+ * -E2BIG if the destination buffer wasn't big enough.  The behavior
+ * is undefined if the string buffers overlap.  The destination buffer
+ * is set to the empty string if the buffer is not big enough.
+ *
+ * Preferred over strlcpy() in all cases, and over strncpy() unless
+ * the latter's zero-fill behavior is desired and truncation of the
+ * source string is known not to be an issue.
+ */
+ssize_t strscpy(char *dest, const char *src, size_t count)
+{
+	ssize_t res = strscpy_truncate(dest, src, count);
+	if (res < 0 && count != 0)
+		dest[0] = '\0';
+	return res;
+}
+EXPORT_SYMBOL(strscpy);
+#endif
+
 #ifndef __HAVE_ARCH_STRCAT
 /**
  * strcat - Append one %NUL-terminated string to another
-- 
2.1.2

Re: [PATCH 2/3] string: provide strscpy() and strscpy_truncate()

[Dan Carpenter]

On Thu, Apr 30, 2015 at 12:01:16PM -0400, Chris Metcalf wrote:
> +ssize_t strscpy(char *dest, const char *src, size_t count)
> +{
> +	ssize_t res = strscpy_truncate(dest, src, count);
> +	if (res < 0 && count != 0)
> +		dest[0] = '\0';

How is this better than returning a truncated string?  Is it just
because the caller was naughty so we give them a spanking?

> +	return res;
> +}

regards,
dan carpenter

Re: [PATCH 2/3] string: provide strscpy() and strscpy_truncate()

[Chris Metcalf]

On 5/6/2015 11:01 AM, Dan Carpenter wrote:
> On Thu, Apr 30, 2015 at 12:01:16PM -0400, Chris Metcalf wrote:
>> >+ssize_t strscpy(char *dest, const char *src, size_t count)
>> >+{
>> >+	ssize_t res = strscpy_truncate(dest, src, count);
>> >+	if (res < 0 && count != 0)
>> >+		dest[0] = '\0';
> How is this better than returning a truncated string?  Is it just
> because the caller was naughty so we give them a spanking?

There are basically two issues here:

1. A truncated string with an error return may still cause program errors,
even if the caller checks for the error return, if the buffer is later interpreted
as a valid string due to some other program error.  It's defensive programming.

2. Programmers are fond of ignoring error returns.  My experience with
truncated strings is that in too many cases, truncation causes program
errors down the line.  It's better to ensure that no partial string is returned
in this case.

In a perfect world, all error returns would be checked, and there would be
no need for this, but we definitely don't live in that world :-)

That said, although I think my approach is correct, I'm open to a consensus
that having strscpy() leave a truncated string in the dest buffer is better.

-- 
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com

Re: [PATCH 2/3] string: provide strscpy() and strscpy_truncate()

[Dan Carpenter]

We actually do have a __must_check tag so it's easy enough to force
people to check.  A different option is we could make it trigger a
WARN_ONCE().

#define strXcpy(dest, src, len) (({				\
	ssize_t __ret = strscpy_truncate(dest, src, len);	\
	WARN_ONCE(__ret < 0, "strXcpy trancates\n");		\
	__ret; }))

I have never really cared about truncation, though.  I think not caring
is the common case.

regards,
dan carpenter

Re: [PATCH 2/3] string: provide strscpy() and strscpy_truncate()

[Geert Uytterhoeven]

On Wed, May 6, 2015 at 5:59 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> We actually do have a __must_check tag so it's easy enough to force
> people to check.  A different option is we could make it trigger a

People tend to ignore compiler warnings...

> WARN_ONCE().
>
> #define strXcpy(dest, src, len) (({                             \
>         ssize_t __ret = strscpy_truncate(dest, src, len);       \
>         WARN_ONCE(__ret < 0, "strXcpy trancates\n");            \
>         __ret; }))

Which will probably trigger only in extreme cases in the wild, not during
development.

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 2/3] string: provide strscpy() and strscpy_truncate()

[Dan Carpenter]

On Wed, May 06, 2015 at 06:45:56PM +0200, Geert Uytterhoeven wrote:
> On Wed, May 6, 2015 at 5:59 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > We actually do have a __must_check tag so it's easy enough to force
> > people to check.  A different option is we could make it trigger a
> 
> People tend to ignore compiler warnings...

We're doing a lot better these days with zero day build testing.  There
is not even one ignored __must_check return in my allmodconfig.

> 
> > WARN_ONCE().
> >
> > #define strXcpy(dest, src, len) (({                             \
> >         ssize_t __ret = strscpy_truncate(dest, src, len);       \
> >         WARN_ONCE(__ret < 0, "strXcpy trancates\n");            \
> >         __ret; }))
> 
> Which will probably trigger only in extreme cases in the wild, not during
> development.

It's less subtle than just putting an empty string there so we're more
likely to get bug reports than with the original code.

regards,
dan carpenter

Re: [PATCH 2/3] string: provide strscpy() and strscpy_truncate()

[Chris Metcalf]

On 05/07/2015 05:00 AM, Dan Carpenter wrote:
> On Wed, May 06, 2015 at 06:45:56PM +0200, Geert Uytterhoeven wrote:
>> On Wed, May 6, 2015 at 5:59 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>>> We actually do have a __must_check tag so it's easy enough to force
>>> people to check.  A different option is we could make it trigger a
>> People tend to ignore compiler warnings...
> We're doing a lot better these days with zero day build testing.  There
> is not even one ignored __must_check return in my allmodconfig.

If we keep the strscpy/strscpy_truncate distinction, I agree that having
__must_check on strscpy seems like a good idea.

>>> WARN_ONCE().
>>>
>>> #define strXcpy(dest, src, len) (({                             \
>>>          ssize_t __ret = strscpy_truncate(dest, src, len);       \
>>>          WARN_ONCE(__ret < 0, "strXcpy trancates\n");            \
>>>          __ret; }))
>> Which will probably trigger only in extreme cases in the wild, not during
>> development.
> It's less subtle than just putting an empty string there so we're more
> likely to get bug reports than with the original code.

The problem with WARN_ONCE() here is that we may be using strscpy()
to take user input of some kind.  If so, we don't want to warn if we
are truncating the string - we just want to return a suitable error up
the call stack.

-- 
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com

[PATCH 3/3] tile: use global strscpy() rather than private copy

[Chris Metcalf]

Now that strscpy() is a standard API, remove the local copy.

Signed-off-by: Chris Metcalf <cmetcalf@ezchip.com>
---
 arch/tile/gxio/mpipe.c | 33 ++++-----------------------------
 1 file changed, 4 insertions(+), 29 deletions(-)

diff --git a/arch/tile/gxio/mpipe.c b/arch/tile/gxio/mpipe.c
index ee186e13dfe6..f102048d9c0e 100644
--- a/arch/tile/gxio/mpipe.c
+++ b/arch/tile/gxio/mpipe.c
@@ -19,6 +19,7 @@
 #include <linux/errno.h>
 #include <linux/io.h>
 #include <linux/module.h>
+#include <linux/string.h>
 
 #include <gxio/iorpc_globals.h>
 #include <gxio/iorpc_mpipe.h>
@@ -29,32 +30,6 @@
 /* HACK: Avoid pointless "shadow" warnings. */
 #define link link_shadow
 
-/**
- * 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
- * @size: 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 size is non-zero) when the
- * buffer is not big enough.
- */
-static size_t strscpy(char *dest, const char *src, size_t size)
-{
-	size_t len = strnlen(src, size) + 1;
-	if (len > size) {
-		if (size)
-			dest[0] = '\0';
-		return 0;
-	}
-	memcpy(dest, src, len);
-	return len;
-}
-
 int gxio_mpipe_init(gxio_mpipe_context_t *context, unsigned int mpipe_index)
 {
 	char file[32];
@@ -540,7 +515,7 @@ int gxio_mpipe_link_instance(const char *link_name)
 	if (!context)
 		return GXIO_ERR_NO_DEVICE;
 
-	if (strscpy(name.name, link_name, sizeof(name.name)) == 0)
+	if (strscpy(name.name, link_name, sizeof(name.name)) < 0)
 		return GXIO_ERR_NO_DEVICE;
 
 	return gxio_mpipe_info_instance_aux(context, name);
@@ -559,7 +534,7 @@ int gxio_mpipe_link_enumerate_mac(int idx, char *link_name, uint8_t *link_mac)
 
 	rv = gxio_mpipe_info_enumerate_aux(context, idx, &name, &mac);
 	if (rv >= 0) {
-		if (strscpy(link_name, name.name, sizeof(name.name)) == 0)
+		if (strscpy(link_name, name.name, sizeof(name.name)) < 0)
 			return GXIO_ERR_INVAL_MEMORY_SIZE;
 		memcpy(link_mac, mac.mac, sizeof(mac.mac));
 	}
@@ -576,7 +551,7 @@ int gxio_mpipe_link_open(gxio_mpipe_link_t *link,
 	_gxio_mpipe_link_name_t name;
 	int rv;
 
-	if (strscpy(name.name, link_name, sizeof(name.name)) == 0)
+	if (strscpy(name.name, link_name, sizeof(name.name)) < 0)
 		return GXIO_ERR_NO_DEVICE;
 
 	rv = gxio_mpipe_link_open_aux(context, name, flags);
-- 
2.1.2

Re: [PATCH 0/3] add new strscpy() API for string copy

[Chris Metcalf]

Ping!  There was a little feedback on the strscpy() patch series,
but I think at this point it boiled down to adding a __must_check
on strscpy(), which I've done.  Any further opinions?  Would
anyone like to volunteer to take this into their tree?  Or Linus,
are you ready to pull it directly when the merge window opens?

git://git.kernel.org/pub/scm/linux/kernel/git/cmetcalf/linux-tile.git 
strscpy

Thanks!

On 04/30/2015 12:01 PM, Chris Metcalf wrote:
> This patch series addresses limitations in strncpy() and strlcpy();
> both the old APIs are unpleasant, as Linus nicely summarized here
> a couple of days ago:
>
>    https://lkml.org/lkml/2015/4/28/570
>
> and of course as other folks (Greg K-H and Linus again) said last year:
>
>    https://plus.google.com/+gregkroahhartman/posts/1amLbuhWbh5
>
> The proposed new API (strscpy(), for "s"afe string copy) has an
> easy-to-use API for detecting buffer overflow, avoids unsafe truncation
> by default, and isn't subject to thread-safety attacks like the current
> strlcpy implementation.  See patch 2/3 for more on why strscpy() is a
> good thing.
>
> To make strscpy() work more efficiently I did the minimum tweaking
> necessary to allow <asm/word-at-a-time.h> to work on all architectures,
> though of course individual maintainers can still make their versions
> more efficient as needed.
>
> It's likely not necessary for per-architecture implementations of
> strscpy() to be written, but I stuck with the standard __HAVE_ARCH_XXX
> model just for consistency with the rest of <linux/string.h>.
>
> I tested the implementation with a simple user-space harness, so I
> believe it is correct for the corner cases I could think of.  In
> particular I pairwise-tested all the unaligned values of source and
> dest, and tested the restriction on src page-crossing at all
> unaligned offsets approaching the page boundary.
>
> This builds on an earlier version of strscpy() submitted as
> a file-static method in the arch/tile/gxio tree last year, after
> an attempt to gather interest in a new generic strscpy failed:
>
>    https://lkml.org/lkml/2014/8/7/368
>
> The patch series is available to be pulled from
>
>    git://git.kernel.org/pub/scm/linux/kernel/git/cmetcalf/linux-tile.git strscpy
>
> Chris Metcalf (3):
>    Make asm/word-at-a-time.h available on all architectures
>    string: provide strscpy() and strscpy_truncate()
>    tile: use global strscpy() rather than private copy
>
>   arch/arc/include/asm/Kbuild          |   1 +
>   arch/avr32/include/asm/Kbuild        |   1 +
>   arch/blackfin/include/asm/Kbuild     |   1 +
>   arch/c6x/include/asm/Kbuild          |   1 +
>   arch/cris/include/asm/Kbuild         |   1 +
>   arch/frv/include/asm/Kbuild          |   1 +
>   arch/hexagon/include/asm/Kbuild      |   1 +
>   arch/ia64/include/asm/Kbuild         |   1 +
>   arch/m32r/include/asm/Kbuild         |   1 +
>   arch/metag/include/asm/Kbuild        |   1 +
>   arch/microblaze/include/asm/Kbuild   |   1 +
>   arch/mips/include/asm/Kbuild         |   1 +
>   arch/mn10300/include/asm/Kbuild      |   1 +
>   arch/nios2/include/asm/Kbuild        |   1 +
>   arch/powerpc/include/asm/Kbuild      |   1 +
>   arch/s390/include/asm/Kbuild         |   1 +
>   arch/score/include/asm/Kbuild        |   1 +
>   arch/tile/gxio/mpipe.c               |  33 ++---------
>   arch/tile/include/asm/Kbuild         |   1 +
>   arch/um/include/asm/Kbuild           |   1 +
>   arch/unicore32/include/asm/Kbuild    |   1 +
>   arch/xtensa/include/asm/Kbuild       |   1 +
>   include/asm-generic/word-at-a-time.h |  80 ++++++++++++++++++++++---
>   include/linux/string.h               |   6 ++
>   lib/string.c                         | 109 +++++++++++++++++++++++++++++++++++
>   25 files changed, 212 insertions(+), 37 deletions(-)
>

-- 
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com

Re: [PATCH 0/3] add new strscpy() API for string copy

[Michael Ellerman]

On Thu, 2015-04-30 at 12:01 -0400, Chris Metcalf wrote:
> This patch series addresses limitations in strncpy() and strlcpy();
> both the old APIs are unpleasant, as Linus nicely summarized here
> a couple of days ago:
> 
>   https://lkml.org/lkml/2015/4/28/570
> 
> and of course as other folks (Greg K-H and Linus again) said last year:
> 
>   https://plus.google.com/+gregkroahhartman/posts/1amLbuhWbh5
> 
> The proposed new API (strscpy(), for "s"afe string copy) has an
> easy-to-use API for detecting buffer overflow, avoids unsafe truncation
> by default, and isn't subject to thread-safety attacks like the current
> strlcpy implementation.  See patch 2/3 for more on why strscpy() is a
> good thing.

+1 on the concept.

> To make strscpy() work more efficiently I did the minimum tweaking
> necessary to allow <asm/word-at-a-time.h> to work on all architectures,
> though of course individual maintainers can still make their versions
> more efficient as needed.
> 
> It's likely not necessary for per-architecture implementations of
> strscpy() to be written, but I stuck with the standard __HAVE_ARCH_XXX
> model just for consistency with the rest of <linux/string.h>.
> 
> I tested the implementation with a simple user-space harness, so I
> believe it is correct for the corner cases I could think of.  In
> particular I pairwise-tested all the unaligned values of source and
> dest, and tested the restriction on src page-crossing at all
> unaligned offsets approaching the page boundary.

Can you please put that in tools/testing/selftests and merge it as part of the
series? That way I can run the tests and be confident it works on powerpc.

cheers

Re: [PATCH 0/3] add new strscpy() API for string copy

[Chris Metcalf]

On 05/14/2015 07:10 PM, Michael Ellerman wrote:
> On Thu, 2015-04-30 at 12:01 -0400, Chris Metcalf wrote:
>> This patch series addresses limitations in strncpy() and strlcpy();
>> both the old APIs are unpleasant, as Linus nicely summarized here
>> a couple of days ago:
>>
>>    https://lkml.org/lkml/2015/4/28/570
>>
>> and of course as other folks (Greg K-H and Linus again) said last year:
>>
>>    https://plus.google.com/+gregkroahhartman/posts/1amLbuhWbh5
>>
>> The proposed new API (strscpy(), for "s"afe string copy) has an
>> easy-to-use API for detecting buffer overflow, avoids unsafe truncation
>> by default, and isn't subject to thread-safety attacks like the current
>> strlcpy implementation.  See patch 2/3 for more on why strscpy() is a
>> good thing.
> +1 on the concept.

Thanks.

>> To make strscpy() work more efficiently I did the minimum tweaking
>> necessary to allow <asm/word-at-a-time.h> to work on all architectures,
>> though of course individual maintainers can still make their versions
>> more efficient as needed.
>>
>> It's likely not necessary for per-architecture implementations of
>> strscpy() to be written, but I stuck with the standard __HAVE_ARCH_XXX
>> model just for consistency with the rest of <linux/string.h>.
>>
>> I tested the implementation with a simple user-space harness, so I
>> believe it is correct for the corner cases I could think of.  In
>> particular I pairwise-tested all the unaligned values of source and
>> dest, and tested the restriction on src page-crossing at all
>> unaligned offsets approaching the page boundary.
> Can you please put that in tools/testing/selftests and merge it as part of the
> series? That way I can run the tests and be confident it works on powerpc.

Unfortunately, the strscpy patch series only changes the one previous
user of the API, which is a tile-architecture-only driver piece, not
particularly useful for anyone else for testing.

The testing I did pulled strscpy() and word-at-a-time out into a
separate, standalone userspace implementation, and tested it there,
rather than doing tests through the syscall API like 
tools/testing/selftests.
So I don't really see a way of committing my test framework, other
than as a real Kconfig-enabled boot-time self-test or some such;
I can certainly do that but I don't know how excited people are to
have that additional level of source-code and Kconfig bloat.

-- 
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com

Re: [PATCH 0/3] add new strscpy() API for string copy

[Michael Ellerman]

On Fri, 2015-05-15 at 11:15 -0400, Chris Metcalf wrote:
> On 05/14/2015 07:10 PM, Michael Ellerman wrote:
> > On Thu, 2015-04-30 at 12:01 -0400, Chris Metcalf wrote:
> >>
> >> I tested the implementation with a simple user-space harness, so I
> >> believe it is correct for the corner cases I could think of.  In
> >> particular I pairwise-tested all the unaligned values of source and
> >> dest, and tested the restriction on src page-crossing at all
> >> unaligned offsets approaching the page boundary.
> > Can you please put that in tools/testing/selftests and merge it as part of the
> > series? That way I can run the tests and be confident it works on powerpc.
> 
> Unfortunately, the strscpy patch series only changes the one previous
> user of the API, which is a tile-architecture-only driver piece, not
> particularly useful for anyone else for testing.
> 
> The testing I did pulled strscpy() and word-at-a-time out into a
> separate, standalone userspace implementation, and tested it there,
> rather than doing tests through the syscall API like 
> tools/testing/selftests.

Not everything in selftests has to or does go through the syscall API.

We (powerpc) have tests of our memcpy/memcmp/load_unaligned_zeropad that are
built as standalone test programs.

Doing that for stuff in lib/string.c does look a bit complicated, because you'd
need to pull in a bunch of kernel headers.

Do you mind posting your test code somewhere so I can run it, and maybe I can
work out how to fold it into a selftest.

cheers

Re: [PATCH 0/3] add new strscpy() API for string copy

[Chris Metcalf]

On 05/17/2015 09:13 PM, Michael Ellerman wrote:
> On Fri, 2015-05-15 at 11:15 -0400, Chris Metcalf wrote:
>> On 05/14/2015 07:10 PM, Michael Ellerman wrote:
>>> On Thu, 2015-04-30 at 12:01 -0400, Chris Metcalf wrote:
>>>>
>>>> I tested the implementation with a simple user-space harness, so I
>>>> believe it is correct for the corner cases I could think of.  In
>>>> particular I pairwise-tested all the unaligned values of source and
>>>> dest, and tested the restriction on src page-crossing at all
>>>> unaligned offsets approaching the page boundary.
>>> Can you please put that in tools/testing/selftests and merge it as part of the
>>> series? That way I can run the tests and be confident it works on powerpc.
>>
>> Unfortunately, the strscpy patch series only changes the one previous
>> user of the API, which is a tile-architecture-only driver piece, not
>> particularly useful for anyone else for testing.
>>
>> The testing I did pulled strscpy() and word-at-a-time out into a
>> separate, standalone userspace implementation, and tested it there,
>> rather than doing tests through the syscall API like 
>> tools/testing/selftests.
>
> Not everything in selftests has to or does go through the syscall API.
>
> We (powerpc) have tests of our memcpy/memcmp/load_unaligned_zeropad that are
> built as standalone test programs.
>
> Doing that for stuff in lib/string.c does look a bit complicated, because you'd
> need to pull in a bunch of kernel headers.
>
> Do you mind posting your test code somewhere so I can run it, and maybe I can
> work out how to fold it into a selftest.

Seems easiest to just post it to LKML so anyone else who wants to can
take a look at it.  Trying to post a collection of files without
violating the "no MIME parts" rule for LKML made me dust off my
Usenet memories and track down a version of shar to use to bundle up
the test.c harness, the strscpy.c code (excerpted from lib/string.c with
suitable #defines and #includes so it builds in userspace), and a
version of word-at-a-time.h that works for the two environments I
tested it in (x86_64 and tile).
-- 
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com


#!/bin/sh
# This is a shell archive (produced by GNU sharutils 4.2.1).
# To extract the files from this archive, save it to some FILE, remove
# everything before the `!/bin/sh' line above, then type `sh FILE'.
#
# existing files WILL be overwritten
#
# This shar contains:
# length mode       name
# ------ ---------- ------------------------------------------
#   2294 -rw-r--r-- test.c
#   3321 -rw-r--r-- strscpy.c
#   2905 -rw-r--r-- word-at-a-time.h
#
echo=echo
if mkdir _sh15429; then
  $echo 'x -' 'creating lock directory'
else
  $echo 'failed to create lock directory'
  exit 1
fi
# ============= test.c ==============
  $echo 'x -' extracting 'test.c' '(text)'
  sed 's/^X//' << 'SHAR_EOF' > 'test.c' &&
/* Compile "gcc -O -o test test.c strscpy.c" and run "./test" */
X
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <unistd.h>
#include <errno.h>
#include <sys/types.h>
#include <sys/mman.h>
X
size_t strscpy_truncate(char *dest, const char *src, size_t count);
size_t strscpy(char *dest, const char *src, size_t count);
X
char dest[1024] __attribute__((aligned(8)));
char src[1024] __attribute__((aligned(8)));
X
#define assert(check) do {                                              \
X  if (!(check)) {                                                       \
X    printf("%s: %d: %d: %s @%zd: %d => %d\n", #check, i, j, s, count, rc, ret); \
X    exit(1);                                                            \
X  } } while (0)
X
void test(int i, int j, char *s, size_t count, int ret)
{
X  int rc;
X
X  if (ret == -1)
X    ret = strlen(s);
X
X  memset(dest, 0, sizeof(dest));
X  memset(src, 0, sizeof(src));
X  strcpy(&src[i], s);
X  rc = strscpy(&dest[j], &src[i], count);
X  assert(rc == ret);
X  if (rc > 0)
X    assert(strcmp(&dest[j], s) == 0);
X  else if (count > 0)
X    assert(dest[j] == '\0');
X
X  memset(dest, 0, sizeof(dest));
X  memset(src, 0, sizeof(src));
X  strcpy(&src[i], s);
X  rc = strscpy_truncate(&dest[j], &src[i], count);
X  assert(rc == ret);
X  if (rc > 0)
X    assert(strcmp(&dest[j], s) == 0);
X  else if (count > 0) {
X    assert(strncmp(&dest[j], s, count-1) == 0);
X    assert(dest[j+count-1] == '\0');
X  }
}
X
int main()
{
X  int i, j;
X
X  for (i = 0; i < 15; ++i) {
X    for (j = 0; j < 15; ++j) {
X      test(i, j, "Hello, world", sizeof(src) - i, -1);
X      test(i, j, "Hello, world", 12, -E2BIG);
X      test(i, j, "foo", 0, -E2BIG);
X      test(i, j, "foo", 1, -E2BIG);
X    }
X  }
X
X  /* Check we never walk across a page boundary past the source. */
X  char *p = mmap(NULL, 2*getpagesize(), PROT_READ|PROT_WRITE,
X                 MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);
X  mprotect(p+getpagesize(), getpagesize(), PROT_NONE);
X  char *base = p + getpagesize() - 16;
X  strcpy(base, "This is some ra");
X  for (i = 0; i < 15; ++i) {
X    memset(dest, 0, 16);
X    int rc = strscpy_truncate(dest, base + i, sizeof(dest));
X    if (rc != 16 - i - 1 || strcmp(dest, base + i) != 0) {
X      printf("Badness at %d %d\n", i, rc);
X      exit(1);
X    }
X  }
X
X  printf("OK!\n");
X  return 0;
}
SHAR_EOF
  chmod 0644 'test.c' ||
  $echo 'restore of' 'test.c' 'failed'
# ============= strscpy.c ==============
  $echo 'x -' extracting 'strscpy.c' '(text)'
  sed 's/^X//' << 'SHAR_EOF' > 'strscpy.c' &&
#include <sys/types.h>
#include <unistd.h>
#include <errno.h>
#include "word-at-a-time.h"
#ifndef __tile__
#define CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
#endif
#define PAGE_SIZE getpagesize()
#define EXPORT_SYMBOL(sym)
X
#ifndef __HAVE_ARCH_STRSCPY_TRUNCATE
/**
X * strscpy_truncate - Copy a C-string into a sized buffer
X * @dest: Where to copy the string to
X * @src: Where to copy the string from
X * @count: Size of destination buffer
X *
X * Copy the string, or as much of it as fits, into the dest buffer.
X * The routine returns the number of characters copied (not including
X * the trailing NUL) or -E2BIG if the destination buffer wasn't big enough.
X * The behavior is undefined if the string buffers overlap.
X *
X * Note that the implementation is robust to the string changing out
X * from underneath it, unlike the current strlcpy() implementation,
X * and it is easier to check overflow than with strlcpy()'s API.
X *
X * strscpy() is preferred over this function unless a truncated string
X * provides some valid semantics in the destination buffer.
X */
ssize_t strscpy_truncate(char *dest, const char *src, size_t count)
{
X	const struct word_at_a_time constants = WORD_AT_A_TIME_CONSTANTS;
X	size_t max = count;
X	long res = 0;
X
X	if (count == 0)
X		return -E2BIG;
X
#ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
X	/*
X	 * If src is unaligned, don't cross a page boundary,
X	 * since we don't know if the next page is mapped.
X	 */
X	if ((long)src & (sizeof(long) - 1)) {
X		size_t limit = PAGE_SIZE - ((long)src & (PAGE_SIZE - 1));
X		if (limit < max)
X			max = limit;
X	}
#else
X	/* If src or dest is unaligned, don't do word-at-a-time. */
X	if (((long) dest | (long) src) & (sizeof(long) - 1))
X		max = 0;
#endif
X
X	while (max >= sizeof(unsigned long)) {
X		unsigned long c, data;
X
X		c = *(unsigned long *)(src+res);
X		*(unsigned long *)(dest+res) = c;
X		if (has_zero(c, &data, &constants)) {
X			data = prep_zero_mask(c, data, &constants);
X			data = create_zero_mask(data);
X			return res + find_zero(data);
X		}
X		res += sizeof(unsigned long);
X		count -= sizeof(unsigned long);
X		max -= sizeof(unsigned long);
X	}
X
X	while (count) {
X		char c;
X
X		c = src[res];
X		dest[res] = c;
X		if (!c)
X			return res;
X		res++;
X		count--;
X	}
X
X	/* Hit buffer length without finding a NUL; force NUL-termination. */
X	if (res)
X		dest[res-1] = '\0';
X
X	return -E2BIG;
}
EXPORT_SYMBOL(strscpy_truncate);
#endif
X
#ifndef __HAVE_ARCH_STRSCPY
/**
X * strscpy - Copy a C-string into a sized buffer, but only if it fits
X * @dest: Where to copy the string to
X * @src: Where to copy the string from
X * @count: Size of destination buffer
X *
X * Copy the string into the dest buffer.  The routine returns the
X * number of characters copied (not including the trailing NUL) or
X * -E2BIG if the destination buffer wasn't big enough.  The behavior
X * is undefined if the string buffers overlap.  The destination buffer
X * is set to the empty string if the buffer is not big enough.
X *
X * Preferred over strlcpy() in all cases, and over strncpy() unless
X * the latter's zero-fill behavior is desired and truncation of the
X * source string is known not to be an issue.
X */
ssize_t strscpy(char *dest, const char *src, size_t count)
{
X	ssize_t res = strscpy_truncate(dest, src, count);
X	if (res < 0 && count != 0)
X		dest[0] = '\0';
X	return res;
}
EXPORT_SYMBOL(strscpy);
#endif
SHAR_EOF
  chmod 0644 'strscpy.c' ||
  $echo 'restore of' 'strscpy.c' 'failed'
# ============= word-at-a-time.h ==============
  $echo 'x -' extracting 'word-at-a-time.h' '(text)'
  sed 's/^X//' << 'SHAR_EOF' > 'word-at-a-time.h' &&
#ifndef _ASM_WORD_AT_A_TIME_H
#define _ASM_WORD_AT_A_TIME_H
X
#define REPEAT_BYTE(x)	((~0ul / 0xff) * (x))
X
#ifndef __tile__
#define IS_UNALIGNED(src, dst)	0
#else
#define IS_UNALIGNED(src, dst)	\
X	(((long) dst | (long) src) & (sizeof(long) - 1))
#endif
X
#ifdef __BIG_ENDIAN__
X
struct word_at_a_time {
X	const unsigned long high_bits, low_bits;
};
X
#define WORD_AT_A_TIME_CONSTANTS { REPEAT_BYTE(0xfe) + 1, REPEAT_BYTE(0x7f) }
X
/* Bit set in the bytes that have a zero */
static inline long prep_zero_mask(unsigned long val, unsigned long rhs, const struct word_at_a_time *c)
{
X	unsigned long mask = (val & c->low_bits) + c->low_bits;
X	return ~(mask | rhs);
}
X
#define create_zero_mask(mask) (mask)
X
static inline long find_zero(unsigned long mask)
{
X	long byte = 0;
#ifdef _LP64
X	if (mask >> 32)
X		mask >>= 32;
X	else
X		byte = 4;
#endif
X	if (mask >> 16)
X		mask >>= 16;
X	else
X		byte += 2;
X	return (mask >> 8) ? byte : byte + 1;
}
X
static inline bool has_zero(unsigned long val, unsigned long *data, const struct word_at_a_time *c)
{
X	unsigned long rhs = val | c->low_bits;
X	*data = rhs;
X	return (val + c->high_bits) & ~rhs;
}
X
#ifndef zero_bytemask
#define zero_bytemask(mask) (~1ul << __fls(mask))
#endif
X
#else
X
/*
X * The optimal byte mask counting is probably going to be something
X * that is architecture-specific. If you have a reliably fast
X * bit count instruction, that might be better than the multiply
X * and shift, for example.
X */
struct word_at_a_time {
X	const unsigned long one_bits, high_bits;
};
X
#define WORD_AT_A_TIME_CONSTANTS { REPEAT_BYTE(0x01), REPEAT_BYTE(0x80) }
X
#ifdef _LP64
X
/*
X * Jan Achrenius on G+: microoptimized version of
X * the simpler "(mask & ONEBYTES) * ONEBYTES >> 56"
X * that works for the bytemasks without having to
X * mask them first.
X */
static inline long count_masked_bytes(unsigned long mask)
{
X	return mask*0x0001020304050608ul >> 56;
}
X
#else	/* 32-bit case */
X
/* Carl Chatfield / Jan Achrenius G+ version for 32-bit */
static inline long count_masked_bytes(long mask)
{
X	/* (000000 0000ff 00ffff ffffff) -> ( 1 1 2 3 ) */
X	long a = (0x0ff0001+mask) >> 23;
X	/* Fix the 1 for 00 case */
X	return a & mask;
}
X
#endif
X
/* Return nonzero if it has a zero */
static inline unsigned long has_zero(unsigned long a, unsigned long *bits, const struct word_at_a_time *c)
{
X	unsigned long mask = ((a - c->one_bits) & ~a) & c->high_bits;
X	*bits = mask;
X	return mask;
}
X
static inline unsigned long prep_zero_mask(unsigned long a, unsigned long bits, const struct word_at_a_time *c)
{
X	return bits;
}
X
static inline unsigned long create_zero_mask(unsigned long bits)
{
X	bits = (bits - 1) & ~bits;
X	return bits >> 7;
}
X
/* The mask we created is directly usable as a bytemask */
#define zero_bytemask(mask) (mask)
X
static inline unsigned long find_zero(unsigned long mask)
{
X	return count_masked_bytes(mask);
}
X
#endif /* __BIG_ENDIAN */
X
#endif /* _ASM_WORD_AT_A_TIME_H */
SHAR_EOF
  chmod 0644 'word-at-a-time.h' ||
  $echo 'restore of' 'word-at-a-time.h' 'failed'
rm -fr _sh15429
exit 0

[GIT PULL] strscpy string copy function

[Chris Metcalf]

Linus,

Please pull the following changes for 4.2 from:

   git://git.kernel.org/pub/scm/linux/kernel/git/cmetcalf/linux-tile.git strscpy

The discussion on this died down on LKML [1], so either (a) everybody loved it and
it was obviously correct, or (b) everybody lost interest.  I'm optimistically
assuming (a), but willing to hear otherwise!

[1] https://lwn.net/Articles/643140/

Chris Metcalf (3):
       Make asm/word-at-a-time.h available on all architectures
       string: provide strscpy() and strscpy_truncate()
       tile: use global strscpy() rather than private copy

  arch/arc/include/asm/Kbuild          |   1 +
  arch/avr32/include/asm/Kbuild        |   1 +
  arch/blackfin/include/asm/Kbuild     |   1 +
  arch/c6x/include/asm/Kbuild          |   1 +
  arch/cris/include/asm/Kbuild         |   1 +
  arch/frv/include/asm/Kbuild          |   1 +
  arch/hexagon/include/asm/Kbuild      |   1 +
  arch/ia64/include/asm/Kbuild         |   1 +
  arch/m32r/include/asm/Kbuild         |   1 +
  arch/metag/include/asm/Kbuild        |   1 +
  arch/microblaze/include/asm/Kbuild   |   1 +
  arch/mips/include/asm/Kbuild         |   1 +
  arch/mn10300/include/asm/Kbuild      |   1 +
  arch/nios2/include/asm/Kbuild        |   1 +
  arch/powerpc/include/asm/Kbuild      |   1 +
  arch/s390/include/asm/Kbuild         |   1 +
  arch/score/include/asm/Kbuild        |   1 +
  arch/tile/gxio/mpipe.c               |  33 ++---------
  arch/tile/include/asm/Kbuild         |   1 +
  arch/um/include/asm/Kbuild           |   1 +
  arch/unicore32/include/asm/Kbuild    |   1 +
  arch/xtensa/include/asm/Kbuild       |   1 +
  include/asm-generic/word-at-a-time.h |  80 ++++++++++++++++++++++---
  include/linux/string.h               |   6 ++
  lib/string.c                         | 110 +++++++++++++++++++++++++++++++++++
  25 files changed, 213 insertions(+), 37 deletions(-)

-- 
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com

Re: [GIT PULL] strscpy string copy function

[Linus Torvalds]

On Tue, Jun 30, 2015 at 11:01 AM, Chris Metcalf <cmetcalf@ezchip.com> wrote:
> Linus,
>
> The discussion on this died down on LKML [1], so either (a) everybody loved
> it and
> it was obviously correct, or (b) everybody lost interest.  I'm
> optimistically
> assuming (a), but willing to hear otherwise!
>
> [1] https://lwn.net/Articles/643140/

Ugh. I thought we agreed to not have the odd "make it zero-sized"
thing be the default.

Let's just make something that is a sane version of strncpy/strlcpy,
not introduce yet another "str*cpy with really odd semantics for the
corner case"

              Linus

[PATCH v2 0/3] add new strscpy() API for string copy

[Chris Metcalf]

v2: eliminate strscpy_truncate(); the single strscpy() API in v2
  just tolerates a truncated string (while returning an error value)
  Also, improve some of the comparison language with strlcpy/strncpy.

This patch series addresses limitations in strncpy() and strlcpy();
both the old APIs are unpleasant, as Linus nicely summarized here:

  https://lkml.org/lkml/2015/4/28/570

and of course as other folks (Greg K-H and Linus again) said last year:

  https://plus.google.com/+gregkroahhartman/posts/1amLbuhWbh5

The proposed new API (strscpy(), for "s"afe string copy) has an
easy-to-use API for detecting buffer overflow, doesn't walk past the
requested copy size on the source string, and isn't subject to
thread-safety attacks like the current strlcpy implementation.  See
patch 2/3 for more on why strscpy() is a good thing.

To make strscpy() work more efficiently I did the minimum tweaking
necessary to allow <asm/word-at-a-time.h> to work on all architectures,
though of course individual maintainers can still make their versions
more efficient as needed.

It's likely not necessary for per-architecture implementations of
strscpy() to be written, but I stuck with the standard __HAVE_ARCH_XXX
model just for consistency with the rest of <linux/string.h>.

I tested the implementation with a simple user-space harness, so I
believe it is correct for the corner cases I could think of.  In
particular I pairwise-tested all the unaligned values of source and
dest, and tested the restriction on src page-crossing at all
unaligned offsets approaching the page boundary.

The patch series is available to be pulled from

  git://git.kernel.org/pub/scm/linux/kernel/git/cmetcalf/linux-tile.git strscpy

Chris Metcalf (3):
  Make asm/word-at-a-time.h available on all architectures
  string: provide strscpy()
  tile: use global strscpy() rather than private copy

 arch/arc/include/asm/Kbuild          |  1 +
 arch/avr32/include/asm/Kbuild        |  1 +
 arch/blackfin/include/asm/Kbuild     |  1 +
 arch/c6x/include/asm/Kbuild          |  1 +
 arch/cris/include/asm/Kbuild         |  1 +
 arch/frv/include/asm/Kbuild          |  1 +
 arch/hexagon/include/asm/Kbuild      |  1 +
 arch/ia64/include/asm/Kbuild         |  1 +
 arch/m32r/include/asm/Kbuild         |  1 +
 arch/metag/include/asm/Kbuild        |  1 +
 arch/microblaze/include/asm/Kbuild   |  1 +
 arch/mips/include/asm/Kbuild         |  1 +
 arch/mn10300/include/asm/Kbuild      |  1 +
 arch/nios2/include/asm/Kbuild        |  1 +
 arch/powerpc/include/asm/Kbuild      |  1 +
 arch/s390/include/asm/Kbuild         |  1 +
 arch/score/include/asm/Kbuild        |  1 +
 arch/tile/gxio/mpipe.c               | 33 ++------------
 arch/tile/include/asm/Kbuild         |  1 +
 arch/um/include/asm/Kbuild           |  1 +
 arch/unicore32/include/asm/Kbuild    |  1 +
 arch/xtensa/include/asm/Kbuild       |  1 +
 include/asm-generic/word-at-a-time.h | 80 ++++++++++++++++++++++++++++----
 include/linux/string.h               |  3 ++
 lib/string.c                         | 88 ++++++++++++++++++++++++++++++++++++
 25 files changed, 188 insertions(+), 37 deletions(-)

-- 
2.1.2

[PATCH v2 1/3] Make asm/word-at-a-time.h available on all architectures

[Chris Metcalf]

Added the x86 implementation of word-at-a-time to the
generic version, which previously only supported big-endian.

Omitted the x86-specific load_unaligned_zeropad(), which in
any case is also not present for the existing BE-only
implementation of a word-at-a-time, and is only used under
CONFIG_DCACHE_WORD_ACCESS.

Added as a "generic-y" to the Kbuilds of all architectures
that didn't previously have it.

Signed-off-by: Chris Metcalf <cmetcalf@ezchip.com>
---
 arch/arc/include/asm/Kbuild          |  1 +
 arch/avr32/include/asm/Kbuild        |  1 +
 arch/blackfin/include/asm/Kbuild     |  1 +
 arch/c6x/include/asm/Kbuild          |  1 +
 arch/cris/include/asm/Kbuild         |  1 +
 arch/frv/include/asm/Kbuild          |  1 +
 arch/hexagon/include/asm/Kbuild      |  1 +
 arch/ia64/include/asm/Kbuild         |  1 +
 arch/m32r/include/asm/Kbuild         |  1 +
 arch/metag/include/asm/Kbuild        |  1 +
 arch/microblaze/include/asm/Kbuild   |  1 +
 arch/mips/include/asm/Kbuild         |  1 +
 arch/mn10300/include/asm/Kbuild      |  1 +
 arch/nios2/include/asm/Kbuild        |  1 +
 arch/powerpc/include/asm/Kbuild      |  1 +
 arch/s390/include/asm/Kbuild         |  1 +
 arch/score/include/asm/Kbuild        |  1 +
 arch/tile/include/asm/Kbuild         |  1 +
 arch/um/include/asm/Kbuild           |  1 +
 arch/unicore32/include/asm/Kbuild    |  1 +
 arch/xtensa/include/asm/Kbuild       |  1 +
 include/asm-generic/word-at-a-time.h | 80 ++++++++++++++++++++++++++++++++----
 22 files changed, 93 insertions(+), 8 deletions(-)

diff --git a/arch/arc/include/asm/Kbuild b/arch/arc/include/asm/Kbuild
index be0c39e76f7c..be895ef03a51 100644
--- a/arch/arc/include/asm/Kbuild
+++ b/arch/arc/include/asm/Kbuild
@@ -49,4 +49,5 @@ generic-y += types.h
 generic-y += ucontext.h
 generic-y += user.h
 generic-y += vga.h
+generic-y += word-at-a-time.h
 generic-y += xor.h
diff --git a/arch/avr32/include/asm/Kbuild b/arch/avr32/include/asm/Kbuild
index 528d70d47a54..10dd6e8470bd 100644
--- a/arch/avr32/include/asm/Kbuild
+++ b/arch/avr32/include/asm/Kbuild
@@ -20,4 +20,5 @@ generic-y += sections.h
 generic-y += topology.h
 generic-y += trace_clock.h
 generic-y += vga.h
+generic-y += word-at-a-time.h
 generic-y += xor.h
diff --git a/arch/blackfin/include/asm/Kbuild b/arch/blackfin/include/asm/Kbuild
index 4bd3c3cfc9ab..9705a527b7b0 100644
--- a/arch/blackfin/include/asm/Kbuild
+++ b/arch/blackfin/include/asm/Kbuild
@@ -46,4 +46,5 @@ generic-y += types.h
 generic-y += ucontext.h
 generic-y += unaligned.h
 generic-y += user.h
+generic-y += word-at-a-time.h
 generic-y += xor.h
diff --git a/arch/c6x/include/asm/Kbuild b/arch/c6x/include/asm/Kbuild
index ae0a51f5376c..535efdb0692d 100644
--- a/arch/c6x/include/asm/Kbuild
+++ b/arch/c6x/include/asm/Kbuild
@@ -59,4 +59,5 @@ generic-y += types.h
 generic-y += ucontext.h
 generic-y += user.h
 generic-y += vga.h
+generic-y += word-at-a-time.h
 generic-y += xor.h
diff --git a/arch/cris/include/asm/Kbuild b/arch/cris/include/asm/Kbuild
index 057e51859b0a..4991539a25f6 100644
--- a/arch/cris/include/asm/Kbuild
+++ b/arch/cris/include/asm/Kbuild
@@ -26,4 +26,5 @@ generic-y += sections.h
 generic-y += topology.h
 generic-y += trace_clock.h
 generic-y += vga.h
+generic-y += word-at-a-time.h
 generic-y += xor.h
diff --git a/arch/frv/include/asm/Kbuild b/arch/frv/include/asm/Kbuild
index e3f81b53578e..ca34d1cf2cdf 100644
--- a/arch/frv/include/asm/Kbuild
+++ b/arch/frv/include/asm/Kbuild
@@ -7,3 +7,4 @@ generic-y += mcs_spinlock.h
 generic-y += preempt.h
 generic-y += scatterlist.h
 generic-y += trace_clock.h
+generic-y += word-at-a-time.h
diff --git a/arch/hexagon/include/asm/Kbuild b/arch/hexagon/include/asm/Kbuild
index c7a99f860b40..872920fba0a6 100644
--- a/arch/hexagon/include/asm/Kbuild
+++ b/arch/hexagon/include/asm/Kbuild
@@ -58,4 +58,5 @@ generic-y += types.h
 generic-y += ucontext.h
 generic-y += unaligned.h
 generic-y += vga.h
+generic-y += word-at-a-time.h
 generic-y += xor.h
diff --git a/arch/ia64/include/asm/Kbuild b/arch/ia64/include/asm/Kbuild
index 9b41b4bcc073..614579fa1e49 100644
--- a/arch/ia64/include/asm/Kbuild
+++ b/arch/ia64/include/asm/Kbuild
@@ -8,3 +8,4 @@ generic-y += preempt.h
 generic-y += scatterlist.h
 generic-y += trace_clock.h
 generic-y += vtime.h
+generic-y += word-at-a-time.h
diff --git a/arch/m32r/include/asm/Kbuild b/arch/m32r/include/asm/Kbuild
index 2edc793372fc..36f850dd4a1e 100644
--- a/arch/m32r/include/asm/Kbuild
+++ b/arch/m32r/include/asm/Kbuild
@@ -9,3 +9,4 @@ generic-y += preempt.h
 generic-y += scatterlist.h
 generic-y += sections.h
 generic-y += trace_clock.h
+generic-y += word-at-a-time.h
diff --git a/arch/metag/include/asm/Kbuild b/arch/metag/include/asm/Kbuild
index 0bf5d525b945..a291b3b2529e 100644
--- a/arch/metag/include/asm/Kbuild
+++ b/arch/metag/include/asm/Kbuild
@@ -54,4 +54,5 @@ generic-y += ucontext.h
 generic-y += unaligned.h
 generic-y += user.h
 generic-y += vga.h
+generic-y += word-at-a-time.h
 generic-y += xor.h
diff --git a/arch/microblaze/include/asm/Kbuild b/arch/microblaze/include/asm/Kbuild
index ab564a6db5c3..966f221b2d67 100644
--- a/arch/microblaze/include/asm/Kbuild
+++ b/arch/microblaze/include/asm/Kbuild
@@ -10,3 +10,4 @@ generic-y += preempt.h
 generic-y += scatterlist.h
 generic-y += syscalls.h
 generic-y += trace_clock.h
+generic-y += word-at-a-time.h
diff --git a/arch/mips/include/asm/Kbuild b/arch/mips/include/asm/Kbuild
index 526539cbc99f..c3cfb83aca1d 100644
--- a/arch/mips/include/asm/Kbuild
+++ b/arch/mips/include/asm/Kbuild
@@ -18,4 +18,5 @@ generic-y += serial.h
 generic-y += trace_clock.h
 generic-y += ucontext.h
 generic-y += user.h
+generic-y += word-at-a-time.h
 generic-y += xor.h
diff --git a/arch/mn10300/include/asm/Kbuild b/arch/mn10300/include/asm/Kbuild
index f892d9de47d9..30a06e2d6e42 100644
--- a/arch/mn10300/include/asm/Kbuild
+++ b/arch/mn10300/include/asm/Kbuild
@@ -9,3 +9,4 @@ generic-y += preempt.h
 generic-y += scatterlist.h
 generic-y += sections.h
 generic-y += trace_clock.h
+generic-y += word-at-a-time.h
diff --git a/arch/nios2/include/asm/Kbuild b/arch/nios2/include/asm/Kbuild
index 24b3d8999ac7..bc87081c0669 100644
--- a/arch/nios2/include/asm/Kbuild
+++ b/arch/nios2/include/asm/Kbuild
@@ -61,4 +61,5 @@ generic-y += types.h
 generic-y += unaligned.h
 generic-y += user.h
 generic-y += vga.h
+generic-y += word-at-a-time.h
 generic-y += xor.h
diff --git a/arch/powerpc/include/asm/Kbuild b/arch/powerpc/include/asm/Kbuild
index 4b87205c230c..dfc61a151890 100644
--- a/arch/powerpc/include/asm/Kbuild
+++ b/arch/powerpc/include/asm/Kbuild
@@ -9,3 +9,4 @@ generic-y += rwsem.h
 generic-y += scatterlist.h
 generic-y += trace_clock.h
 generic-y += vtime.h
+generic-y += word-at-a-time.h
diff --git a/arch/s390/include/asm/Kbuild b/arch/s390/include/asm/Kbuild
index c631f98fd524..27c37b3d1dad 100644
--- a/arch/s390/include/asm/Kbuild
+++ b/arch/s390/include/asm/Kbuild
@@ -6,3 +6,4 @@ generic-y += mcs_spinlock.h
 generic-y += preempt.h
 generic-y += scatterlist.h
 generic-y += trace_clock.h
+generic-y += word-at-a-time.h
diff --git a/arch/score/include/asm/Kbuild b/arch/score/include/asm/Kbuild
index 83ed116d414c..16958f529aa7 100644
--- a/arch/score/include/asm/Kbuild
+++ b/arch/score/include/asm/Kbuild
@@ -13,3 +13,4 @@ generic-y += sections.h
 generic-y += trace_clock.h
 generic-y += xor.h
 generic-y += serial.h
+generic-y += word-at-a-time.h
diff --git a/arch/tile/include/asm/Kbuild b/arch/tile/include/asm/Kbuild
index f5433e0e34e0..0804122e579a 100644
--- a/arch/tile/include/asm/Kbuild
+++ b/arch/tile/include/asm/Kbuild
@@ -39,4 +39,5 @@ generic-y += termbits.h
 generic-y += termios.h
 generic-y += trace_clock.h
 generic-y += types.h
+generic-y += word-at-a-time.h
 generic-y += xor.h
diff --git a/arch/um/include/asm/Kbuild b/arch/um/include/asm/Kbuild
index 9176fa11d49b..ee02364d8922 100644
--- a/arch/um/include/asm/Kbuild
+++ b/arch/um/include/asm/Kbuild
@@ -26,4 +26,5 @@ generic-y += sections.h
 generic-y += switch_to.h
 generic-y += topology.h
 generic-y += trace_clock.h
+generic-y += word-at-a-time.h
 generic-y += xor.h
diff --git a/arch/unicore32/include/asm/Kbuild b/arch/unicore32/include/asm/Kbuild
index 3e0c19d0f4c5..f456bfa1dd54 100644
--- a/arch/unicore32/include/asm/Kbuild
+++ b/arch/unicore32/include/asm/Kbuild
@@ -62,4 +62,5 @@ generic-y += ucontext.h
 generic-y += unaligned.h
 generic-y += user.h
 generic-y += vga.h
+generic-y += word-at-a-time.h
 generic-y += xor.h
diff --git a/arch/xtensa/include/asm/Kbuild b/arch/xtensa/include/asm/Kbuild
index 86a9ab2e2ca9..6ac0a9ab3ea3 100644
--- a/arch/xtensa/include/asm/Kbuild
+++ b/arch/xtensa/include/asm/Kbuild
@@ -29,4 +29,5 @@ generic-y += statfs.h
 generic-y += termios.h
 generic-y += topology.h
 generic-y += trace_clock.h
+generic-y += word-at-a-time.h
 generic-y += xor.h
diff --git a/include/asm-generic/word-at-a-time.h b/include/asm-generic/word-at-a-time.h
index 94f9ea8abcae..011dde083f23 100644
--- a/include/asm-generic/word-at-a-time.h
+++ b/include/asm-generic/word-at-a-time.h
@@ -1,15 +1,10 @@
 #ifndef _ASM_WORD_AT_A_TIME_H
 #define _ASM_WORD_AT_A_TIME_H
 
-/*
- * This says "generic", but it's actually big-endian only.
- * Little-endian can use more efficient versions of these
- * interfaces, see for example
- *	 arch/x86/include/asm/word-at-a-time.h
- * for those.
- */
-
 #include <linux/kernel.h>
+#include <asm/byteorder.h>
+
+#ifdef __BIG_ENDIAN
 
 struct word_at_a_time {
 	const unsigned long high_bits, low_bits;
@@ -53,4 +48,73 @@ static inline bool has_zero(unsigned long val, unsigned long *data, const struct
 #define zero_bytemask(mask) (~1ul << __fls(mask))
 #endif
 
+#else
+
+/*
+ * The optimal byte mask counting is probably going to be something
+ * that is architecture-specific. If you have a reliably fast
+ * bit count instruction, that might be better than the multiply
+ * and shift, for example.
+ */
+struct word_at_a_time {
+	const unsigned long one_bits, high_bits;
+};
+
+#define WORD_AT_A_TIME_CONSTANTS { REPEAT_BYTE(0x01), REPEAT_BYTE(0x80) }
+
+#ifdef CONFIG_64BIT
+
+/*
+ * Jan Achrenius on G+: microoptimized version of
+ * the simpler "(mask & ONEBYTES) * ONEBYTES >> 56"
+ * that works for the bytemasks without having to
+ * mask them first.
+ */
+static inline long count_masked_bytes(unsigned long mask)
+{
+	return mask*0x0001020304050608ul >> 56;
+}
+
+#else	/* 32-bit case */
+
+/* Carl Chatfield / Jan Achrenius G+ version for 32-bit */
+static inline long count_masked_bytes(long mask)
+{
+	/* (000000 0000ff 00ffff ffffff) -> ( 1 1 2 3 ) */
+	long a = (0x0ff0001+mask) >> 23;
+	/* Fix the 1 for 00 case */
+	return a & mask;
+}
+
+#endif
+
+/* Return nonzero if it has a zero */
+static inline unsigned long has_zero(unsigned long a, unsigned long *bits, const struct word_at_a_time *c)
+{
+	unsigned long mask = ((a - c->one_bits) & ~a) & c->high_bits;
+	*bits = mask;
+	return mask;
+}
+
+static inline unsigned long prep_zero_mask(unsigned long a, unsigned long bits, const struct word_at_a_time *c)
+{
+	return bits;
+}
+
+static inline unsigned long create_zero_mask(unsigned long bits)
+{
+	bits = (bits - 1) & ~bits;
+	return bits >> 7;
+}
+
+/* The mask we created is directly usable as a bytemask */
+#define zero_bytemask(mask) (mask)
+
+static inline unsigned long find_zero(unsigned long mask)
+{
+	return count_masked_bytes(mask);
+}
+
+#endif /* __BIG_ENDIAN */
+
 #endif /* _ASM_WORD_AT_A_TIME_H */
-- 
2.1.2

[PATCH v2 2/3] string: provide strscpy()

[Chris Metcalf]

The strscpy() API is intended to be used instead of strlcpy(),
and instead of most uses of strncpy().

- Unlike strlcpy(), it doesn't read from memory beyond (src + size).

- Unlike strlcpy() or strncpy(), the API provides an easy way to check
  for destination buffer overflow: an -E2BIG error return value.

- The provided implementation is robust in the face of the source
  buffer being asynchronously changed during the copy, unlike the
  current implementation of strlcpy().

- Unlike strncpy(), the destination buffer will be NUL-terminated
  if the string in the source buffer is too long.

- Also unlike strncpy(), the destination buffer will not be updated
  beyond the NUL termination, avoiding strncpy's behavior of zeroing
  the entire tail end of the destination buffer.  (A memset() after
  the strscpy() can be used if this behavior is desired.)

- The implementation should be reasonably performant on all
  platforms since it uses the asm/word-at-a-time.h API rather than
  simple byte copy.  Kernel-to-kernel string copy is not considered
  to be performance critical in any case.

Signed-off-by: Chris Metcalf <cmetcalf@ezchip.com>
---
 include/linux/string.h |  3 ++
 lib/string.c           | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 91 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index e40099e585c9..213274a0d356 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -25,6 +25,9 @@ extern char * strncpy(char *,const char *, __kernel_size_t);
 #ifndef __HAVE_ARCH_STRLCPY
 size_t strlcpy(char *, const char *, size_t);
 #endif
+#ifndef __HAVE_ARCH_STRSCPY
+ssize_t __must_check strscpy(char *, const char *, size_t);
+#endif
 #ifndef __HAVE_ARCH_STRCAT
 extern char * strcat(char *, const char *);
 #endif
diff --git a/lib/string.c b/lib/string.c
index a5792019193c..0ff54eb78f52 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -27,6 +27,10 @@
 #include <linux/bug.h>
 #include <linux/errno.h>
 
+#include <asm/byteorder.h>
+#include <asm/word-at-a-time.h>
+#include <asm/page.h>
+
 #ifndef __HAVE_ARCH_STRNCASECMP
 /**
  * strncasecmp - Case insensitive, length-limited string comparison
@@ -146,6 +150,90 @@ size_t strlcpy(char *dest, const char *src, size_t size)
 EXPORT_SYMBOL(strlcpy);
 #endif
 
+#ifndef __HAVE_ARCH_STRSCPY
+/**
+ * strscpy - Copy a C-string into a sized buffer
+ * @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 number of characters copied (not including
+ * the trailing NUL) or -E2BIG if the destination buffer wasn't big enough.
+ * The behavior is undefined if the string buffers overlap.
+ * If the destination buffer isn't big enough, it is NUL terminated.
+ *
+ * Preferred to strlcpy() since the API doesn't require reading memory
+ * from the src string beyond the specified "count" bytes, and since
+ * the return value is easier to error-check than strlcpy()'s.
+ * In addition, the implementation is robust to the string changing out
+ * from underneath it, unlike the current strlcpy() implementation.
+ *
+ * Preferred to strncpy() since it always returns a valid string, and
+ * doesn't unnecessarily force the tail of the destination buffer to be
+ * zeroed.  If the zeroing is desired, it's likely cleaner to use strscpy()
+ * with an overflow test, then just memset() the tail of the dest buffer.
+ */
+ssize_t strscpy(char *dest, const char *src, size_t count)
+{
+	const struct word_at_a_time constants = WORD_AT_A_TIME_CONSTANTS;
+	size_t max = count;
+	long res = 0;
+
+	if (count == 0)
+		return -E2BIG;
+
+#ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
+	/*
+	 * If src is unaligned, don't cross a page boundary,
+	 * since we don't know if the next page is mapped.
+	 */
+	if ((long)src & (sizeof(long) - 1)) {
+		size_t limit = PAGE_SIZE - ((long)src & (PAGE_SIZE - 1));
+		if (limit < max)
+			max = limit;
+	}
+#else
+	/* If src or dest is unaligned, don't do word-at-a-time. */
+	if (((long) dest | (long) src) & (sizeof(long) - 1))
+		max = 0;
+#endif
+
+	while (max >= sizeof(unsigned long)) {
+		unsigned long c, data;
+
+		c = *(unsigned long *)(src+res);
+		*(unsigned long *)(dest+res) = c;
+		if (has_zero(c, &data, &constants)) {
+			data = prep_zero_mask(c, data, &constants);
+			data = create_zero_mask(data);
+			return res + find_zero(data);
+		}
+		res += sizeof(unsigned long);
+		count -= sizeof(unsigned long);
+		max -= sizeof(unsigned long);
+	}
+
+	while (count) {
+		char c;
+
+		c = src[res];
+		dest[res] = c;
+		if (!c)
+			return res;
+		res++;
+		count--;
+	}
+
+	/* Hit buffer length without finding a NUL; force NUL-termination. */
+	if (res)
+		dest[res-1] = '\0';
+
+	return -E2BIG;
+}
+EXPORT_SYMBOL(strscpy);
+#endif
+
 #ifndef __HAVE_ARCH_STRCAT
 /**
  * strcat - Append one %NUL-terminated string to another
-- 
2.1.2

Re: [PATCH v2 2/3] string: provide strscpy()

[Geert Uytterhoeven]

Hi Chris,

On Wed, Jul 8, 2015 at 10:20 PM, Chris Metcalf <cmetcalf@ezchip.com> wrote:
> + * strscpy - Copy a C-string into a sized buffer
> + * @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 number of characters copied (not including
> + * the trailing NUL) or -E2BIG if the destination buffer wasn't big enough.
> + * The behavior is undefined if the string buffers overlap.
> + * If the destination buffer isn't big enough, it is NUL terminated.

... unless count is zero

and it's also NUL terminated if it is big enough ;-)

Perhaps
"The destination buffer is always NUL terminated, unless it's zero-sized."

> + *
> + * Preferred to strlcpy() since the API doesn't require reading memory
> + * from the src string beyond the specified "count" bytes, and since
> + * the return value is easier to error-check than strlcpy()'s.
> + * In addition, the implementation is robust to the string changing out
> + * from underneath it, unlike the current strlcpy() implementation.
> + *
> + * Preferred to strncpy() since it always returns a valid string, and
> + * doesn't unnecessarily force the tail of the destination buffer to be
> + * zeroed.  If the zeroing is desired, it's likely cleaner to use strscpy()
> + * with an overflow test, then just memset() the tail of the dest buffer.
> + */
> +ssize_t strscpy(char *dest, const char *src, size_t count)
> +{
> +       const struct word_at_a_time constants = WORD_AT_A_TIME_CONSTANTS;
> +       size_t max = count;
> +       long res = 0;
> +
> +       if (count == 0)
> +               return -E2BIG;

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

[PATCH v2 3/3] tile: use global strscpy() rather than private copy

[Chris Metcalf]

Now that strscpy() is a standard API, remove the local copy.

Signed-off-by: Chris Metcalf <cmetcalf@ezchip.com>
---
 arch/tile/gxio/mpipe.c | 33 ++++-----------------------------
 1 file changed, 4 insertions(+), 29 deletions(-)

diff --git a/arch/tile/gxio/mpipe.c b/arch/tile/gxio/mpipe.c
index ee186e13dfe6..f102048d9c0e 100644
--- a/arch/tile/gxio/mpipe.c
+++ b/arch/tile/gxio/mpipe.c
@@ -19,6 +19,7 @@
 #include <linux/errno.h>
 #include <linux/io.h>
 #include <linux/module.h>
+#include <linux/string.h>
 
 #include <gxio/iorpc_globals.h>
 #include <gxio/iorpc_mpipe.h>
@@ -29,32 +30,6 @@
 /* HACK: Avoid pointless "shadow" warnings. */
 #define link link_shadow
 
-/**
- * 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
- * @size: 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 size is non-zero) when the
- * buffer is not big enough.
- */
-static size_t strscpy(char *dest, const char *src, size_t size)
-{
-	size_t len = strnlen(src, size) + 1;
-	if (len > size) {
-		if (size)
-			dest[0] = '\0';
-		return 0;
-	}
-	memcpy(dest, src, len);
-	return len;
-}
-
 int gxio_mpipe_init(gxio_mpipe_context_t *context, unsigned int mpipe_index)
 {
 	char file[32];
@@ -540,7 +515,7 @@ int gxio_mpipe_link_instance(const char *link_name)
 	if (!context)
 		return GXIO_ERR_NO_DEVICE;
 
-	if (strscpy(name.name, link_name, sizeof(name.name)) == 0)
+	if (strscpy(name.name, link_name, sizeof(name.name)) < 0)
 		return GXIO_ERR_NO_DEVICE;
 
 	return gxio_mpipe_info_instance_aux(context, name);
@@ -559,7 +534,7 @@ int gxio_mpipe_link_enumerate_mac(int idx, char *link_name, uint8_t *link_mac)
 
 	rv = gxio_mpipe_info_enumerate_aux(context, idx, &name, &mac);
 	if (rv >= 0) {
-		if (strscpy(link_name, name.name, sizeof(name.name)) == 0)
+		if (strscpy(link_name, name.name, sizeof(name.name)) < 0)
 			return GXIO_ERR_INVAL_MEMORY_SIZE;
 		memcpy(link_mac, mac.mac, sizeof(mac.mac));
 	}
@@ -576,7 +551,7 @@ int gxio_mpipe_link_open(gxio_mpipe_link_t *link,
 	_gxio_mpipe_link_name_t name;
 	int rv;
 
-	if (strscpy(name.name, link_name, sizeof(name.name)) == 0)
+	if (strscpy(name.name, link_name, sizeof(name.name)) < 0)
 		return GXIO_ERR_NO_DEVICE;
 
 	rv = gxio_mpipe_link_open_aux(context, name, flags);
-- 
2.1.2