netdev - Re: [RFC PATCH v8 03/20] bpf: Allow struct_ops prog to return referenced kptr

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]

Date: Thu, 16 May 2024 22:30:32 -0700
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Amery Hung <ameryhung@...il.com>
Cc: bpf@...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, netdev@...r.kernel.org
Subject: Re: [RFC PATCH v8 03/20] bpf: Allow struct_ops prog to return
 referenced kptr

On 5/16/24 7:06 PM, Amery Hung wrote:
> Arguments and the return for helpers and kfuncs, where we transition
> between bpf program and kernel, can be tagged, so that we can do
> proper checks. struct_ops shares the similar property in that sense,
> but currently lacks the ability to tag the return.
> 
> A discussion was that, here we assume the returning referenced kptr
> "may be null" because that's what Qdisc expects. 

As a return value of a kernel function, it usually needs to return error. I was 
thinking "may be null" is actually the common case if it is not ERR_PTR.

> I think it would make sense to only allow it when the return is explicitly
> tagged with MAY_BE_NULL. How about doing so in the stub function name?

I think this is something internal and qdisc is the only case now. The default 
is something that could be improved later as a followup and not necessary a blocker?

But, yeah, if it is obvious to make the return ptr expectation/tagging 
consistent to how __nullable means to the argument, it would be nice. Tagging 
the stub function name like "__ret_null"? That should work. I think it will need 
some plumbing from bpf_struct_ops.c to the verifier here.