Re: [RFC bpf-next v3 3/5] bpf: Prevent BPF programs from access the buffer pointed by user_optval.

[Kui-Feng Lee <sinquersw@gmail.com>]

On 8/16/23 17:55, Martin KaFai Lau wrote:
> On 8/15/23 10:47 AM, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> Since the buffer pointed by ctx->user_optval is in user space, BPF 
>> programs
>> in kernel space should not access it directly.  They should use
>> bpf_copy_from_user() and bpf_copy_to_user() to move data between user and
>> kernel space.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   kernel/bpf/cgroup.c   | 16 +++++++++--
>>   kernel/bpf/verifier.c | 66 +++++++++++++++++++++----------------------
>>   2 files changed, 47 insertions(+), 35 deletions(-)
>>
>> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
>> index b977768a28e5..425094e071ba 100644
>> --- a/kernel/bpf/cgroup.c
>> +++ b/kernel/bpf/cgroup.c
>> @@ -2494,12 +2494,24 @@ static bool cg_sockopt_is_valid_access(int 
>> off, int size,
>>       case offsetof(struct bpf_sockopt, optval):
>>           if (size != sizeof(__u64))
>>               return false;
>> -        info->reg_type = PTR_TO_PACKET;
>> +        if (prog->aux->sleepable)
>> +            /* Prohibit access to the memory pointed by optval
>> +             * in sleepable programs.
>> +             */
>> +            info->reg_type = PTR_TO_PACKET | MEM_USER;
> 
> Is MEM_USER needed to call bpf_copy_from_user()?
> 
> Also, from looking at patch 4, the optval could be changed from user 
> memory to kernel memory during runtime. Is it useful to check MEM_USER 
> during the verifier load time?

It has been checked.
optval & optval_end are exported and can be used to compute the size
of the user space buffer. However, it can not be used to read the
content of the user space buffer.

To be specific, __check_mem_access() will fail due to having MEM_USER
in reg->type. However, it is implicit. I will make it explicit if
necessary.

> 
> How about just return false here to disallow sleepable prog to use 
> ->optval and ->optval_end. Enforce sleepable prog to stay with the 
> bpf_dynptr_read/write API and avoid needing the optval + len > 
> optval_end check in the sleepable bpf prog. WDYT?

Then, we need to export another variable to get the size of the buffer
pointed by optval. Then, I would like to have a new context type
instead of struct bpf_sockopt for the sleepable programs. With the new
type, we can remove optval & optval_end completely from the view of
sleepable ones. They will get errors of accessing optval & optval_end
as early as compile time.

> 
> Regarding ->optlen, do you think the sleepable prog can stay with the 
> bpf_dynptr_size() and bpf_dynptr_adjust() such that no need to expose 
> optlen to the sleepable prog also.
> 
>> +        else
>> +            info->reg_type = PTR_TO_PACKET;
>>           break;
>>       case offsetof(struct bpf_sockopt, optval_end):
>>           if (size != sizeof(__u64))
>>               return false;
>> -        info->reg_type = PTR_TO_PACKET_END;
>> +        if (prog->aux->sleepable)
>> +            /* Prohibit access to the memory pointed by
>> +             * optval_end in sleepable programs.
>> +             */
>> +            info->reg_type = PTR_TO_PACKET_END | MEM_USER;
>> +        else
>> +            info->reg_type = PTR_TO_PACKET_END;
>>           break;
>