netdev - Re: [PATCH v11 bpf-next 09/10] bpf: Add bpf_dynptr_slice and bpf_dynptr_slice

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: <20230222203214.5e33mmjzljoaowbq@MacBook-Pro-6.local>
Date:   Wed, 22 Feb 2023 12:32:26 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Joanne Koong <joannelkoong@...il.com>
Cc:     bpf@...r.kernel.org, martin.lau@...nel.org, andrii@...nel.org,
        ast@...nel.org, memxor@...il.com, daniel@...earbox.net,
        netdev@...r.kernel.org, kernel-team@...com, toke@...nel.org
Subject: Re: [PATCH v11 bpf-next 09/10] bpf: Add bpf_dynptr_slice and
 bpf_dynptr_slice_rdwr

On Tue, Feb 21, 2023 at 10:07:46PM -0800, Joanne Koong wrote:
> @@ -9975,8 +10023,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>  		}
>  	}
>  
> -	for (i = 0; i < CALLER_SAVED_REGS; i++)
> -		mark_reg_not_init(env, regs, caller_saved[i]);
> +	mark_reg_not_init(env, regs, caller_saved[BPF_REG_0]);

...

>  	/* Check return type */
>  	t = btf_type_skip_modifiers(desc_btf, func_proto->type, NULL);
> @@ -10062,6 +10109,41 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>  				regs[BPF_REG_0].type = PTR_TO_BTF_ID | PTR_UNTRUSTED;
>  				regs[BPF_REG_0].btf = desc_btf;
>  				regs[BPF_REG_0].btf_id = meta.arg_constant.value;
> +			} else if (meta.func_id == special_kfunc_list[KF_bpf_dynptr_slice] ||
> +				   meta.func_id == special_kfunc_list[KF_bpf_dynptr_slice_rdwr]) {
> +				enum bpf_type_flag type_flag = get_dynptr_type_flag(meta.initialized_dynptr.type);
> +
> +				mark_reg_known_zero(env, regs, BPF_REG_0);
> +
> +				if (!tnum_is_const(regs[BPF_REG_4].var_off)) {
> +					verbose(env, "mem_size must be a constant\n");
> +					return -EINVAL;
> +				}
> +				regs[BPF_REG_0].mem_size = regs[BPF_REG_4].var_off.value;

This is a bit fragile.
Instead of moving above 'for' may be let's use meta.arg_constant approach ?
Just doing:
if (is_kfunc_arg_constant(meta->btf, &args[i]) || is_kfunc_arg_mem_size(...))
won't quite work, since we'll be doing mark_chain_precision() twice.
(check_kfunc_mem_size_reg() will do it 2nd time).

Another issue is that __sz allows var_off, but bpf_dynptr_slice() needs tnum_is_const
and the patch is doing it explicitly.
Ideally some combination of __sz and __k is needed.

I don't like our suffix logic, but maybe let's do quick "__szk" for now ?
Then it can do normal __sz check and in addition do tnum_is_const() check
and set meta.arg_constant ?

Then 'for' will stay in place and regs[BPF_REG_4] won't be needed.

> +
> +				/* PTR_MAYBE_NULL will be added when is_kfunc_ret_null is checked */
> +				regs[BPF_REG_0].type = PTR_TO_MEM | type_flag;
> +
> +				if (meta.func_id == special_kfunc_list[KF_bpf_dynptr_slice]) {
> +					regs[BPF_REG_0].type |= MEM_RDONLY;
> +				} else {
> +					/* this will set env->seen_direct_write to true */
> +					if (!may_access_direct_pkt_data(env, NULL, BPF_WRITE)) {
> +						verbose(env, "the prog does not allow writes to packet data\n");
> +						return -EINVAL;
> +					}
> +				}
> +
> +				if (!meta.initialized_dynptr.id) {
> +					verbose(env, "verifier internal error: no dynptr id\n");
> +					return -EFAULT;
> +				}
> +				regs[BPF_REG_0].dynptr_id = meta.initialized_dynptr.id;
> +
> +				/* we don't need to set BPF_REG_0's ref obj id
> +				 * because packet slices are not refcounted (see
> +				 * dynptr_type_refcounted)
> +				 */
>  			} else {
>  				verbose(env, "kernel function %s unhandled dynamic return type\n",
>  					meta.func_name);
> @@ -10121,6 +10203,9 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>  			regs[BPF_REG_0].id = ++env->id_gen;
>  	} /* else { add_kfunc_call() ensures it is btf_type_is_void(t) } */
>  
> +	for (i = BPF_REG_1; i < CALLER_SAVED_REGS; i++)
> +		mark_reg_not_init(env, regs, caller_saved[i]);
> +