Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Roberto Sassu <roberto.sassu@huaweicloud.com>
To:     ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org,
        martin.lau@linux.dev, song@kernel.org, yhs@fb.com,
        john.fastabend@gmail.com, kpsingh@kernel.org, sdf@google.com,
        haoluo@google.com, jolsa@kernel.org, revest@chromium.org,
        jackmanb@chromium.org, mykolal@fb.com, paul@paul-moore.com,
        jmorris@namei.org, serge@hallyn.com, shuah@kernel.org
Cc:     bpf@vger.kernel.org, linux-security-module@vger.kernel.org,
        linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org,
        Roberto Sassu <roberto.sassu@huawei.com>
Subject: [RFC][PATCH v2 2/7] bpf: Mark ALU32 operations in bpf_reg_state structure
Date:   Wed,  7 Dec 2022 18:24:29 +0100
Message-Id: <20221207172434.435893-3-roberto.sassu@huaweicloud.com>
In-Reply-To: <20221207172434.435893-1-roberto.sassu@huaweicloud.com>
References: <20221207172434.435893-1-roberto.sassu@huaweicloud.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

From: Roberto Sassu <roberto.sassu@huawei.com>

BPF LSM needs a reliable source of information to determine if the return
value given by eBPF programs is acceptable or not. At the moment, choosing
either the 64 bit or the 32 bit one does not seem to be an option
(selftests fail).

If we choose the 64 bit one, the following happens.

      14:	61 10 00 00 00 00 00 00	r0 = *(u32 *)(r1 + 0)
      15:	74 00 00 00 15 00 00 00	w0 >>= 21
      16:	54 00 00 00 01 00 00 00	w0 &= 1
      17:	04 00 00 00 ff ff ff ff	w0 += -1

This is the last part of test_deny_namespace. After #16, the register
values are:

smin_value = 0x0, smax_value = 0x1,
s32_min_value = 0x0, s32_max_value = 0x1,

After #17, they become:

smin_value = 0x0, smax_value = 0xffffffff,
s32_min_value = 0xffffffff, s32_max_value = 0x0

where only the 32 bit values are correct.

If we choose the 32 bit ones, the following happens.

0000000000000000 <check_access>:
       0:	79 12 00 00 00 00 00 00	r2 = *(u64 *)(r1 + 0)
       1:	79 10 08 00 00 00 00 00	r0 = *(u64 *)(r1 + 8)
       2:	67 00 00 00 3e 00 00 00	r0 <<= 62
       3:	c7 00 00 00 3f 00 00 00	r0 s>>= 63

This is part of test_libbpf_get_fd_by_id_opts (no_alu32 version). In this
case, 64 bit register values should be used (for the 32 bit ones, there is
no precise information from the verifier).

As the examples above suggest that which register values to use depends on
the specific case, mark ALU32 operations in bpf_reg_state structure, so
that BPF LSM can choose the proper ones.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 include/linux/bpf_verifier.h |  1 +
 kernel/bpf/verifier.c        | 10 +++++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 70d06a99f0b8..29c9cf6b0d01 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -181,6 +181,7 @@ struct bpf_reg_state {
 	enum bpf_reg_liveness live;
 	/* if (!precise && SCALAR_VALUE) min/max/tnum don't affect safety */
 	bool precise;
+	bool alu32;
 };
 
 enum bpf_stack_slot_type {
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 8c5f0adbbde3..edce85c425a2 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -10524,9 +10524,13 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env,
 		break;
 	}
 
+	dst_reg->alu32 = false;
+
 	/* ALU32 ops are zero extended into 64bit register */
-	if (alu32)
+	if (alu32) {
 		zext_32_to_64(dst_reg);
+		dst_reg->alu32 = true;
+	}
 	reg_bounds_sync(dst_reg);
 	return 0;
 }
@@ -10700,6 +10704,7 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
 				*dst_reg = *src_reg;
 				dst_reg->live |= REG_LIVE_WRITTEN;
 				dst_reg->subreg_def = DEF_NOT_SUBREG;
+				dst_reg->alu32 = false;
 			} else {
 				/* R1 = (u32) R2 */
 				if (is_pointer_value(env, insn->src_reg)) {
@@ -10716,6 +10721,7 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
 					dst_reg->id = 0;
 					dst_reg->live |= REG_LIVE_WRITTEN;
 					dst_reg->subreg_def = env->insn_idx + 1;
+					dst_reg->alu32 = true;
 				} else {
 					mark_reg_unknown(env, regs,
 							 insn->dst_reg);
@@ -10733,9 +10739,11 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
 			if (BPF_CLASS(insn->code) == BPF_ALU64) {
 				__mark_reg_known(regs + insn->dst_reg,
 						 insn->imm);
+				regs[insn->dst_reg].alu32 = false;
 			} else {
 				__mark_reg_known(regs + insn->dst_reg,
 						 (u32)insn->imm);
+				regs[insn->dst_reg].alu32 = true;
 			}
 		}
 
-- 
2.25.1