NACK: [PATCH bpf v2 1/2] bpf, x86: Validate computation of branch displacements for x86-64

[Colin Ian King]

Igore, I fat fingered a git send.

Colin

On 06/07/2021 19:26, Colin King wrote:
> From: Piotr Krysiuk <piotras at gmail.com>
> 
> The branch displacement logic in the BPF JIT compilers for x86 assumes
> that, for any generated branch instruction, the distance cannot
> increase between optimization passes.
> 
> But this assumption can be violated due to how the distances are
> computed. Specifically, whenever a backward branch is processed in
> do_jit(), the distance is computed by subtracting the positions in the
> machine code from different optimization passes. This is because part
> of addrs[] is already updated for the current optimization pass, before
> the branch instruction is visited.
> 
> And so the optimizer can expand blocks of machine code in some cases.
> 
> This can confuse the optimizer logic, where it assumes that a fixed
> point has been reached for all machine code blocks once the total
> program size stops changing. And then the JIT compiler can output
> abnormal machine code containing incorrect branch displacements.
> 
> To mitigate this issue, we assert that a fixed point is reached while
> populating the output image. This rejects any problematic programs.
> 
> The issue affects both x86-32 and x86-64. We mitigate separately to
> ease backporting.
> 
> Signed-off-by: Piotr Krysiuk <piotras at gmail.com>
> ---
>  arch/x86/net/bpf_jit_comp.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 023ac12f54a2..f104c36f1e79 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -1476,7 +1476,15 @@ xadd:			if (is_imm8(insn->off))
>  		}
>  
>  		if (image) {
> -			if (unlikely(proglen + ilen > oldproglen)) {
> +			/*
> +			 * When populating the image, assert that
> +			 * i) we do not write beyond the allocated space, and
> +			 * ii) addrs[i] did not change from the prior run, in order
> +			 *     to validate assumptions made for computing branch
> +			 *     displacements
> +			 */
> +			if (unlikely(proglen + ilen > oldproglen ||
> +				     proglen + ilen != addrs[i])) {
>  				pr_err("bpf_jit: fatal error\n");
>  				return -EFAULT;
>  			}
>