linux-kernel - 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]

Date:   Thu, 1 Jun 2023 08:50:19 -0700
From:   Martin KaFai Lau <martin.lau@...ux.dev>
To:     Feng Zhou <zhoufeng.zf@...edance.com>
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: [PATCH bpf-next] bpf: getsockopt hook to get optval without
 checking kernel retval

On 5/31/23 11:05 PM, Feng Zhou wrote:
> 在 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.

This part does not make sense. It is a (get)sockopt. Why the bpf prog should 
expect anything useful in the original __user optval? Other than unnecessary 
copy for other common cases, it looks like a bad api, so consider it a NAK.

> 
>>
>> 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);
>>
>