linux-kernel - Re: [PATCH bpf-next v1] bpf: Fix out-of-bounds read in check_atomic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250315130028.92105-1-enjuk@amazon.com>
Date: Sat, 15 Mar 2025 21:59:39 +0900
From: Kohei Enju <enjuk@...zon.com>
To: <enjuk@...zon.com>
CC: <andrii@...nel.org>, <ast@...nel.org>, <bpf@...r.kernel.org>,
	<daniel@...earbox.net>, <eddyz87@...il.com>, <haoluo@...gle.com>,
	<iii@...ux.ibm.com>, <john.fastabend@...il.com>, <jolsa@...nel.org>,
	<kohei.enju@...il.com>, <kpsingh@...nel.org>, <kuniyu@...zon.com>,
	<linux-kernel@...r.kernel.org>, <martin.lau@...ux.dev>, <sdf@...ichev.me>,
	<song@...nel.org>, <syzbot+a5964227adc0f904549c@...kaller.appspotmail.com>,
	<yepeilin@...gle.com>, <yonghong.song@...ux.dev>
Subject: Re: [PATCH bpf-next v1] bpf: Fix out-of-bounds read in check_atomic_load/store()

>> ...
>>>  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