linux-kernel - Re: [PATCH bpf-next] bpf: Clean code with bpf_copy_to

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

Message-ID: <9f92515f-6dad-4ff6-884e-e60016e36ae7@linux.dev>
Date: Fri, 4 Jul 2025 00:19:33 +0800
From: Tao Chen <chen.dylane@...ux.dev>
To: Yonghong Song <yonghong.song@...ux.dev>, ast@...nel.org,
 daniel@...earbox.net, john.fastabend@...il.com, andrii@...nel.org,
 martin.lau@...ux.dev, eddyz87@...il.com, song@...nel.org,
 kpsingh@...nel.org, sdf@...ichev.me, haoluo@...gle.com, jolsa@...nel.org
Cc: bpf@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next] bpf: Clean code with bpf_copy_to_user

在 2025/7/4 00:14, Yonghong Song 写道:
> 
> 
> On 7/3/25 9:03 AM, Tao Chen wrote:
>> 在 2025/7/3 23:35, Yonghong Song 写道:
>>>
>>>
>>> On 7/3/25 5:43 AM, Tao Chen wrote:
>>>> No logic change, just use bpf_copy_to_user to clean code.
>>>>
>>>> Signed-off-by: Tao Chen <chen.dylane@...ux.dev>
>>>> ---
>>>>   kernel/bpf/syscall.c | 17 +++--------------
>>>>   1 file changed, 3 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>>>> index e6eea594f1c..ca152d36312 100644
>>>> --- a/kernel/bpf/syscall.c
>>>> +++ b/kernel/bpf/syscall.c
>>>> @@ -5208,21 +5208,10 @@ static int bpf_task_fd_query_copy(const 
>>>> union bpf_attr *attr,
>>>>               if (put_user(zero, ubuf))
>>>>                   return -EFAULT;
>>>> -        } else if (input_len >= len + 1) {
>>>> -            /* ubuf can hold the string with NULL terminator */
>>>> -            if (copy_to_user(ubuf, buf, len + 1))
>>>> -                return -EFAULT;
>>>>           } else {
>>>> -            /* ubuf cannot hold the string with NULL terminator,
>>>> -             * do a partial copy with NULL terminator.
>>>> -             */
>>>> -            char zero = '\0';
>>>> -
>>>> -            err = -ENOSPC;
>>>> -            if (copy_to_user(ubuf, buf, input_len - 1))
>>>> -                return -EFAULT;
>>>> -            if (put_user(zero, ubuf + input_len - 1))
>>>> -                return -EFAULT;
>>>> +            err = bpf_copy_to_user(ubuf, buf, input_len, len);
>>>> +            if (err)
>>>> +                return err;
>>>>           }
>>>>       }
>>>
>>> Actually, there is a return value change with this patch.
>>> bpf_copy_to_user() return returns -ENOSPC while the original
>>> implementation may return -EFAULT due to following code.
>>>
>>>          if (put_user(prog_id, &uattr->task_fd_query.prog_id) ||
>>>              put_user(fd_type, &uattr->task_fd_query.fd_type) ||
>>>              put_user(probe_offset, &uattr- 
>>> >task_fd_query.probe_offset) ||
>>>              put_user(probe_addr, &uattr->task_fd_query.probe_addr))
>>>                  return -EFAULT;
>>>
>>>          return err;
>>>
>>
>> You are right, maybe we can just use:
>>     err = bpf_copy_to_user(ubuf, buf, input_len, len);
>> and no return check
>> or move these put_user code to the front.
> 
> Maybe do the following?
> 
>     err = bpf_copy_to_user(ubuf, buf, input_len, len);
>     if (err && err != -ENOSPC)
>       return err;
> 

Well, it looks better, i will change it in v2, thanks.

-- 
Best Regards
Tao Chen