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 */
