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 anothereBPF program from within the kernel. In this case the argument unionbpf_attr pointer (as well as the insns and license pointers inside) is akernel address instead of a userspace address (which is the case of ausual bpf() syscall). To make the memory copying process in the syscallwork 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 auserspace address and a memcpy() is performed for a kernel address [2].This can lead to problems because the in-kernel pointer is never checkedfor validity. If an eBPF syscall program tries to call bpf_sys_bpf()with a bad insns pointer, say 0xdeadbeef (which is supposed to point tothe start of the instruction array) in the bpf_attr union, memcpy() is always happy to dereference the bad pointer to cause a un-handle-ablepage fault and in turn an oops. However, this is not supposed to happenbecause 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#n44Signed-off-by: Jinghao Jia <jinghao@xxxxxxxxxxxxx> --- 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() helpermight 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#n2566Best, Jinghaostatic 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 networkingas 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 mighthave 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
