linux-kernel - Re: [PATCH v2] kprobes: cleanup _kprobe

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] [day] [month] [year] [list]

Message-Id: <20170327111939.99484c418a641cb33cec20f5@kernel.org>
Date:   Mon, 27 Mar 2017 11:19:39 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     Ananth N Mavinakayanahalli <ananth@...ux.vnet.ibm.com>,
        Anil S Keshavamurthy <anil.s.keshavamurthy@...el.com>,
        "David S. Miller" <davem@...emloft.net>,
        linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH v2] kprobes: cleanup _kprobe_addr()

On Fri, 24 Mar 2017 12:50:34 +0300
Dan Carpenter <dan.carpenter@...cle.com> wrote:

> The important bit here is that at the end of the function we know that
> since "addr" isn't be NULL, it means we don't need to test
> "addr + offset" for NULL.
> 
> The NULL test generates a static checker warning because often something
> else is intended.  For example, a common bug looks like:
> 
> 	addr = strchr(buf, ':') + 1;
> 	if (addr) {
> 
> In that example, we intended to test the return of strchr() instead of
> "strchr() + 1".
> 
> Techinically, with these static checker warnings you could worry about
> the addition wrapping but the NULL check wouldn't prevent "addr" from
> pointing to some other invalid memory outside the text area.  Also
> pointer wrapping is undefined behavior in C.
> 

Looks good to me :)

Acked-by: Masami Hiramatsu <mhiramat@...nel.org>

> Signed-off-by: Dan Carpenter <dan.carpenter@...cle.com>
> ---
> v2: more cleanups and changelog
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index d733479a10ee..ad30b5e22bda 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1395,7 +1395,7 @@ static kprobe_opcode_t *_kprobe_addr(kprobe_opcode_t *addr,
>  			const char *symbol_name, unsigned int offset)
>  {
>  	if ((symbol_name && addr) || (!symbol_name && !addr))
> -		goto invalid;
> +		return ERR_PTR(-EINVAL);
>  
>  	if (symbol_name) {
>  		kprobe_lookup_name(symbol_name, addr);
> @@ -1403,12 +1403,7 @@ static kprobe_opcode_t *_kprobe_addr(kprobe_opcode_t *addr,
>  			return ERR_PTR(-ENOENT);
>  	}
>  
> -	addr = (kprobe_opcode_t *)(((char *)addr) + offset);
> -	if (addr)
> -		return addr;
> -
> -invalid:
> -	return ERR_PTR(-EINVAL);
> +	return (kprobe_opcode_t *)(((char *)addr) + offset);
>  }
>  
>  static kprobe_opcode_t *kprobe_addr(struct kprobe *p)


-- 
Masami Hiramatsu <mhiramat@...nel.org>