Re: [PATCH bpf-next v1] bpf: Fix out-of-bounds read in check_atomic_load/store()

[Kohei Enju <enjuk@amazon.com>]

>> ...
>>>  kernel/bpf/verifier.c | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>> 
>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>> index 3303a3605ee8..6481604ab612 100644
>>> --- a/kernel/bpf/verifier.c
>>> +++ b/kernel/bpf/verifier.c
>>> @@ -7788,6 +7788,12 @@ static int check_atomic_rmw(struct bpf_verifier_env *env,
>>>  static int check_atomic_load(struct bpf_verifier_env *env,
>>>  			     struct bpf_insn *insn)
>>>  {
>>> +	int err;
>>> +
>>> +	err = check_reg_arg(env, insn->src_reg, SRC_OP);
>>> +	if (err)
>>> +		return err;
>>> +
>>
>>I agree with these changes, however, both check_load_mem() and
>>check_store_reg() already do check_reg_arg() first thing.
>>Maybe just swap the atomic_ptr_type_ok() and check_load_mem()?
>
>You're absolutely right. The additional check_reg_arg() introduces some 
>redundancy since check_load_mem() and check_store_reg() do that.

IMHO, in this specific case, I believe OOB is caused by invalid register 
number in reg_state(), so check_reg_arg() is too much and instead just 
checking register number before atomic_ptr_type_ok() is sufficient.

Although it's still somewhat redundant and not a fundamental solution, I 
think it would be better than doing whole register checking by 
check_reg_arg().

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 3303a3605ee8..48131839ac26 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -7788,6 +7788,11 @@ static int check_atomic_rmw(struct bpf_verifier_env *env,
 static int check_atomic_load(struct bpf_verifier_env *env,
                 struct bpf_insn *insn)
 {
+	if (insn->src_reg >= MAX_BPF_REG) {
+		verbose(env, "R%d is invalid\n", insn->src_reg);
+		return -EINVAL;
+	}
+
    if (!atomic_ptr_type_ok(env, insn->src_reg, insn)) {
        verbose(env, "BPF_ATOMIC loads from R%d %s is not allowed\n",
            insn->src_reg,
@@ -7801,6 +7806,11 @@ static int check_atomic_load(struct bpf_verifier_env *env,
 static int check_atomic_store(struct bpf_verifier_env *env,
                  struct bpf_insn *insn)
 {
+	if (insn->dst_reg >= MAX_BPF_REG) {
+		verbose(env, "R%d is invalid\n", insn->dst_reg);
+		return -EINVAL;
+	}
+
    if (!atomic_ptr_type_ok(env, insn->dst_reg, insn)) {
        verbose(env, "BPF_ATOMIC stores into R%d %s is not allowed\n",
            insn->dst_reg,

>I've revised the patch to simply swap the order and syzbot didn't trigger 
>the issue in this context.
>    https://lore.kernel.org/all/20250315055941.10487-2-enjuk@amazon.com/
> ...

Regards,
Kohei