netdev - Re: [PATCH bpf v2 8/9] bpf: prevent out of bounds speculation on pointer arithmetic

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: <20190102141101.58e28b28@cakuba.hsd1.ca.comcast.net>
Date:   Wed, 2 Jan 2019 14:11:01 -0800
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     ast@...nel.org, jannh@...gle.com, davem@...emloft.net,
        netdev@...r.kernel.org
Subject: Re: [PATCH bpf v2 8/9] bpf: prevent out of bounds speculation on
 pointer arithmetic

On Wed,  2 Jan 2019 00:20:45 +0100, Daniel Borkmann wrote:
> Jann reported that the original commit back in b2157399cc98
> ("bpf: prevent out-of-bounds speculation") was not sufficient
> to stop CPU from speculating out of bounds memory access:
> While b2157399cc98 only focussed on masking array map access
> for unprivileged users for tail calls and data access such
> that the user provided index gets sanitized from BPF program
> and syscall side, there is still a more generic form affected
> from BPF programs that applies to most maps that hold user
> data in relation to dynamic map access when dealing with
> unknown scalars or "slow" known scalars as access offset...

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 8e5da1c..448a828 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5499,6 +5610,13 @@ static bool states_equal(struct bpf_verifier_env *env,
>  	if (old->curframe != cur->curframe)
>  		return false;
>  
> +	/* Verification state from speculative execution simulation
> +	 * must never prune a non-speculative execution one.
> +	 */
> +	if (old->speculative != cur->speculative ||
> +	    (old->speculative && !cur->speculative))
> +		return false;

nit: if I read this correctly it looks more conservative than the
     comment suggests. 

     The second case (old->speculative && !cur->speculative) implies 
     the first case (old->speculative != cur->speculative).
     Perhaps:

	if (old->speculative && !cur->speculative)

     Or:

	if (old->speculative > cur->speculative)

> +
>  	/* for states to be equal callsites have to be the same
>  	 * and all frame states need to be equivalent
>  	 */
> @@ -5530,6 +5648,11 @@ static int propagate_liveness(struct bpf_verifier_env *env,
>  		     vparent->curframe, vstate->curframe);
>  		return -EFAULT;
>  	}
> +
> +	/* Don't propagate to non-speculative parent. */
> +	if (vparent->speculative != vstate->speculative)

I haven't thought this trough fully, but is this really necessary?
Do we assume the CPU will not speculate twice?  It seems not impossible
to have a register only accessed on the speculation path, and therefore
if we don't propagate liveness non-speculative walk may prune
speculative checks.

> +		return 0;
> +
>  	/* Propagate read liveness of registers... */
>  	BUILD_BUG_ON(BPF_REG_FP + 1 != MAX_BPF_REG);
>  	/* We don't need to worry about FP liveness because it's read-only */