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

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

On 7/28/22 7:16 PM, Yonghong Song wrote:
>
>
> On 7/28/22 11:01 AM, Jinghao Jia wrote:
>>
>> 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().
>
> Thanks for explanation. It would be great if you can put the above
> details in the commit message (esp. call stack) which leads to
> the kernel panic(?).
>
>>
>> [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);
>>>>   }
>
> I think we should not modify copy_from_sockptr() and 
> strncpy_from_sockptr(). These two functions are used by networking
> as well and nofault version should not be called for calls in
> networking stack.
>
> So I suggest you directly modify copy_from_bpfptr() and 
> strncpy_from_bpfptr() since these two functions indeed might
> have invalid kernel address and may cause fault.
>

Thanks a lot for the feedback! I do agree that the changes on sockptr 
functions might be problematic. I will roll out a V2 based on your comments.

--Jinghao
>>>>
>>>> base-commit: d295daf505758f9a0e4d05f4ee3bfdfb4192c18f