linux-kernel - Re: [PATCH bpf-next 1/2] bpf: Relax KF_ACQUIRE kfuncs strict type matching constraint on non-zero offset pointers

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAADnVQ+wbFj7-s-VH=bx2MVbWJ5ea_2xdzY-mDKss1m146Ux1A@mail.gmail.com>
Date: Wed, 21 Aug 2024 11:02:05 -0700
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: Juntong Deng <juntong.deng@...look.com>
Cc: Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, 
	John Fastabend <john.fastabend@...il.com>, Andrii Nakryiko <andrii@...nel.org>, 
	Martin KaFai Lau <martin.lau@...ux.dev>, Eddy Z <eddyz87@...il.com>, Song Liu <song@...nel.org>, 
	Yonghong Song <yonghong.song@...ux.dev>, KP Singh <kpsingh@...nel.org>, 
	Stanislav Fomichev <sdf@...ichev.me>, Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>, 
	bpf <bpf@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH bpf-next 1/2] bpf: Relax KF_ACQUIRE kfuncs strict type
 matching constraint on non-zero offset pointers

On Fri, Aug 16, 2024 at 6:24 AM Juntong Deng <juntong.deng@...look.com> wrote:
>
> Currently the non-zero offset pointer constraint for KF_TRUSTED_ARGS
> kfuncs has been relaxed in commit 605c96997d89 ("bpf: relax zero fixed
> offset constraint on KF_TRUSTED_ARGS/KF_RCU"), which means that non-zero
> offset does not affect whether a pointer is valid.
>
> But currently we still cannot pass non-zero offset pointers to
> KF_ACQUIRE kfuncs. This is because KF_ACQUIRE kfuncs requires strict
> type matching, but non-zero offset does not change the type of pointer,
> which causes the ebpf program to be rejected by the verifier.
>
> This can cause some problems, one example is that bpf_skb_peek_tail
> kfunc [0] cannot be implemented by just passing in non-zero offset
> pointers.
>
> This patch makes KF_ACQUIRE kfuncs not require strict type matching
> on non-zero offset pointers.
>
> [0]: https://lore.kernel.org/bpf/AM6PR03MB5848CA39CB4B7A4397D380B099B12@AM6PR03MB5848.eurprd03.prod.outlook.com/
>
> Signed-off-by: Juntong Deng <juntong.deng@...look.com>
> ---
>  kernel/bpf/verifier.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index ebec74c28ae3..3a14002d24a0 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -11484,7 +11484,7 @@ static int process_kf_arg_ptr_to_btf_id(struct bpf_verifier_env *env,
>          * btf_struct_ids_match() to walk the struct at the 0th offset, and
>          * resolve types.
>          */
> -       if (is_kfunc_acquire(meta) ||
> +       if ((is_kfunc_acquire(meta) && !reg->off) ||

Agree that relaxing is fine and calling acquire kfunc like:
  bpf_kfunc_nested_acquire_test(&sk->sk_write_queue);

should be allowed,
but above check is strange, since
if offsetof(&sk_write_queue) == 0
it will disallow calling a kfunc.
I mean if the field is the first in the outer struct this
condition will force strict type match which will fail, right?

So should we remove the above is_kfunc_acquire() check instead?

pw-bot: cr