Re: [PATCH] BPF: Fix potential bad pointer dereference in bpf_sys_bpf

[Jinghao Jia <jinghao@linux.ibm.com>]

On 7/28/22 10:52 AM, Yonghong Song wrote:
>
>
> On 7/27/22 6:29 AM, Jinghao Jia wrote:
>> The bpf_sys_bpf() helper function allows an eBPF program to load another
>> eBPF program from within the kernel. In this case the argument union
>> bpf_attr pointer (as well as the insns and license pointers inside) is a
>> kernel address instead of a userspace address (which is the case of a
>> usual bpf() syscall). To make the memory copying process in the syscall
>> work in both cases, bpfptr_t [1] was introduced to wrap around the
>> pointer and distinguish its origin. Specifically, when copying memory
>> contents from a bpfptr_t, a copy_from_user() is performed in case of a
>> userspace address and a memcpy() is performed for a kernel address [2].
>>
>> This can lead to problems because the in-kernel pointer is never checked
>> for validity. If an eBPF syscall program tries to call bpf_sys_bpf()
>> with a bad insns pointer, say 0xdeadbeef (which is supposed to point to
>> the start of the instruction array) in the bpf_attr union, memcpy() is
>> always happy to dereference the bad pointer to cause a un-handle-able
>> page fault and in turn an oops. However, this is not supposed to happen
>> because at that point the eBPF program is already verified and should
>> not cause a memory error. The same issue in userspace is handled
>> gracefully by copy_from_user(), which would return -EFAULT in such a
>> case.
>>
>> Replace memcpy() with the safer copy_from_kernel_nofault() and
>> strncpy_from_kernel_nofault().
>>
>> [1]: 
>> https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/include/linux/bpfptr.h
>> [2]: 
>> https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/include/linux/sockptr.h#n44
>>
>> Signed-off-by: Jinghao Jia <jinghao@linux.ibm.com>
>> ---
>>   include/linux/sockptr.h | 11 +++--------
>>   1 file changed, 3 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/linux/sockptr.h b/include/linux/sockptr.h
>> index d45902fb4cad..3b8a41c82516 100644
>> --- a/include/linux/sockptr.h
>> +++ b/include/linux/sockptr.h
>> @@ -46,8 +46,7 @@ static inline int copy_from_sockptr_offset(void 
>> *dst, sockptr_t src,
>>   {
>>       if (!sockptr_is_kernel(src))
>>           return copy_from_user(dst, src.user + offset, size);
>> -    memcpy(dst, src.kernel + offset, size);
>> -    return 0;
>> +    return copy_from_kernel_nofault(dst, src.kernel + offset, size);
>>   }
>
> The subject and commit message mentioned it is bpf_sys_bpf() helper
> might have issues. But the patch itself tries to modify 
> copy_from_sockptr_offset() and strncpy_from_sockptr(), why?
>

Hi Yonghong,

Sorry for the confusion. The problem happens when bpf_sys_bpf() helper 
is called with a bad kernel address but the dereference takes place in 
the copy_from_sockptr_offset() and strncpy_from_sockptr() functions.

Let's assume we are doing a BPF_PROG_LOAD operation using bpf_sys_bpf() 
and our insns pointer inside the union bpf_attr argument is set to NULL 
(could be any other bad address). The helper calls __sys_bpf() which 
would then call bpf_prog_load() to load the program. bpf_prog_load() is 
responsible for copying the eBPF instructions to the newly allocated 
memory for the program, which uses the bpfptr_t API [1]. Internally, all 
bpfptr_t operations are backed by the corresponding sockptr_t 
operations. In other words, the code that performs the copy (and 
therefore the deref of the pointer) is inside copy_from_sockptr_offset() 
and strncpy_from_sockptr().

[1]: 
https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/kernel/bpf/syscall.c#n2566

Best,
Jinghao
>>     static inline int copy_from_sockptr(void *dst, sockptr_t src, 
>> size_t size)
>> @@ -93,12 +92,8 @@ static inline void *memdup_sockptr_nul(sockptr_t 
>> src, size_t len)
>>     static inline long strncpy_from_sockptr(char *dst, sockptr_t src, 
>> size_t count)
>>   {
>> -    if (sockptr_is_kernel(src)) {
>> -        size_t len = min(strnlen(src.kernel, count - 1) + 1, count);
>> -
>> -        memcpy(dst, src.kernel, len);
>> -        return len;
>> -    }
>> +    if (sockptr_is_kernel(src))
>> +        return strncpy_from_kernel_nofault(dst, src.kernel, count);
>>       return strncpy_from_user(dst, src.user, count);
>>   }
>>
>> base-commit: d295daf505758f9a0e4d05f4ee3bfdfb4192c18f