Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1239144 > unrolled thread

Re: [GIT PULL] strscpy string copy function

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [GIT PULL] strscpy string copy function Linus Torvalds <torvalds@linux-foundation.org> - 2015-10-04 18:00 +0200
    [PATCH] string: Improve the generic strlcpy() implementation Ingo Molnar <mingo@kernel.org> - 2015-10-05 13:30 +0200
      Re: [PATCH] string: Improve the generic strlcpy() implementation Ingo Molnar <mingo@kernel.org> - 2015-10-05 14:00 +0200
        Re: [PATCH] string: Improve the generic strlcpy() implementation Ingo Molnar <mingo@kernel.org> - 2015-10-05 15:20 +0200
          Re: [PATCH] string: Improve the generic strlcpy() implementation Ingo Molnar <mingo@kernel.org> - 2015-10-05 16:10 +0200
          Re: [PATCH] string: Improve the generic strlcpy() implementation Ingo Molnar <mingo@kernel.org> - 2015-10-05 16:10 +0200
          Re: [PATCH] string: Improve the generic strlcpy() implementation Ingo Molnar <mingo@kernel.org> - 2015-10-05 16:40 +0200
            Re: [PATCH] string: Improve the generic strlcpy() implementation Linus Torvalds <torvalds@linux-foundation.org> - 2015-10-05 17:40 +0200
              Re: [PATCH] string: Improve the generic strlcpy() implementation Ingo Molnar <mingo@kernel.org> - 2015-10-05 18:10 +0200
      Re: [PATCH] string: Improve the generic strlcpy() implementation Linus Torvalds <torvalds@linux-foundation.org> - 2015-10-05 14:30 +0200
        Re: [PATCH] string: Improve the generic strlcpy() implementation Ingo Molnar <mingo@kernel.org> - 2015-10-05 15:20 +0200

#1239144 — Re: [GIT PULL] strscpy string copy function

On Thu, Sep 10, 2015 at 8:43 PM, Chris Metcalf <cmetcalf@ezchip.com> wrote:
>
> Please pull the following changes for 4.3 from:
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/cmetcalf/linux-tile.git strscpy

So I finally pulled it. I like the patch, I like the new interface,
but despite that I wasn't really sure if I wanted to pull it in - thus
the long delay of me just seeing this in my list of pending pulls for
almost a month, but never really getting to the point where I decided
I want to commit to it.

I wrote a longish merge message about why - but it boils down to me
hating the mindless trivial conversion patches. Which were not in the
pull request, but I want to make it clear to everybody that I have
absolutely zero interest in seeing such patches. I want to encourage
judicious use of strscpy() in new code, or in code that gets modified
because it is buggy or is updated for other reasons (and thus thought
about and tested), but I am *not* going to accept patches that do mass
conversions of strlcpy or strncpy to the new interface.

So just pulling the support seemed safe since ghere are no actual
*users* of this yet. So it's purely preparatory for future patches, so
it still made sense just before I'm doing an -rc4. Of course, I hope I
won't regret that "seems safe", since I'm sure the newly exposed
word-at-a-time things may well break architectures that I am not
test-compiling (ie all of them except x86-64), but it looked fine and
any breakage should be trivial.

Side note: I'm not entirely convinced about the "__must_check". There
are real cases where you don't really care whether you get a full
string or not, and strncpy() or strlcpy() may be unacceptable due to
their respective problems. But let's see once we start getting real
users who really thought about what they want.

                Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1239474 — [PATCH] string: Improve the generic strlcpy() implementation

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, Sep 10, 2015 at 8:43 PM, Chris Metcalf <cmetcalf@ezchip.com> wrote:
> >
> > Please pull the following changes for 4.3 from:
> >
> >   git://git.kernel.org/pub/scm/linux/kernel/git/cmetcalf/linux-tile.git strscpy
> 
> So I finally pulled it. I like the patch, I like the new interface,
> but despite that I wasn't really sure if I wanted to pull it in - thus
> the long delay of me just seeing this in my list of pending pulls for
> almost a month, but never really getting to the point where I decided
> I want to commit to it.

Interesting. I noticed that strscpy() says this in its comments:

 * In addition, the implementation is robust to the string changing out
 * from underneath it, unlike the current strlcpy() implementation.

The strscpy() interface is very nice, but shouldn't we also fix this strlcpy() 
unrobustness/race it refers to, in light of the 2000+ existing strlcpy() call 
sites?

The comment does not spell out what the exact race is, but I can see only a single 
race in the current generic strlcpy() implementation, which all architectures 
except s390 uses:

size_t strlcpy(char *dest, const char *src, size_t size)
{
        size_t ret = strlen(src);

        if (size) {
                size_t len = (ret >= size) ? size - 1 : ret;
                memcpy(dest, src, len);
                dest[len] = '\0';
        }
        return ret;
}

If another CPU or an interrupt changes the source string after the strlen(), but 
before the copy is complete, and shortens the source string, then we copy over the 
NUL byte of the source buffer - including fragments of earlier source string 
tails. The target buffer will still be properly NUL terminated - but it will be a 
shorter string than the returned 'ret' source buffer length. (despite there not 
being truncation.)

The s390 arch implementation has the same race AFAICS.

This may cause bugs if the return code is subsequently used to assume that it is 
equal to the destination string's length. (While in reality it's shorter.)

The race is not automatically lethal, because it's guaranteed that the returned 
length is indeed zero-delimited (due to the overlong copy we did) - so if the 
string is memcpy()-ed, then it will still result in a weirdly padded but valid 
string.

But if any subsequent use of the return code relies on the return code being equal 
to a subsequent call of strlen(dest), then that use might lead to bugs. I.e. our 
implementation of strlcpy() is indeed racy and unrobust.

But we could fix this race: by iterating over the string in a single go and 
determining the length and copying the string at once. Like the new strscpy() code 
does it, but with strlcpy() semantics.

This will also make strlcpy() faster, FWIMBW.

I also noticed another problem with our current generic strlcpy() implementation: 
AFAICS it will also happily do bad stuff if we pass it a negative size. Instead of 
that we should print a warning and return safely.

I've implemented all that, see the patch below.

(Only very lightly tested so far and no benchmarking done.)

Thanks,

	Ingo

===================================>
From 53fc46c16ed65e67906d5b453e19d8f688093f70 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@kernel.org>
Date: Mon, 5 Oct 2015 10:56:50 +0200
Subject: [PATCH] string: Improve the generic strlcpy() implementation

The current strlcpy() implementation has two implementational
weaknesses:

1)

There's a race:

size_t strlcpy(char *dest, const char *src, size_t size)
{
        size_t ret = strlen(src);

        if (size) {
                size_t len = (ret >= size) ? size - 1 : ret;
                memcpy(dest, src, len);
                dest[len] = '\0';
        }
        return ret;
}

If another CPU or an interrupt changes the source string after the strlen(), but
before the copy is complete, and shortens the source string, then we copy over the
NUL byte of the source buffer - including fragments of earlier source string
tails. The target buffer will still be properly NUL terminated - but it will be a
shorter string than the returned 'ret' source buffer length. (despite there not
being truncation.)

The s390 arch implementation has the same race AFAICS.

This may cause bugs if the return code is subsequently used to assume that it is
equal to the destination string's length. (While in reality it's shorter.)

The race is not automatically lethal, because it's guaranteed that the returned
length is indeed zero-delimited (due to the overlong copy we did) - so if the
string is memcpy()-ed, then it will still result in a weirdly padded but valid
string.

But if any subsequent use of the return code relies on the return code being equal
to a subsequent call of strlen(dest), then that use might lead to bugs. I.e. our
implementation of strlcpy() is indeed racy and unrobust.

But we can fix this race: by iterating over the string in a single go and
determining the length and copying the string at once. Like strscpy(), but with
strlcpy() semantics.

The new implementation uses word-by-word iteration over the strings if possible,
so this will also make strlcpy() faster as well.

2)

Another problem is that strlcpy() will also happily do bad stuff if we pass
it a negative size. Instead of that we will from now on print a (one time)
warning and return safely.

Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 lib/string.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 78 insertions(+), 8 deletions(-)

diff --git a/lib/string.c b/lib/string.c
index 8dbb7b1eab50..e0cfca299606 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -129,23 +129,93 @@ EXPORT_SYMBOL(strncpy);
  * strlcpy - Copy a C-string into a sized buffer
  * @dest: Where to copy the string to
  * @src: Where to copy the string from
- * @size: size of destination buffer
+ * @dest_size: size of destination buffer
  *
  * Compatible with *BSD: the result is always a valid
  * NUL-terminated string that fits in the buffer (unless,
  * of course, the buffer size is zero). It does not pad
  * out the result like strncpy() does.
  */
-size_t strlcpy(char *dest, const char *src, size_t size)
+size_t strlcpy(char *dest, const char *src, size_t dest_size)
 {
-	size_t ret = strlen(src);
+	const struct word_at_a_time constants = WORD_AT_A_TIME_CONSTANTS;
+	size_t dest_left = dest_size;
+	size_t dest_aligned_left = dest_left;
+	long src_len = 0;
+
+	/* Overflow check: */
+	if (unlikely(dest_size < 0)) {
+		WARN_ONCE(1, "strlcpy(): dest_size < 0 underflow!");
+		return strlen(src);
+	}
+
+#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 < dest_aligned_left)
+			dest_aligned_left = limit;
+	}
+#else
+	/* If src or dest is unaligned, don't do word-at-a-time. */
+	if (((long) dest | (long) src) & (sizeof(long) - 1))
+		dest_aligned_left = 0;
+#endif
+
+	/* First do the word-at-a-time copy of the aligned portion (if any): */
+	while (dest_aligned_left >= sizeof(unsigned long)) {
+		unsigned long c, data;
 
-	if (size) {
-		size_t len = (ret >= size) ? size - 1 : ret;
-		memcpy(dest, src, len);
-		dest[len] = '\0';
+		c = *(unsigned long *)(src+src_len);
+		*(unsigned long *)(dest+src_len) = c;
+
+		if (has_zero(c, &data, &constants)) {
+			data = prep_zero_mask(c, data, &constants);
+			data = create_zero_mask(data);
+			/* The target string was terminated by the above word copy */
+			return src_len + find_zero(data);
+		}
+		src_len += sizeof(unsigned long);
+		dest_left -= sizeof(unsigned long);
+		dest_aligned_left -= sizeof(unsigned long);
 	}
-	return ret;
+
+	/*
+	 * We get here either for tails smaller than word size, or
+	 * unaligned strings. Copy byte by byte and return the
+	 * length of the source string if we find its end:
+	 */
+	while (dest_left) {
+		char c;
+
+		c = src[src_len];
+		dest[src_len] = c;
+		if (!c)
+			/* The target string was terminated by the above byte copy */
+			return src_len;
+		src_len++;
+		dest_left--;
+	}
+
+	/*
+	 * We get here if the source string is larger than the destination buffer.
+	 *
+	 * The strlcpy() semantics require us to return the length of the
+	 * source string - so we have to continue until we find its end.
+	 *
+	 * We first zero-terminate the (truncated, hence non yet terminated)
+	 * target string.
+	 */
+	if (dest_size)
+		dest[dest_size-1] = '\0';
+
+	while (src[src_len])
+		src_len++;
+
+	return src_len;
 }
 EXPORT_SYMBOL(strlcpy);
 #endif
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239489 — Re: [PATCH] string: Improve the generic strlcpy() implementation

* Ingo Molnar <mingo@kernel.org> wrote:

> 2)
> 
> Another problem is that strlcpy() will also happily do bad stuff if we pass
> it a negative size. Instead of that we will from now on print a (one time)
> warning and return safely.

Hm, so this check is buggy, as 'size_t' is unsigned - and for some reason GCC 
didn't warn about the never-met comparison and the resulting unreachable dead
code here:

> +	/* Overflow check: */
> +	if (unlikely(dest_size < 0)) {
> +		WARN_ONCE(1, "strlcpy(): dest_size < 0 underflow!");
> +		return strlen(src);
> +	}

which is annoying.

Would people object to something like:

> +	/* Overflow check: */
> +	if (unlikely((ssize_t)dest_size < 0)) {
> +		WARN_ONCE(1, "strlcpy(): dest_size < 0 underflow!");
> +		return strlen(src);
> +	}

?

As I doubt it's legit to have larger than 2GB strings.

Also, I'm wondering why GCC didn't warn.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239549 — Re: [PATCH] string: Improve the generic strlcpy() implementation

* Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Ingo Molnar <mingo@kernel.org> wrote:
> 
> > 2)
> > 
> > Another problem is that strlcpy() will also happily do bad stuff if we pass
> > it a negative size. Instead of that we will from now on print a (one time)
> > warning and return safely.
> 
> Hm, so this check is buggy, as 'size_t' is unsigned - and for some reason GCC 
> didn't warn about the never-met comparison and the resulting unreachable dead
> code here:
> 
> > +	/* Overflow check: */
> > +	if (unlikely(dest_size < 0)) {
> > +		WARN_ONCE(1, "strlcpy(): dest_size < 0 underflow!");
> > +		return strlen(src);
> > +	}
> 
> which is annoying.
> 
> Would people object to something like:
> 
> > +	/* Overflow check: */
> > +	if (unlikely((ssize_t)dest_size < 0)) {
> > +		WARN_ONCE(1, "strlcpy(): dest_size < 0 underflow!");
> > +		return strlen(src);
> > +	}
> 
> ?
> 
> As I doubt it's legit to have larger than 2GB strings.
> 
> Also, I'm wondering why GCC didn't warn.

Hm, so GCC (v4.9.2) will only warn about this bug if -Wtype-limits is enabled 
explicitly:

 lib/string.c: In function ‘strlcpy’:
 lib/string.c:228:32: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
   if (unlikely((size_t)dst_size < 0)) {
                                 ^

... which we don't do in the kernel.

Has anyone considered enabling -Wtype-limits? It seems to catch real bugs.

I can see there are patches that enable -Wextra (which enables -Wtype-limits and 
many other warnings), but it would be more manageable to just enable one such 
warning at a time.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239581 — Re: [PATCH] string: Improve the generic strlcpy() implementation

* Ingo Molnar <mingo@kernel.org> wrote:

> Hm, so GCC (v4.9.2) will only warn about this bug if -Wtype-limits is enabled 
> explicitly:
> 
>  lib/string.c: In function ‘strlcpy’:
>  lib/string.c:228:32: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
>    if (unlikely((size_t)dst_size < 0)) {
>                                  ^
> 
> ... which we don't do in the kernel.
> 
> Has anyone considered enabling -Wtype-limits? It seems to catch real bugs.
> 
> I can see there are patches that enable -Wextra (which enables -Wtype-limits and 
> many other warnings), but it would be more manageable to just enable one such 
> warning at a time.

So I checked this and -Wtype-limits is super chatty at the moment, on various 
configs on x86:

 def64:    warnings:  51, delta: +51
 def32:    warnings:  50, delta: +50
 allno64:  warnings:  24, delta: +21
 allno32:  warnings:  26, delta: +24
 allyes64: warnings: 292, delta: +278
 allyes32: warnings: 318, delta: +277
 allmod64: warnings: 298, delta: +287
 allmod32: warnings: 324, delta: +286

(The delta column is the number of new warnings relative to v4.3-rc4.)

I picked 10 random warnings that triggered in files that looked interesting to me:

1) false positive warning:

./arch/x86/include/asm/apic.h:33:11: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]

is caused by macro substitution:

#define apic_printk(v, s, a...) do {       \
                if ((v) <= apic_verbosity) \
                        printk(s, ##a);    \
        } while (0)

so if 'v' is 0 GCC thinks it's a bad comparison - while it isn't.

This would be easily worked around if we moved the code from CPP to C - which is 
beneficial in any case.

2) confused code (possibly harmful):

arch/x86/kernel/pci-calgary_64.c:299:25: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]

this:

  static void iommu_free(struct iommu_table *tbl, dma_addr_t dma_addr, unsigned int npages)
  {

	...
        if (unlikely((dma_addr >= DMA_ERROR_CODE) && (dma_addr < badend))) {
                WARN(1, KERN_ERR "Calgary: driver tried unmapping bad DMA "
                       "address 0x%Lx\n", dma_addr);
                return;
        }

is nonsense because dma_addr is unsigned and DMA_ERROR_CODE is 0. The right way to 
check for DMA_ERROR_CODE is to check it:

  triton:~/tip> git grep DMA_ERROR_CODE | grep -E '==|!=' | wc -l
  29

not compare it:

  triton:~/tip> git grep DMA_ERROR_CODE | grep -E ' <= | >= | < | > ' | wc -l
  1

3) false positive warning:

block/cfq-iosched.c:4657:13: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]

substition of '0' in the STORE_FUNCTION() macro causes this one.

4) confused code (looks harmless):

crypto/asymmetric_keys/x509_cert_parser.c:549:11: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]

        unsigned year, mon, day, hour, min, sec, mon_len;
	...

            hour < 0 || hour > 23 ||

so 'hour' cannot be negative. Looks harmless.

5) false positive warning:

drivers/nvdimm/pmem.c:257:38: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]

        if (nvdimm_namespace_capacity(ndns) < ND_PFN_ALIGN

so ND_PFN_ALIGN can be 0 if CONFIG_NVDIMM_PFN is disabled.

6) confused code (looks harmless):

kernel/auditsc.c:1027:23: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]

        size_t len, len_left, to_send;

	...

        if (WARN_ON_ONCE(len < 0 || len > MAX_ARG_STRLEN - 1)) {

So this looks like a real bug similar to the one I made in the strlcpy() warning: 
the code wants to warn about a negative underflow - but instead it does not check 
for that condition at all.

It's harmless because the len > MAX_ARG_STRLEN-1 check would catch any underflows.

7) confused code (looks harmless):

mm/memblock.c:840:11: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]


	void __init_memblock __next_reserved_mem_region(u64 *idx,
	...

        if (*idx >= 0 && *idx < type->cnt) {

so *idx is u64 so it's always >= 0. Looks harmless.

8) confused code (looks harmless):

fs/cachefiles/bind.c:42:30: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]

        /* start by checking things over */
        ASSERT(cache->fstop_percent >= 0 &&
               cache->fstop_percent < cache->fcull_percent &&
               cache->fcull_percent < cache->frun_percent &&
               cache->frun_percent  < 100);


'fstop_percent' is unsigned int, so the >= 0 test is superfluous. Looks harmless.

9) confused code (looks harmless):

fs/cachefiles/daemon.c:225:14: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]

                                       size_t datalen,
	...

        if (datalen < 0 || datalen > PAGE_SIZE - 1)
                return -EOPNOTSUPP;


so 'datalen' is unsigned and this can never be negative - and the second check 
catches any underflows. Looks harmless.

10) somewhat confused code (harmless):

net/rds/iw_recv.c:203:26: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]

So this comes from the comparison of RDS_PAGE_LAST_OFF:

        get_page(recv->r_frag->f_page);

        if (ic->i_frag.f_offset < RDS_PAGE_LAST_OFF) {
                ic->i_frag.f_offset += RDS_FRAG_SIZE;
        } else {
                put_page(ic->i_frag.f_page);
                ic->i_frag.f_page = NULL;
                ic->i_frag.f_offset = 0;
        }

but i_frag.f_offset is unsigned:

 net/rds/iw.h:   unsigned long           f_offset;

and:

 net/rds/iw.h:#define RDS_PAGE_LAST_OFF (((PAGE_SIZE  / RDS_FRAG_SIZE) - 1) * RDS_FRAG_SIZE)

where RDS_FRAG_SIZE == 4096, so RDS_PAGE_LAST_OFF becomes 0.

if RDS_FRAG_SIZE was smaller than 4096, say 512, then RDS_PAGE_LAST_OFF would show 
the offset within the page of the last fragment, i.e. 3584.

So the code looks correct but inefficient: it does the get_page()/put_page() 
unnecessarily in the RDS_FRAG_SIZE == PAGE_SIZE case, which is the default on most 
architectures.

I'd write this as:

        if (ic->i_frag.f_offset < RDS_PAGE_LAST_OFF) {
	        get_page(recv->r_frag->f_page);
                ic->i_frag.f_offset += RDS_FRAG_SIZE;
        } else {
                ic->i_frag.f_page = NULL;
                ic->i_frag.f_offset = 0;
        }

(but this would still generate the warning.)

============

So the summary from 10 examples is:

 1) false positive warning
 2) confused code (possibly harmful)
 3) false positive warning
 4) confused code (looks harmless)
 5) false positive warning
 6) confused code (looks harmless)
 7) confused code (looks harmless)
 8) confused code (looks harmless)
 9) confused code (looks harmless)
10) somewhat confused code (harmless)

I.e. 3 genuine false positive warnings, 6 pointing out harmless looking code 
confusion, 1 pointing out harmful looking code confusion - and one of the sites it 
pointed out highlighted an inefficiency.

That doesn't look too bad of a false positive ration from a compiler warning, 
especially considering that static checkers (and people running -Wextra builds) 
have been cleaning out these cases for quite some time it appears, which means 
these are the leftover warnings.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239583 — Re: [PATCH] string: Improve the generic strlcpy() implementation

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Oct 5, 2015 14:15, "Ingo Molnar" <mingo@kernel.org> wrote:
> >
> > Hm, so GCC (v4.9.2) will only warn about this bug if -Wtype-limits is enabled 
> > explicitly:
> 
> Some of the warnings are really nasty, and cause people to write worse code.
> 
> For example, this is inherently good code:
> 
>     if (x < 0 || x > MAXLEN)
>         return -EINVAL;
> 
> and a compiler that warns about that is pure and utter crap. Obviously. Agreed?
> 
> Now, imagine that "x" here is some random type. Maybe it's s "char" and you 
> don't even know the sign. Maybe it's "loff_t". Maybe it's "size_t", or whatever.
> 
> Note how that test is correct *regardless* of the sign of the type. A compiler 
> that warns about the "x < 0" part just because x happens to be unsigned is a bad 
> bad compiler, and makes people remove that check, even though it's good for 
> readability, and good for robustness wrt changing the type.

Yeah, that's true.

> We really do have types where sightedness depends on architecture or
> possibly configuration options. "char" is the obvious example, but the type
> limit can matter too: on some architectures you might have a type that is
> 16 bits, on another it might be 32 bits. Do you really think that
> 
>      if (x > 65535)
>           return -E2BIG;
> 
> should have some #ifdef __xyz__ around it just because the compiler warns
> if the type happens to be 16 bits wide?
> 
> So type limit warnings break not things than they fix.

Yeah, too bad.

> These things come up in macros too (think range checking etc).
> 
> In other words, that warning really isn't good if it's done mindlessly. And I've 
> never seen a compiler that did it sanely and trying to take context into 
> account.
> 
> So no. Don't enable that broken warning. We have had it on, and it caused people 
> to send in patches for warnings that made the code actively worse.

Okay. Please disregard my other mail.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239611 — Re: [PATCH] string: Improve the generic strlcpy() implementation

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Oct 5, 2015 14:15, "Ingo Molnar" <mingo@kernel.org> wrote:
> >
> > Hm, so GCC (v4.9.2) will only warn about this bug if -Wtype-limits is
> enabled
> > explicitly:
> 
> Some of the warnings are really nasty, and cause people to write worse code.
> 
> For example, this is inherently good code:
> 
>     if (x < 0 || x > MAXLEN)
>         return -EINVAL;
> 
> and a compiler that warns about that is pure and utter crap. Obviously.
> Agreed?
> 
> Now, imagine that "x" here is some random type. Maybe it's s "char" and you
> don't even know the sign. Maybe it's "loff_t". Maybe it's "size_t", or
> whatever.
> 
> Note how that test is correct *regardless* of the sign of the type. A
> compiler that warns about the "x < 0" part just because x happens to be
> unsigned is a bad bad compiler, and makes people remove that check, even
> though it's good for readability, and good for robustness wrt changing the
> type.

Hm, so there's a flip side here - if we consider 'example 6)' in my previous mail:

  kernel/auditsc.c:1027:23: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]

        size_t len, len_left, to_send;

        ...

        if (WARN_ON_ONCE(len < 0 || len > MAX_ARG_STRLEN - 1)) {

Now if this code was written as:

        if (WARN_ON_ONCE(len < 0)) {

then it would be a clear bug, right?

So we could solve that by adding a generic range check:

 static inline int range_ok(unsigned long low, unsigned long val, unsigned long high)
 {               
        if (val < low)
                return 0;
        if (val >= high)
                return 0;

        return 1;
 }

and we could write this:

        if (len < 0 || len > MAX_ARG_STRLEN - 1) {

as:

        if (!range_ok(0, len, MAX_ARG_STRLEN)) {

?

That kind of construct:

 - is robust against a changed type for 'len'

 - is robust against these common typos for open coded security checks:

        if (len <= 0 || len > MAX_ARG_STRLEN - 1) {

        if (len < 0 || len >= MAX_ARG_STRLEN - 1) {

        if (len < 0 || len > MAX_ARG_STRLEN) {

   the first and second ones over-check and are harmless in this context, the 
   third one is harmful because it does not catch the MAX_ARG_STRLEN case.

 - it would also clearly document range checking performed in a function that gets
   untrusted data.

Hypothetically, if this was acceptable then we could use this in the cases where 
GCC generates a bogus warning.

But ... no strong feelings. Just found it weird that GCC let my bug slip through.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239686 — Re: [PATCH] string: Improve the generic strlcpy() implementation

On Mon, Oct 5, 2015 at 3:33 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> So we could solve that by adding a generic range check:
>
>  static inline int range_ok(unsigned long low, unsigned long val, unsigned long high)

So what about the type of the thing you're checking?

Maybe negative values are ok. It's unusual, but it's not unheard of.
We do have cases like

    if ((uch_config < -1) || (uch_config > 31)) {

so we have range checks that actually have signed ranges.

So I don't think a "generic" range check helper can force types like
"unsigned long".

That said, doing a simple

    git grep '<.*||.*>'

does show that the "positive or non-zero ranges with constant range
limits" case is fairly common, and maybe we could have a macro that
does some magic compile-time checking that (a) the range really is a
compile-time constant and (b) that range is valid and (c) avoids the
comparison with zero if the expression to be tested is unsigned.

So it is possible that we could enable type limit checking if we also
introduce a good way to not then create crap patches that actually
make the code more fragile or less readable. I'm not violently against
that. But I *am* violently against introducing that braindead warning
without very clear rules that we don't then have the mindless and
wrong changes to remove proper and obvious range checking and replace
it with "the expression is unsigned so we remove the nice readable
lower bounds check as unnecessary".

            Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239714 — Re: [PATCH] string: Improve the generic strlcpy() implementation

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> So I don't think a "generic" range check helper can force types like
> "unsigned long".

Yeah.

> That said, doing a simple
> 
>     git grep '<.*||.*>'
> 
> does show that the "positive or non-zero ranges with constant range
> limits" case is fairly common, and maybe we could have a macro that
> does some magic compile-time checking that (a) the range really is a
> compile-time constant and (b) that range is valid and (c) avoids the
> comparison with zero if the expression to be tested is unsigned.
> 
> So it is possible that we could enable type limit checking if we also
> introduce a good way to not then create crap patches that actually
> make the code more fragile or less readable. I'm not violently against
> that. But I *am* violently against introducing that braindead warning
> without very clear rules that we don't then have the mindless and
> wrong changes to remove proper and obvious range checking and replace
> it with "the expression is unsigned so we remove the nice readable
> lower bounds check as unnecessary".

Ok, and fully agreed.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239507 — Re: [PATCH] string: Improve the generic strlcpy() implementation

On Mon, Oct 5, 2015 at 12:27 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> Interesting. I noticed that strscpy() says this in its comments:
>
>  * In addition, the implementation is robust to the string changing out
>  * from underneath it, unlike the current strlcpy() implementation.
>
> The strscpy() interface is very nice, but shouldn't we also fix this strlcpy()
> unrobustness/race it refers to, in light of the 2000+ existing strlcpy() call
> sites?

Well, I'm not sure the race really matters. I personally think
strlcpy() is a horrible interface, and the thing is, the return value
of strlcpy (which is what can race) is kind of useless because it's
not actually the size of the resulting string *anyway* (because of the
overflow issue).

So I'm not sure it's worth even fixing.

Also, if you do this, then you're better off using the (hopefully
optimized) "strlen()" for the tail part of the strlcpy destination for
the overflow case that didn't get copied.

In other words, I think your patch is overly fragile and complex.
Instead, you might choose to implement strlcpy() in terms of
"strscpy()" and "strlen()".

Something like

  int strlcpy(dst, src, len)
  {
     // do the actual copy
     int n = strscpy(dst, src, len);

     // handle the insane and broken strlcpy overflow return value
     if (n < 0)
         return len + strlen(src+len);

     return n;
   }

but I didn't actually verify that the above is correct for all the corner case.

The point being, that you really shouldn't waste your time
implementing the broken BSD strlcpy crap as an actual first-class
implementation. You're better off just using a strscpy() as the
primary engine, and then implementing the broken strlcpy interfaces on
top of it.

Does the above work? I'd take a patch that implements that if it's
tested and somebody has thought about it a lot. But I don't like your
patch that open-codes the insane interface with complex and fragile
code.

          Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239554 — Re: [PATCH] string: Improve the generic strlcpy() implementation

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Mon, Oct 5, 2015 at 12:27 PM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > Interesting. I noticed that strscpy() says this in its comments:
> >
> >  * In addition, the implementation is robust to the string changing out
> >  * from underneath it, unlike the current strlcpy() implementation.
> >
> > The strscpy() interface is very nice, but shouldn't we also fix this strlcpy()
> > unrobustness/race it refers to, in light of the 2000+ existing strlcpy() call
> > sites?
> 
> Well, I'm not sure the race really matters. [...]

Yeah, so if we do the word-by-word optimization for strlcpy() then the race is 
fixed 'automatically', for free.

But you are right:

> [...] I personally think strlcpy() is a horrible interface, and the thing is, 
> the return value of strlcpy (which is what can race) is kind of useless because 
> it's not actually the size of the resulting string *anyway* (because of the 
> overflow issue).
> 
> So I'm not sure it's worth even fixing.

> Also, if you do this, then you're better off using the (hopefully optimized) 
> "strlen()" for the tail part of the strlcpy destination for the overflow case 
> that didn't get copied.

Indeed, this on top of my patch should do that:

 lib/string.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/lib/string.c b/lib/string.c
index e0cfca299606..dfd24b557e84 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -212,10 +212,7 @@ size_t strlcpy(char *dest, const char *src, size_t dest_size)
 	if (dest_size)
 		dest[dest_size-1] = '\0';
 
-	while (src[src_len])
-		src_len++;
-
-	return src_len;
+	return strlen(src) + src_len;
 }
 EXPORT_SYMBOL(strlcpy);
 #endif

(untested)

> In other words, I think your patch is overly fragile and complex.

Well, it's mostly a copy of strscpy() with obvious conversion of the return 
convention, but you are right to point out:

> Instead, you might choose to implement strlcpy() in terms of
> "strscpy()" and "strlen()".
> 
> Something like
> 
>   int strlcpy(dst, src, len)
>   {
>      // do the actual copy
>      int n = strscpy(dst, src, len);
> 
>      // handle the insane and broken strlcpy overflow return value
>      if (n < 0)
>          return len + strlen(src+len);
> 
>      return n;
>    }
> 
> but I didn't actually verify that the above is correct for all the corner case.

Hm, so I considered doing that initially, but managed to convince myself that it's 
not equivalent: but on a second thought your variant should indeed work!

> The point being, that you really shouldn't waste your time implementing the 
> broken BSD strlcpy crap as an actual first-class implementation. You're better 
> off just using a strscpy() as the primary engine, and then implementing the 
> broken strlcpy interfaces on top of it.
> 
> Does the above work? I'd take a patch that implements that if it's tested and 
> somebody has thought about it a lot. But I don't like your patch that open-codes 
> the insane interface with complex and fragile code.

So the below untested variant does that plus an overflow check - it's only FYI, 
not signed off yet.

Thanks,

	Ingo

==============>

Not-Signed-off-by: Ingo Molnar <mingo@kernel.org>

 lib/string.c | 60 ++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 34 insertions(+), 26 deletions(-)

diff --git a/lib/string.c b/lib/string.c
index 8dbb7b1eab50..6b89c035df74 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -124,32 +124,6 @@ char *strncpy(char *dest, const char *src, size_t count)
 EXPORT_SYMBOL(strncpy);
 #endif
 
-#ifndef __HAVE_ARCH_STRLCPY
-/**
- * strlcpy - Copy a C-string into a sized buffer
- * @dest: Where to copy the string to
- * @src: Where to copy the string from
- * @size: size of destination buffer
- *
- * Compatible with *BSD: the result is always a valid
- * NUL-terminated string that fits in the buffer (unless,
- * of course, the buffer size is zero). It does not pad
- * out the result like strncpy() does.
- */
-size_t strlcpy(char *dest, const char *src, size_t size)
-{
-	size_t ret = strlen(src);
-
-	if (size) {
-		size_t len = (ret >= size) ? size - 1 : ret;
-		memcpy(dest, src, len);
-		dest[len] = '\0';
-	}
-	return ret;
-}
-EXPORT_SYMBOL(strlcpy);
-#endif
-
 #ifndef __HAVE_ARCH_STRSCPY
 /**
  * strscpy - Copy a C-string into a sized buffer
@@ -234,6 +208,40 @@ ssize_t strscpy(char *dest, const char *src, size_t count)
 EXPORT_SYMBOL(strscpy);
 #endif
 
+#ifndef __HAVE_ARCH_STRLCPY
+/**
+ * strlcpy - Copy a C-string into a sized buffer
+ * @dst: Where to copy the string to
+ * @src: Where to copy the string from
+ * @dst_size: size of destination buffer
+ *
+ * Compatible with *BSD: the result is always a valid
+ * NUL-terminated string that fits in the buffer (unless,
+ * of course, the buffer size is zero). It does not pad
+ * out the result like strncpy() does.
+ */
+size_t strlcpy(char *dst, const char *src, size_t dst_size)
+{
+	int ret;
+
+	/* Overflow check: */
+	if (unlikely((ssize_t)dst_size < 0)) {
+		WARN_ONCE(1, "strlcpy(): dst_size < 0 underflow!");
+		return strlen(src);
+	}
+
+	ret = strscpy(dst, src, dst_size);
+
+	/* Handle the insane and broken strlcpy() overflow return value: */
+	if (ret < 0)
+		return dst_size + strlen(src+dst_size);
+
+	return ret;
+}
+EXPORT_SYMBOL(strlcpy);
+#endif
+
+
 #ifndef __HAVE_ARCH_STRCAT
 /**
  * strcat - Append one %NUL-terminated string to another

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web