netdev - Re: [PATCH bpf-next] bpf: Prepare btf_ctx_access for non raw

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: <f229a628-52fc-2728-6dd8-1c47a4962201@fb.com>
Date:   Thu, 24 Oct 2019 21:39:47 +0000
From:   Alexei Starovoitov <ast@...com>
To:     Martin Lau <kafai@...com>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     Daniel Borkmann <daniel@...earbox.net>,
        David Miller <davem@...emloft.net>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH bpf-next] bpf: Prepare btf_ctx_access for non raw_tp use
 case.

On 10/24/19 1:15 PM, Martin KaFai Lau wrote:
> This patch makes a few changes to btf_ctx_access() to prepare
> it for non raw_tp use case.
> 
> btf_ctx_access() only needs the attach_btf_id from prog.  Hence, this patch
> only passes the attach_btf_id instead of passing prog.  It allows other
> use cases when the prog->aux->attach_btf_id may not be a typedef.
> For example, in the future, a bpf_prog can attach to
> "struct tcp_congestion_ops" and its attach_btf_id is
> pointing to the btf_id of "struct tcp_congestion_ops".
> 
> While at it, allow btf_ctx_access to directly take a BTF_KIND_FUNC_PROTO
> btf_id.  It is to prepare for a later patch that does not need a "typedef"
> to figure out the func_proto.  For example, when attaching a bpf_prog
> to an ops in "struct tcp_congestion_ops", the func_proto can be
> found directly by following the members of "struct tcp_congestion_ops".
> 
> For the no typedef use case, there is no extra first arg.  Hence, this
> patch only limits the skip arg logic to raw_tp only.
> 
> Since a BTF_KIND_FUNC_PROTO type does not have a name (i.e. "(anon)"),
> an optional name arg is added also.  If specified, this name will be used
> in the bpf_log() message instead of the type's name obtained from btf_id.
> For example, the function pointer member name of
> "struct tcp_congestion_ops" can be used.
> 
> Signed-off-by: Martin KaFai Lau <kafai@...com>

...

> +static bool btf_type_is_typedef(const struct btf_type *t)
> +{
> +	return BTF_INFO_KIND(t->info) == BTF_KIND_TYPEDEF;
> +}

yeah. I was to lazy to add it. Thanks for doing this cleanup.

> @@ -3459,37 +3464,49 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>   	}
>   
>   	t = btf_type_by_id(btf_vmlinux, btf_id);
> -	if (!t || BTF_INFO_KIND(t->info) != BTF_KIND_TYPEDEF) {
> +	if (!t || (!btf_type_is_typedef(t) && !btf_type_is_func_proto(t))) {
>   		bpf_log(log, "btf_id is invalid\n");
>   		return false;
>   	}
>   
>   	tname = __btf_name_by_offset(btf_vmlinux, t->name_off);
> -	if (strncmp(prefix, tname, sizeof(prefix) - 1)) {
> -		bpf_log(log, "btf_id points to wrong type name %s\n", tname);
> -		return false;
> +	if (btf_type_is_typedef(t)) {

This check cannot be done conditionally.
Otherwise raw_tp bpf prog with partially "invalid" attach_btf_id
will successfully load, but will fail to attach to raw_tp
in raw_tp_open command.
I think better way is to move this check early into the verifier.
That will speed up this function as well that is called
multiple times for ever ctx access in the prog.
Thanks!