Re: [PATCH v2 bpf-next 11/13] bpf: Mark the arguments of fmod_ret progs as PTR_UNTRUSTED

[Anton Protopopov <a.s.protopopov@gmail.com>]

On 26/11/13 05:08AM, Alexei Starovoitov wrote:
> From: Alexei Starovoitov <ast@kernel.org>
> 
> fmod_ret attaches to about 12000 kernel functions now. Nobody looked at
> what these functions get. A pointer argument can be NULL, an ERR_PTR(),
> stale, or an object that is not set up yet.
> 
> 	SEC("fmod_ret/tcp_v4_connect")
> 	int BPF_PROG(guard, struct sock *sk, struct sockaddr_unsized *addr,
> 		     int addr_len, int ret)
> 	{
> 		if (bpf_get_socket_cookie(sk) == cookie)
> 
> sk is PTR_TO_BTF_ID without a flag. The verifier does not rely on such
> a pointer: each load through it is BPF_PROBE_MEM, stores are refused,
> kfuncs want a trusted pointer. But helpers take it, for compatibility, and
> dereference it.
> 
> PTR_UNTRUSTED is the flag of a pointer that nobody vouches for. Such
> a pointer can do what PTR_TO_BTF_ID without a flag can, except that
> helpers refuse it:
> 
>   R1 type=untrusted_ptr_ expected=sock_common, sock, tcp_sock, ..
> 
> What is loaded through an untrusted pointer is untrusted too, also a __user
> or a __percpu member.
> 
> That holds for each fmod_ret prog, also on a target that is allowed today:
> security_*(), a syscall, the error injection list. A prog there does not
> load any more if a helper that expects a kernel object gets an argument:
> 
> 	SEC("fmod_ret/security_file_open")
> 		bpf_d_path(&file->f_path, buf, sizeof(buf));
> 
> bpf_probe_read_kernel() takes each address, as before. All fmod_ret progs
> of the selftests, Tetragon, runtime-enforcer and bad-bpf load as before.
> 
> The arguments of fentry and fexit progs keep their type for now.

I am not 100% following why this change is needed without enforcing
this for fentry as well. One can attach fentry before fmodret and
follow all the same args. (But in general, +1 for this and the
following patches.)

> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---
>  kernel/bpf/btf.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 228b93d4f8c9..4ed660dc4612 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -7618,6 +7618,8 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>  	info->reg_type = PTR_TO_BTF_ID;
>  	if (prog_args_trusted(prog))
>  		info->reg_type |= PTR_TRUSTED;
> +	else if (prog->expected_attach_type == BPF_MODIFY_RETURN)
> +		info->reg_type |= PTR_UNTRUSTED;
>  
>  	if (btf_param_match_suffix(btf, &args[arg], "__nullable"))
>  		info->reg_type |= PTR_MAYBE_NULL;
> -- 
> 2.55.0
>