linux-kernel - Re: [PATCH 01/12] rdmacg: Replace strncmp with str_has

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: <201907292117.DA40CA7D@keescook>
Date:   Mon, 29 Jul 2019 21:26:19 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Chuhong Yuan <hslester96@...il.com>
Cc:     Tejun Heo <tj@...nel.org>, Li Zefan <lizefan@...wei.com>,
        Johannes Weiner <hannes@...xchg.org>, cgroups@...r.kernel.org,
        linux-kernel@...r.kernel.org, Joe Perches <joe@...ches.com>,
        Laura Abbott <labbott@...hat.com>,
        Michael Ellerman <mpe@...erman.id.au>,
        Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [PATCH 01/12] rdmacg: Replace strncmp with str_has_prefix

On Mon, Jul 29, 2019 at 11:13:46PM +0800, Chuhong Yuan wrote:
> strncmp(str, const, len) is error-prone.
> We had better use newly introduced
> str_has_prefix() instead of it.

Wait, stop. :) After Laura called my attention to your conversion series,
mpe pointed out that str_has_prefix() is almost redundant to strstarts()
(from 2009), and the latter has many more users. Let's fix strstarts()
match str_has_prefix()'s return behavior (all the existing callers are
doing boolean tests, so the change in return value won't matter), and
then we can continue with this replacement. (And add some documentation
to Documenation/process/deprecated.rst along with a checkpatch.pl test
maybe too?)

Actually I'd focus first on the actually broken cases first (sizeof()
without the "-1", etc):

$ git grep strncmp.*sizeof | grep -v -- '-' | wc -l
17

I expect the "copy/paste" changes could just be a Coccinelle script that
Linus could run to fix all the cases (and should be added to the kernel
source's list of Coccinelle scripts). Especially since the bulk of the
usage pattern are doing literals like this:

arch/alpha/kernel/setup.c:   if (strncmp(p, "mem=", 4) == 0) {

$ git grep -E 'strncmp.*(sizeof|, *[0-9]*)' | wc -l
2565

And some cases are weirdly backwards:

tools/perf/util/callchain.c:  if (!strncmp(tok, "none", strlen(tok))) {

-Kees

> Signed-off-by: Chuhong Yuan <hslester96@...il.com>
> ---
>  kernel/cgroup/rdma.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/cgroup/rdma.c b/kernel/cgroup/rdma.c
> index ae042c347c64..fd12a227f8e4 100644
> --- a/kernel/cgroup/rdma.c
> +++ b/kernel/cgroup/rdma.c
> @@ -379,7 +379,7 @@ static int parse_resource(char *c, int *intval)
>  			return -EINVAL;
>  		return i;
>  	}
> -	if (strncmp(value, RDMACG_MAX_STR, len) == 0) {
> +	if (str_has_prefix(value, RDMACG_MAX_STR)) {
>  		*intval = S32_MAX;
>  		return i;
>  	}
> -- 
> 2.20.1
> 

-- 
Kees Cook