netdev - Re: [PATCH net-next v5 05/15] net-timestamp: add strict check in some BPF calls

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: <35e2c693-244f-4d55-88f3-99e1ed1e2745@linux.dev>
Date: Fri, 17 Jan 2025 18:15:26 -0800
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Jason Xing <kerneljasonxing@...il.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
 pabeni@...hat.com, dsahern@...nel.org, willemdebruijn.kernel@...il.com,
 willemb@...gle.com, ast@...nel.org, daniel@...earbox.net, andrii@...nel.org,
 eddyz87@...il.com, song@...nel.org, yonghong.song@...ux.dev,
 john.fastabend@...il.com, kpsingh@...nel.org, sdf@...ichev.me,
 haoluo@...gle.com, jolsa@...nel.org, horms@...nel.org, bpf@...r.kernel.org,
 netdev@...r.kernel.org
Subject: Re: [PATCH net-next v5 05/15] net-timestamp: add strict check in some
 BPF calls

On 1/15/25 3:32 PM, Jason Xing wrote:
>> +static bool is_locked_tcp_sock_ops(struct bpf_sock_ops_kern *bpf_sock)
>> +{
>> +       return bpf_sock->op <= BPF_SOCK_OPS_WRITE_HDR_OPT_CB;
> 
> I wonder if I can use the code snippets in the previous reply in this
> thread, only checking if we are in the timestamping callback?
> +#define BPF_SOCK_OPTS_TS               (BPF_SOCK_OPS_TS_SCHED_OPT_CB | \
> +                                        BPF_SOCK_OPS_TS_SW_OPT_CB | \
> +                                        BPF_SOCK_OPS_TS_ACK_OPT_CB | \
> +                                        BPF_SOCK_OPS_TS_TCP_SND_CB)

Note that BPF_SOCK_OPS_*_CB is not a bit.

My understanding is it is a blacklist. Please correct me if I miss-interpret the 
intention.

> 
> Then other developers won't worry too much whether they will cause
> some safety problems. If not, they will/must add callbacks earlier
> than BPF_SOCK_OPS_WRITE_HDR_OPT_CB.

It can't be added earlier because it is in uapi. If the future new cb is safe to 
use these helpers, then it needs to adjust the BPF_SOCK_OPS_WRITE_HDR_OPT_CB 
check. is_locked_tcp_sock_ops() is a whitelist. The worst is someone will 
discover the helpers are not usable in the new cb, so no safety issue.

If forgot to adjust the blacklist and the new cb should not use the helpers, 
then it is a safety issue.

Anyhow, I don't have a strong opinion here. I did think about checking the new 
TS callback instead. I went with the simplest way in the code and also 
considering the BPF_SOCK_OPS_TS_*_CB is only introduced starting from patch 7.