linux-kernel - Re: [PATCH v2 1/8] lib: string: Introduce strreplace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87pp55jqp6.fsf@rasmusvillemoes.dk>
Date:	Tue, 09 Jun 2015 09:17:41 +0200
From:	Rasmus Villemoes <linux@...musvillemoes.dk>
To:	Joe Perches <joe@...ches.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Daniel Borkmann <daniel@...earbox.net>,
	Herbert Xu <herbert@...dor.apana.org.au>,
	Al Viro <viro@...IV.linux.org.uk>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/8] lib: string: Introduce strreplace

On Tue, Jun 09 2015, Joe Perches <joe@...ches.com> wrote:

> On Tue, 2015-06-09 at 01:26 +0200, Rasmus Villemoes wrote:
>> Strings are sometimes sanitized by replacing a certain character
>> (often '/') by another (often '!').
> []
>> v2: spello fixed, parameters renamed 'old' and 'new' (just so the
>> kernel doc aligns nicely, and because that's what python -c
>> 'help(str.replace)' uses). Still EXPORT_SYMBOL, not inline (tried it,
>> caused more bloat), still called strreplace.
>
> OK, thanks.  I think the chars should be ints though
> just for consistency for strchr variants.

I disagree. That way lies subtle (semi)bugs. Quick quiz: What does
memscan return below?

  char a[1] = {X}; /* for some suitable X */
  char *p = memscan(a, a[0], 1);

Here's the kerneldoc+prototype for memscan:

/**
 * memscan(void *addr, int c, size_t size) - Find a character in an area of memory.
 * @addr: The memory area
 * @c: The byte to search for
 * @size: The size of the area.
 *
 * returns the address of the first occurrence of @c, or 1 byte past
 * the area if @c is not found
 */

So obviously p==a, right? Wrong. Or rather, wrong when char is signed
and X lies outside the ascii range. Or maybe right, if you're on an
architecture with its own memscan that DTRT. And 'the right thing' is
obviously to use only the LSB of c, which would have been harder to get
wrong if c was just a u8 to begin with.

(The only in-tree callers which do not pass an explicit non-negative
constant seem to be in drivers/hid/usbhid/usbkbd.c and
net/bluetooth/hidp/core.c, and they both pass something from an unsigned
char array).

We're stuck with int in the libc functions, but we can do better for new
interfaces.

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