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

[Yonghong Song <yhs@fb.com>]

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?

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