netdev - Re: [RFC PATCH v9 01/11] bpf: Support getting referenced kptr from struct

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 for Android: free password hash cracker in your pocket

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

Message-ID: <4582b8db-e59d-4ca0-9a0e-4d0f21a1af66@linux.dev>
Date: Wed, 24 Jul 2024 18:28:51 -0700
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Amery Hung <ameryhung@...il.com>, alexei.starovoitov@...il.com
Cc: bpf@...r.kernel.org, netdev@...r.kernel.org, yangpeihao@...u.edu.cn,
 daniel@...earbox.net, andrii@...nel.org, martin.lau@...nel.org,
 sinquersw@...il.com, toke@...hat.com, jhs@...atatu.com, jiri@...nulli.us,
 sdf@...gle.com, xiyou.wangcong@...il.com, yepeilin.cs@...il.com
Subject: Re: [RFC PATCH v9 01/11] bpf: Support getting referenced kptr from
 struct_ops argument

On 7/24/24 10:00 AM, Amery Hung wrote:
> On Tue, Jul 23, 2024 at 5:32 PM Martin KaFai Lau <martin.lau@...ux.dev> wrote:
>>
>> On 7/14/24 10:51 AM, Amery Hung wrote:
>>> @@ -21004,6 +21025,13 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
>>>                mark_reg_known_zero(env, regs, BPF_REG_1);
>>>        }
>>>
>>> +     if (env->prog->type == BPF_PROG_TYPE_STRUCT_OPS) {
>>> +             ctx_arg_info = (struct bpf_ctx_arg_aux *)env->prog->aux->ctx_arg_info;
>>> +             for (i = 0; i < env->prog->aux->ctx_arg_info_size; i++)
>>> +                     if (ctx_arg_info[i].refcounted)
>>> +                             ctx_arg_info[i].ref_obj_id = acquire_reference_state(env, 0);
>>> +     }
>>> +
>>
>> I think this will miss a case when passing the struct_ops prog ctx (i.e. "__u64
>> *ctx") to a global subprog. Something like this:
>>
>> __noinline int subprog_release(__u64 *ctx __arg_ctx)
>> {
>>          struct task_struct *task = (struct task_struct *)ctx[1];
>>          int dummy = (int)ctx[0];
>>
>>          bpf_task_release(task);
>>
>>          return dummy + 1;
>> }
>>
>> SEC("struct_ops/subprog_ref")
>> __failure
>> int test_subprog_ref(__u64 *ctx)
>> {
>>          struct task_struct *task = (struct task_struct *)ctx[1];
>>
>>          bpf_task_release(task);
>>
>>          return subprog_release(ctx);;
>> }
>>
>> SEC(".struct_ops.link")
>> struct bpf_testmod_ops subprog_ref = {
>>          .test_refcounted = (void *)test_subprog_ref,
>> };
>>
> 
> Thanks for pointing this out. The test did failed.
> 
>> A quick thought is, I think tracking the ctx's ref id in the env->cur_state may
>> not be the correct place.
> 
> I think it is a bit tricky because subprogs are checked independently
> and their state is folded (i.e., there can be multiple edges from the
> main program to a subprog).
> 
> Maybe the verifier can rewrite the program: set the refcounted ctx to
> NULL when releasing reference. Then, in do_check_common(), if it is a
> global subprog, we mark refcounted ctx as PTR_MAYBE_NULL to force a
> runtime check. How does it sound?

don't know how to get the ctx pointer to patch the code. It is not always in r1.

A case like this should still break even with the PTR_MAYBE_NULL marking in all 
main and subprog (I haven't tried this one myself):

SEC("struct_ops/subprog_ref")
int test_subprog_ref(__u64 *ctx)
{
	struct task_struct *task = (struct task_struct *)ctx[1];

	if (task) {
		subprog_release(ctx);
		bpf_task_release(task);
	}

	return;
}

afaik, the global subprog is checked independently from the main prog and it 
does not know the state of the main prog. Take a look at the subprog_is_global() 
case in the check_func_call().

How about only acquire_reference_state() for the main prog? Yes, the global 
subprog cannot do the bpf_kptr_xchg() and bpf_qdisc_skb_drop() but it can still 
read the skb. The non-global subprog (static) should work though (please test).

I don't have other better idea. May be Alexei can provide some guidance here?