diff mbox series

[SRU,Bionic,1/1] bpf: Fix mask direction swap upon off reg sign change

Message ID 20210624001439.3106740-2-cascardo@canonical.com
State New
Headers show
Series CVE-2021-33200 | expand

Commit Message

Thadeu Lima de Souza Cascardo June 24, 2021, 12:14 a.m. UTC 
From: Daniel Borkmann <daniel@iogearbox.net>

Masking direction as indicated via mask_to_left is considered to be
calculated once and then used to derive pointer limits. Thus, this
needs to be placed into bpf_sanitize_info instead so we can pass it
to sanitize_ptr_alu() call after the pointer move. Piotr noticed a
corner case where the off reg causes masking direction change which
then results in an incorrect final aux->alu_limit.

Fixes: 7fedb63a8307 ("bpf: Tighten speculative pointer arithmetic mask")
Reported-by: Piotr Krysiuk <piotras@gmail.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Reviewed-by: Piotr Krysiuk <piotras@gmail.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
(cherry picked from commit bb01a1bba579b4b1c5566af24d95f1767859771e)
CVE-2021-33200
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
---
 kernel/bpf/verifier.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

Comments

Stefan Bader June 24, 2021, 7:31 a.m. UTC | #1 
On 24.06.21 02:14, Thadeu Lima de Souza Cascardo wrote:
> From: Daniel Borkmann <daniel@iogearbox.net>
> 
> Masking direction as indicated via mask_to_left is considered to be
> calculated once and then used to derive pointer limits. Thus, this
> needs to be placed into bpf_sanitize_info instead so we can pass it
> to sanitize_ptr_alu() call after the pointer move. Piotr noticed a
> corner case where the off reg causes masking direction change which
> then results in an incorrect final aux->alu_limit.
> 
> Fixes: 7fedb63a8307 ("bpf: Tighten speculative pointer arithmetic mask")
> Reported-by: Piotr Krysiuk <piotras@gmail.com>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Reviewed-by: Piotr Krysiuk <piotras@gmail.com>
> Acked-by: Alexei Starovoitov <ast@kernel.org>
> (cherry picked from commit bb01a1bba579b4b1c5566af24d95f1767859771e)
> CVE-2021-33200
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
> ---
>   kernel/bpf/verifier.c | 22 ++++++++++++----------
>   1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 60952535b626..daaec44afd04 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -1992,18 +1992,10 @@ enum {
>   };
>   
>   static int retrieve_ptr_limit(const struct bpf_reg_state *ptr_reg,
> -			      const struct bpf_reg_state *off_reg,
> -			      u32 *alu_limit, u8 opcode)
> +			      u32 *alu_limit, bool mask_to_left)
>   {
> -	bool off_is_neg = off_reg->smin_value < 0;
> -	bool mask_to_left = (opcode == BPF_ADD &&  off_is_neg) ||
> -			    (opcode == BPF_SUB && !off_is_neg);
>   	u32 max = 0, ptr_limit = 0;
>   
> -	if (!tnum_is_const(off_reg->var_off) &&
> -	    (off_reg->smin_value < 0) != (off_reg->smax_value < 0))
> -		return REASON_BOUNDS;
> -
>   	switch (ptr_reg->type) {
>   	case PTR_TO_STACK:
>   		/* Offset 0 is out-of-bounds, but acceptable start for the
> @@ -2071,6 +2063,7 @@ static bool sanitize_needed(u8 opcode)
>   
>   struct bpf_sanitize_info {
>   	struct bpf_insn_aux_data aux;
> +	bool mask_to_left;
>   };
>   
>   static int sanitize_ptr_alu(struct bpf_verifier_env *env,
> @@ -2102,7 +2095,16 @@ static int sanitize_ptr_alu(struct bpf_verifier_env *env,
>   	if (vstate->speculative)
>   		goto do_sim;
>   
> -	err = retrieve_ptr_limit(ptr_reg, off_reg, &alu_limit, opcode);
> +	if (!commit_window) {
> +		if (!tnum_is_const(off_reg->var_off) &&
> +		    (off_reg->smin_value < 0) != (off_reg->smax_value < 0))
> +			return REASON_BOUNDS;
> +
> +		info->mask_to_left = (opcode == BPF_ADD &&  off_is_neg) ||
> +				     (opcode == BPF_SUB && !off_is_neg);
> +	}
> +
> +	err = retrieve_ptr_limit(ptr_reg, &alu_limit, info->mask_to_left);
>   	if (err < 0)
>   		return err;
>   
>
Kleber Sacilotto de Souza June 24, 2021, 8:30 a.m. UTC | #2 
On 24.06.21 02:14, Thadeu Lima de Souza Cascardo wrote:
> From: Daniel Borkmann <daniel@iogearbox.net>
> 
> Masking direction as indicated via mask_to_left is considered to be
> calculated once and then used to derive pointer limits. Thus, this
> needs to be placed into bpf_sanitize_info instead so we can pass it
> to sanitize_ptr_alu() call after the pointer move. Piotr noticed a
> corner case where the off reg causes masking direction change which
> then results in an incorrect final aux->alu_limit.
> 
> Fixes: 7fedb63a8307 ("bpf: Tighten speculative pointer arithmetic mask")
> Reported-by: Piotr Krysiuk <piotras@gmail.com>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Reviewed-by: Piotr Krysiuk <piotras@gmail.com>
> Acked-by: Alexei Starovoitov <ast@kernel.org>
> (cherry picked from commit bb01a1bba579b4b1c5566af24d95f1767859771e)
> CVE-2021-33200
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>

Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>

Thanks

> ---
>   kernel/bpf/verifier.c | 22 ++++++++++++----------
>   1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 60952535b626..daaec44afd04 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -1992,18 +1992,10 @@ enum {
>   };
>   
>   static int retrieve_ptr_limit(const struct bpf_reg_state *ptr_reg,
> -			      const struct bpf_reg_state *off_reg,
> -			      u32 *alu_limit, u8 opcode)
> +			      u32 *alu_limit, bool mask_to_left)
>   {
> -	bool off_is_neg = off_reg->smin_value < 0;
> -	bool mask_to_left = (opcode == BPF_ADD &&  off_is_neg) ||
> -			    (opcode == BPF_SUB && !off_is_neg);
>   	u32 max = 0, ptr_limit = 0;
>   
> -	if (!tnum_is_const(off_reg->var_off) &&
> -	    (off_reg->smin_value < 0) != (off_reg->smax_value < 0))
> -		return REASON_BOUNDS;
> -
>   	switch (ptr_reg->type) {
>   	case PTR_TO_STACK:
>   		/* Offset 0 is out-of-bounds, but acceptable start for the
> @@ -2071,6 +2063,7 @@ static bool sanitize_needed(u8 opcode)
>   
>   struct bpf_sanitize_info {
>   	struct bpf_insn_aux_data aux;
> +	bool mask_to_left;
>   };
>   
>   static int sanitize_ptr_alu(struct bpf_verifier_env *env,
> @@ -2102,7 +2095,16 @@ static int sanitize_ptr_alu(struct bpf_verifier_env *env,
>   	if (vstate->speculative)
>   		goto do_sim;
>   
> -	err = retrieve_ptr_limit(ptr_reg, off_reg, &alu_limit, opcode);
> +	if (!commit_window) {
> +		if (!tnum_is_const(off_reg->var_off) &&
> +		    (off_reg->smin_value < 0) != (off_reg->smax_value < 0))
> +			return REASON_BOUNDS;
> +
> +		info->mask_to_left = (opcode == BPF_ADD &&  off_is_neg) ||
> +				     (opcode == BPF_SUB && !off_is_neg);
> +	}
> +
> +	err = retrieve_ptr_limit(ptr_reg, &alu_limit, info->mask_to_left);
>   	if (err < 0)
>   		return err;
>   
>
diff mbox series

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 60952535b626..daaec44afd04 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -1992,18 +1992,10 @@  enum {
 };
 
 static int retrieve_ptr_limit(const struct bpf_reg_state *ptr_reg,
-			      const struct bpf_reg_state *off_reg,
-			      u32 *alu_limit, u8 opcode)
+			      u32 *alu_limit, bool mask_to_left)
 {
-	bool off_is_neg = off_reg->smin_value < 0;
-	bool mask_to_left = (opcode == BPF_ADD &&  off_is_neg) ||
-			    (opcode == BPF_SUB && !off_is_neg);
 	u32 max = 0, ptr_limit = 0;
 
-	if (!tnum_is_const(off_reg->var_off) &&
-	    (off_reg->smin_value < 0) != (off_reg->smax_value < 0))
-		return REASON_BOUNDS;
-
 	switch (ptr_reg->type) {
 	case PTR_TO_STACK:
 		/* Offset 0 is out-of-bounds, but acceptable start for the
@@ -2071,6 +2063,7 @@  static bool sanitize_needed(u8 opcode)
 
 struct bpf_sanitize_info {
 	struct bpf_insn_aux_data aux;
+	bool mask_to_left;
 };
 
 static int sanitize_ptr_alu(struct bpf_verifier_env *env,
@@ -2102,7 +2095,16 @@  static int sanitize_ptr_alu(struct bpf_verifier_env *env,
 	if (vstate->speculative)
 		goto do_sim;
 
-	err = retrieve_ptr_limit(ptr_reg, off_reg, &alu_limit, opcode);
+	if (!commit_window) {
+		if (!tnum_is_const(off_reg->var_off) &&
+		    (off_reg->smin_value < 0) != (off_reg->smax_value < 0))
+			return REASON_BOUNDS;
+
+		info->mask_to_left = (opcode == BPF_ADD &&  off_is_neg) ||
+				     (opcode == BPF_SUB && !off_is_neg);
+	}
+
+	err = retrieve_ptr_limit(ptr_reg, &alu_limit, info->mask_to_left);
 	if (err < 0)
 		return err;