[PATCH] lib: string.c: Added a funktion function strzcpy

[PATCH] lib: string.c: Added a funktion function strzcpy

[Rickard Strandqvist]

I've added a feature strzcpy to make it easier to not do wrong
when you use the function strncpy.
By not have to add a guaranteed trailing null character:
  strncpy(to, src, c);
  to[c - 1] = '\0';


This arose after a discussion with Dan Carpenter:

https://lkml.org/lkml/2014/9/15/74

And I was thinking of making it with the return value corresponding
to strlen, but could not make any neat/optimal solution.
But if we are to add this maybe it's worth thinking about whether
we can use the return value to something sensible.


Rickard Strandqvist (1):
  lib: string.c:  Added a funktion function strzcpy

 include/linux/string.h |    1 +
 lib/string.c           |   31 +++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

-- 
1.7.10.4

[PATCH] lib: string.c: Added a funktion function strzcpy

[Rickard Strandqvist]

Added a function strzcpy which works the same as strncpy,
but guaranteed to produce the trailing null character.

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 include/linux/string.h |    1 +
 lib/string.c           |   31 +++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index d36977e..d789ee5e 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -22,6 +22,7 @@ extern char * strcpy(char *,const char *);
 #ifndef __HAVE_ARCH_STRNCPY
 extern char * strncpy(char *,const char *, __kernel_size_t);
 #endif
+extern char *strzcpy(char *, const char *, __kernel_size_t);
 #ifndef __HAVE_ARCH_STRLCPY
 size_t strlcpy(char *, const char *, size_t);
 #endif
diff --git a/lib/string.c b/lib/string.c
index f3c6ff5..582a832 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -134,6 +134,37 @@ char *strncpy(char *dest, const char *src, size_t count)
 EXPORT_SYMBOL(strncpy);
 #endif
 
+/**
+ * strzcpy - Copy a length-limited, C-string
+ * @dest: Where to copy the string to
+ * @src: Where to copy the string from
+ * @count: The maximum number of bytes to copy
+ *
+ * The result is %NUL-terminated,
+ * as long as count is greater than zero.
+ *
+ * In the case where the length of @src is less than  that  of
+ * count, the remainder of @dest will be padded with %NUL.
+ *
+ */
+char *strzcpy(char *dest, const char *src, size_t count)
+{
+	char *tmp = dest;
+
+	while (count) {
+		if ((*tmp = *src) != 0)
+			src++;
+		tmp++;
+		count--;
+	}
+
+	if (dest != tmp)
+		*--tmp = '\0';
+
+	return dest;
+}
+EXPORT_SYMBOL(strzcpy);
+
 #ifndef __HAVE_ARCH_STRLCPY
 /**
  * strlcpy - Copy a C-string into a sized buffer
-- 
1.7.10.4

Re: [PATCH] lib: string.c: Added a funktion function strzcpy

[Andrew Morton]

On Wed, 24 Sep 2014 00:13:36 +0200 Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se> wrote:

> +/**
> + * strzcpy - Copy a length-limited, C-string
> + * @dest: Where to copy the string to
> + * @src: Where to copy the string from
> + * @count: The maximum number of bytes to copy
> + *
> + * The result is %NUL-terminated,
> + * as long as count is greater than zero.
> + *
> + * In the case where the length of @src is less than  that  of
> + * count, the remainder of @dest will be padded with %NUL.

As far as I can tell, this sentence describes the only difference
between strzcpy() and strlcpy().  Only the sentence is wrong - the
implementation doesn't actually do that.

> + */
> +char *strzcpy(char *dest, const char *src, size_t count)
> +{
> +	char *tmp = dest;
> +
> +	while (count) {
> +		if ((*tmp = *src) != 0)
> +			src++;
> +		tmp++;
> +		count--;
> +	}
> +
> +	if (dest != tmp)
> +		*--tmp = '\0';
> +
> +	return dest;
> +}
> +EXPORT_SYMBOL(strzcpy);

Re: [PATCH] lib: string.c: Added a funktion function strzcpy

[Andi Kleen]

On Wed, Sep 24, 2014 at 12:13:36AM +0200, Rickard Strandqvist wrote:
> Added a function strzcpy which works the same as strncpy,
> but guaranteed to produce the trailing null character.

Do we really need the bizarre strncpy padding semantics for anything? 
Why not just use strlcpy?

-Andi

Re: [PATCH] lib: string.c: Added a funktion function strzcpy

[Dan Carpenter]

On Tue, Sep 23, 2014 at 06:17:53PM -0700, Andi Kleen wrote:
> On Wed, Sep 24, 2014 at 12:13:36AM +0200, Rickard Strandqvist wrote:
> > Added a function strzcpy which works the same as strncpy,
> > but guaranteed to produce the trailing null character.
> 
> Do we really need the bizarre strncpy padding semantics for anything?
> Why not just use strlcpy?

We do need the padding in many places to prevent information leaks.

So we end up open coding the last NUL after so many of the strncpy()
calls.  And we're adding more NUL terminators all over the place now
just to make the code easier to audit.

regards,
dan carpenter

Re: [PATCH] lib: string.c: Added a funktion function strzcpy

[Lennart Sorensen]

On Wed, Sep 24, 2014 at 10:52:06AM +0300, Dan Carpenter wrote:
> On Tue, Sep 23, 2014 at 06:17:53PM -0700, Andi Kleen wrote:
> > Do we really need the bizarre strncpy padding semantics for anything?
> > Why not just use strlcpy?
> 
> We do need the padding in many places to prevent information leaks.
> 
> So we end up open coding the last NUL after so many of the strncpy()
> calls.  And we're adding more NUL terminators all over the place now
> just to make the code easier to audit.

I think the suggestion to use strlcpy makes sense too.  strlcpy seems
to do exactly what is needed and has a known behaviour already.

-- 
Len Sorensen

Re: [PATCH] lib: string.c: Added a funktion function strzcpy

[Andi Kleen]

On Wed, Sep 24, 2014 at 10:52:06AM +0300, Dan Carpenter wrote:
> On Tue, Sep 23, 2014 at 06:17:53PM -0700, Andi Kleen wrote:
> > On Wed, Sep 24, 2014 at 12:13:36AM +0200, Rickard Strandqvist wrote:
> > > Added a function strzcpy which works the same as strncpy,
> > > but guaranteed to produce the trailing null character.
> > 
> > Do we really need the bizarre strncpy padding semantics for anything?
> > Why not just use strlcpy?
> 
> We do need the padding in many places to prevent information leaks.

Like where?

Usually explicit memset is far cleaner/safer.

-andi
-- 
ak@linux.intel.com -- Speaking for myself only

Re: [PATCH] lib: string.c: Added a funktion function strzcpy

[Andi Kleen]

> I can show a large amount of patches that show that many people do not
> seem aware that strncpy does not guarantee a terminating null character.
> Add to that all the code I've seen like this:

Yes that's understood.

The question was: where do we rely on the padding semantics of strncpy?

-Andi

Re: [PATCH] lib: string.c: Added a funktion function strzcpy

[Dan Carpenter]

On Wed, Sep 24, 2014 at 07:35:55AM -0700, Andi Kleen wrote:
> On Wed, Sep 24, 2014 at 10:52:06AM +0300, Dan Carpenter wrote:
> > On Tue, Sep 23, 2014 at 06:17:53PM -0700, Andi Kleen wrote:
> > > On Wed, Sep 24, 2014 at 12:13:36AM +0200, Rickard Strandqvist wrote:
> > > > Added a function strzcpy which works the same as strncpy,
> > > > but guaranteed to produce the trailing null character.
> > > 
> > > Do we really need the bizarre strncpy padding semantics for anything?
> > > Why not just use strlcpy?
> > 
> > We do need the padding in many places to prevent information leaks.
> 
> Like where?
> 

You're asking what would break if we switched every strncpy() to
strlcpy() but it's not an easy question to answer.

I've looked a lot at information leaks, but strings are still a blind
spot for my Smatch.  My check only looks at normal variables, arrays.
Eventually I hope to fix this, of course.

I did a git search and Rickard has added some examples, but there were
definitely other places that rely strncpy() padding before.

regards,
dan carpenter

Re: [PATCH] lib: string.c: Added a funktion function strzcpy

[Rickard Strandqvist]

2014-09-24 17:41 GMT+02:00 Dan Carpenter <dan.carpenter@oracle.com>:
> On Wed, Sep 24, 2014 at 07:35:55AM -0700, Andi Kleen wrote:
>> On Wed, Sep 24, 2014 at 10:52:06AM +0300, Dan Carpenter wrote:
>> > On Tue, Sep 23, 2014 at 06:17:53PM -0700, Andi Kleen wrote:
>> > > On Wed, Sep 24, 2014 at 12:13:36AM +0200, Rickard Strandqvist wrote:
>> > > > Added a function strzcpy which works the same as strncpy,
>> > > > but guaranteed to produce the trailing null character.
>> > >
>> > > Do we really need the bizarre strncpy padding semantics for anything?
>> > > Why not just use strlcpy?
>> >
>> > We do need the padding in many places to prevent information leaks.
>>
>> Like where?
>>
>
> You're asking what would break if we switched every strncpy() to
> strlcpy() but it's not an easy question to answer.
>
> I've looked a lot at information leaks, but strings are still a blind
> spot for my Smatch.  My check only looks at normal variables, arrays.
> Eventually I hope to fix this, of course.
>
> I did a git search and Rickard has added some examples, but there were
> definitely other places that rely strncpy() padding before.
>
> regards,
> dan carpenter


Hi

If you want to see examples of this type of error, you can check the
patches I've done over the past two months.
So in linux-next

git log --patch-with-stat

And search for my email address.

And there is also an approximately 20 more patches I waited to see if
I will be able to use a potential strzcpy  :-)


Kind regards
Rickard Strandqvist

Re: [PATCH] lib: string.c: Added a funktion function strzcpy

[Andrew Morton]

On Wed, 24 Sep 2014 22:51:14 +0200 Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se> wrote:

> 2014-09-24 17:41 GMT+02:00 Dan Carpenter <dan.carpenter@oracle.com>:
> > On Wed, Sep 24, 2014 at 07:35:55AM -0700, Andi Kleen wrote:
> >> On Wed, Sep 24, 2014 at 10:52:06AM +0300, Dan Carpenter wrote:
> >> > On Tue, Sep 23, 2014 at 06:17:53PM -0700, Andi Kleen wrote:
> >> > > On Wed, Sep 24, 2014 at 12:13:36AM +0200, Rickard Strandqvist wrote:
> >> > > > Added a function strzcpy which works the same as strncpy,
> >> > > > but guaranteed to produce the trailing null character.
> >> > >
> >> > > Do we really need the bizarre strncpy padding semantics for anything?
> >> > > Why not just use strlcpy?
> >> >
> >> > We do need the padding in many places to prevent information leaks.
> >>
> >> Like where?
> >>
> >
> > You're asking what would break if we switched every strncpy() to
> > strlcpy() but it's not an easy question to answer.
> >
> > I've looked a lot at information leaks, but strings are still a blind
> > spot for my Smatch.  My check only looks at normal variables, arrays.
> > Eventually I hope to fix this, of course.
> >
> > I did a git search and Rickard has added some examples, but there were
> > definitely other places that rely strncpy() padding before.
> >
> > regards,
> > dan carpenter
> 
> 
> Hi
> 
> If you want to see examples of this type of error, you can check the
> patches I've done over the past two months.
> So in linux-next
> 
> git log --patch-with-stat
> 
> And search for my email address.
> 
> And there is also an approximately 20 more patches I waited to see if
> I will be able to use a potential strzcpy  :-)

I have to say, this email thread has been a good demonstration of the
effects of incomplete changelogging :(

How's about you try again and this time include a *full* explanation of
why you believe the kernel needs strzcpy()?  Describe the problem,
provide those examples, explain the proposed solution, etc.

Also...  I misread the code - that implementation indeed zeroes out to
the end of the destination.  I'm not sure that it does it very
efficiently though - it repeatedly dereferences `src' just to obtain a
null byte?  A second nulling loop would be clearer and perhaps faster.

Re: [PATCH] lib: string.c: Added a funktion function strzcpy

[Rickard Strandqvist]

2014-09-24 23:26 GMT+02:00 Andrew Morton <akpm@linux-foundation.org>:
> On Wed, 24 Sep 2014 22:51:14 +0200 Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se> wrote:
>
>> 2014-09-24 17:41 GMT+02:00 Dan Carpenter <dan.carpenter@oracle.com>:
>> > On Wed, Sep 24, 2014 at 07:35:55AM -0700, Andi Kleen wrote:
>> >> On Wed, Sep 24, 2014 at 10:52:06AM +0300, Dan Carpenter wrote:
>> >> > On Tue, Sep 23, 2014 at 06:17:53PM -0700, Andi Kleen wrote:
>> >> > > On Wed, Sep 24, 2014 at 12:13:36AM +0200, Rickard Strandqvist wrote:
>> >> > > > Added a function strzcpy which works the same as strncpy,
>> >> > > > but guaranteed to produce the trailing null character.
>> >> > >
>> >> > > Do we really need the bizarre strncpy padding semantics for anything?
>> >> > > Why not just use strlcpy?
>> >> >
>> >> > We do need the padding in many places to prevent information leaks.
>> >>
>> >> Like where?
>> >>
>> >
>> > You're asking what would break if we switched every strncpy() to
>> > strlcpy() but it's not an easy question to answer.
>> >
>> > I've looked a lot at information leaks, but strings are still a blind
>> > spot for my Smatch.  My check only looks at normal variables, arrays.
>> > Eventually I hope to fix this, of course.
>> >
>> > I did a git search and Rickard has added some examples, but there were
>> > definitely other places that rely strncpy() padding before.
>> >
>> > regards,
>> > dan carpenter
>>
>>
>> Hi
>>
>> If you want to see examples of this type of error, you can check the
>> patches I've done over the past two months.
>> So in linux-next
>>
>> git log --patch-with-stat
>>
>> And search for my email address.
>>
>> And there is also an approximately 20 more patches I waited to see if
>> I will be able to use a potential strzcpy  :-)
>
> I have to say, this email thread has been a good demonstration of the
> effects of incomplete changelogging :(
>
> How's about you try again and this time include a *full* explanation of
> why you believe the kernel needs strzcpy()?  Describe the problem,
> provide those examples, explain the proposed solution, etc.
>
> Also...  I misread the code - that implementation indeed zeroes out to
> the end of the destination.  I'm not sure that it does it very
> efficiently though - it repeatedly dereferences `src' just to obtain a
> null byte?  A second nulling loop would be clearer and perhaps faster.


Hi

Thought that I was still fairly clear, or that I just assumed that
everyone was more or less familiar with the problems between strncpy
strlcpy etc.
Did not think all that should be includerad in chang log.

But okay, will take a few days before I have time to do all that :-(

I can agree that the code does not look well optimized, but because it
is exactly the same code as the current strncpy I assumed that someone
made that choice already...?

My addition were only the:

if (dest != tmp)
   *--tmp = '\0';


Kind regards
Rickard Strandqvist

Re: [PATCH] lib: string.c: Added a funktion function strzcpy

[Andi Kleen]

> Thought that I was still fairly clear, or that I just assumed that
> everyone was more or less familiar with the problems between strncpy
> strlcpy etc.
> Did not think all that should be includerad in chang log.

I don't think you need to write a novel. 

So the basic issue is that you're not sure if you can replace
strncpy with strlcpy (due to not knowing what the padding 
requirements are), but you still want to make sure everything
is zero terminated.

That should have been in the changelog.

-Andi