diff mbox series

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

Message ID 20210706182635.37501-1-colin.king@canonical.com
State New
Headers show
Series [bpf,v2,1/2] bpf, x86: Validate computation of branch displacements for x86-64 | expand

Commit Message

Colin Ian King July 6, 2021, 6:26 p.m. UTC
From: Piotr Krysiuk <piotras@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@gmail.com>
---
 arch/x86/net/bpf_jit_comp.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Colin Ian King July 6, 2021, 6:27 p.m. UTC | #1
Igore, I fat fingered a git send.

Colin

On 06/07/2021 19:26, Colin King wrote:
> From: Piotr Krysiuk <piotras@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@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;
>  			}
>
diff mbox series

Patch

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;
 			}