On 11/4/20 9:18 PM, Daniel Xu wrote:
On Wed Nov 4, 2020 at 8:24 AM PST, Daniel Borkmann wrote:
On 11/4/20 3:29 AM, Daniel Xu wrote:
do_strncpy_from_user() may copy some extra bytes after the NUL
terminator into the destination buffer. This usually does not matter for
normal string operations. However, when BPF programs key BPF maps with
strings, this matters a lot.
A BPF program may read strings from user memory by calling the
bpf_probe_read_user_str() helper which eventually calls
do_strncpy_from_user(). The program can then key a map with the
resulting string. BPF map keys are fixed-width and string-agnostic,
meaning that map keys are treated as a set of bytes.
The issue is when do_strncpy_from_user() overcopies bytes after the NUL
terminator, it can result in seemingly identical strings occupying
multiple slots in a BPF map. This behavior is subtle and totally
unexpected by the user.
This commit uses the proper word-at-a-time APIs to avoid overcopying.
Signed-off-by: Daniel Xu <dxu@xxxxxxxxx>
It looks like this is a regression from the recent refactoring of the
mem probing
util functions?
I think it was like this from the beginning, at 6ae08ae3dea2 ("bpf: Add
probe_read_{user, kernel} and probe_read_{user, kernel}_str helpers").
The old bpf_probe_read_str() used the kernel's byte-by-byte copying
routine. bpf_probe_read_user_str() started using strncpy_from_user()
which has been doing the long-sized strides since ~2012 or earlier.
I tried to build and test the kernel at that commit but it seems my
compiler is too new to build that old code. Bunch of build failures.
I assume the refactor you're referring to is 8d92db5c04d1 ("bpf: rework
the compat kernel probe handling").
Ah I see, it was just reusing 3d7081822f7f ("uaccess: Add non-pagefault user-space
read functions"). Potentially it might be safer choice to just rework the
strncpy_from_user_nofault() to mimic strncpy_from_kernel_nofault() in that
regard?
Could we add a Fixes tag and then we'd also need to target the fix
against bpf tree instead of bpf-next, no?
Sure, will do in v2.
Moreover, a BPF kselftest would help to make sure it doesn't regress in
future again.
Ditto.
[..]
Thanks,
Daniel
