linux-kernel - Re: Re: [PATCH bpf-next] bpf: getsockopt hook to get optval without checking kernel retval

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: <20881602-9afc-96b7-3d58-51c31e3f50b7@bytedance.com>
Date:   Thu, 1 Jun 2023 14:05:31 +0800
From:   Feng Zhou <zhoufeng.zf@...edance.com>
To:     Martin KaFai Lau <martin.lau@...ux.dev>
Cc:     bpf@...r.kernel.org, linux-kernel@...r.kernel.org,
        yangzhenze@...edance.com, wangdongdong.6@...edance.com,
        ast@...nel.org, daniel@...earbox.net, andrii@...nel.org,
        song@...nel.org, yhs@...com, john.fastabend@...il.com,
        kpsingh@...nel.org, sdf@...gle.com, haoluo@...gle.com,
        jolsa@...nel.org
Subject: Re: Re: [PATCH bpf-next] bpf: getsockopt hook to get optval without
 checking kernel retval

在 2023/6/1 13:37, Martin KaFai Lau 写道:
> On 5/31/23 7:49 PM, Feng zhou wrote:
>> From: Feng Zhou <zhoufeng.zf@...edance.com>
>>
>> Remove the judgment on retval and pass bpf ctx by default. The
>> advantage of this is that it is more flexible. Bpf getsockopt can
>> support the new optname without using the module to call the
>> nf_register_sockopt to register.
>>
>> Signed-off-by: Feng Zhou <zhoufeng.zf@...edance.com>
>> ---
>>   kernel/bpf/cgroup.c | 35 +++++++++++++----------------------
>>   1 file changed, 13 insertions(+), 22 deletions(-)
>>
>> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
>> index 5b2741aa0d9b..ebad5442d8bb 100644
>> --- a/kernel/bpf/cgroup.c
>> +++ b/kernel/bpf/cgroup.c
>> @@ -1896,30 +1896,21 @@ int __cgroup_bpf_run_filter_getsockopt(struct 
>> sock *sk, int level,
>>       if (max_optlen < 0)
>>           return max_optlen;
>> -    if (!retval) {
>> -        /* If kernel getsockopt finished successfully,
>> -         * copy whatever was returned to the user back
>> -         * into our temporary buffer. Set optlen to the
>> -         * one that kernel returned as well to let
>> -         * BPF programs inspect the value.
>> -         */
>> -
>> -        if (get_user(ctx.optlen, optlen)) {
>> -            ret = -EFAULT;
>> -            goto out;
>> -        }
>> +    if (get_user(ctx.optlen, optlen)) {
>> +        ret = -EFAULT;
>> +        goto out;
>> +    }
>> -        if (ctx.optlen < 0) {
>> -            ret = -EFAULT;
>> -            goto out;
>> -        }
>> -        orig_optlen = ctx.optlen;
>> +    if (ctx.optlen < 0) {
>> +        ret = -EFAULT;
>> +        goto out;
>> +    }
>> +    orig_optlen = ctx.optlen;
>> -        if (copy_from_user(ctx.optval, optval,
>> -                   min(ctx.optlen, max_optlen)) != 0) {
>> -            ret = -EFAULT;
>> -            goto out;
>> -        }
>> +    if (copy_from_user(ctx.optval, optval,
>> +                min(ctx.optlen, max_optlen)) != 0) {
> What is in optval that is useful to copy from if the kernel didn't 
> handle the optname?

For example, if the user customizes a new optname, it will not be 
processed if the kernel does not support it. Then the data stored in 
optval is the data put by the user. If this part can be seen by bpf 
prog, the user can implement processing logic of the custom optname 
through bpf prog.

> 
> and there is no selftest also.
> 

Yes, if remove this restriction, everyone thinks it's ok, I'll add it in 
the next version.

>> +        ret = -EFAULT;
>> +        goto out;
>>       }
>>       lock_sock(sk);
>