| Message ID | cover.1605134506.git.dxu@dxuuu.xyz (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fix bpf_probe_read_user_str() overcopying | expand |
On Wed, Nov 11, 2020 at 2:46 PM Daniel Xu <dxu@dxuuu.xyz> 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 has strncpy start copying a byte at a time if a NUL is > spotted. > > Fixes: 6ae08ae3dea2 ("bpf: Add probe_read_{user, kernel} and probe_read_{user, kernel}_str helpers") > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz> > --- This looks more immediately correct. Acked-by: Andrii Nakryiko <andrii@kernel.org> > lib/strncpy_from_user.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c > index e6d5fcc2cdf3..83180742e729 100644 > --- a/lib/strncpy_from_user.c > +++ b/lib/strncpy_from_user.c > @@ -40,12 +40,11 @@ static inline long do_strncpy_from_user(char *dst, const char __user *src, > /* Fall back to byte-at-a-time if we get a page fault */ > unsafe_get_user(c, (unsigned long __user *)(src+res), byte_at_a_time); > > + if (has_zero(c, &data, &constants)) > + goto byte_at_a_time; > + > *(unsigned long *)(dst+res) = c; > - if (has_zero(c, &data, &constants)) { > - data = prep_zero_mask(c, data, &constants); > - data = create_zero_mask(data); > - return res + find_zero(data); > - } > + > res += sizeof(unsigned long); > max -= sizeof(unsigned long); > } > -- > 2.29.2 >
On Wed, Nov 11, 2020 at 2:46 PM Daniel Xu <dxu@dxuuu.xyz> wrote: > > 6ae08ae3dea2 ("bpf: Add probe_read_{user, kernel} and probe_read_{user, > kernel}_str helpers") introduced a subtle bug where > bpf_probe_read_user_str() would potentially copy a few extra bytes after > the NUL terminator. > > This issue is particularly nefarious when strings are used as map keys, > as seemingly identical strings can occupy multiple entries in a map. > > This patchset fixes the issue and introduces a selftest to prevent > future regressions. > > v4 -> v5: > * don't read potentially uninitialized memory I think the bigger problem was that it could overwrite unintended memory. E.g., in BPF program, if you had something like: char my_buf[8 + 3]; char my_precious_data[5] = {1, 2, 3, 4, 5}; With previous version you'd overwrite my_precious data. BTW, do you test such scenario in the selftests you added? If not, we should have something like this as well and validate 1, 2, 3, 4, 5 stay intact. > > v3 -> v4: > * directly pass userspace pointer to prog > * test more strings of different length > > v2 -> v3: > * set pid filter before attaching prog in selftest > * use long instead of int as bpf_probe_read_user_str() retval > * style changes > > v1 -> v2: > * add Fixes: tag > * add selftest > > Daniel Xu (2): > lib/strncpy_from_user.c: Don't overcopy bytes after NUL terminator > selftest/bpf: Test bpf_probe_read_user_str() strips trailing bytes > after NUL > > lib/strncpy_from_user.c | 9 ++- > .../bpf/prog_tests/probe_read_user_str.c | 71 +++++++++++++++++++ > .../bpf/progs/test_probe_read_user_str.c | 25 +++++++ > 3 files changed, 100 insertions(+), 5 deletions(-) > create mode 100644 tools/testing/selftests/bpf/prog_tests/probe_read_user_str.c > create mode 100644 tools/testing/selftests/bpf/progs/test_probe_read_user_str.c > > -- > 2.29.2 >
On Wed Nov 11, 2020 at 3:22 PM PST, Andrii Nakryiko wrote: > On Wed, Nov 11, 2020 at 2:46 PM Daniel Xu <dxu@dxuuu.xyz> wrote: > > > > 6ae08ae3dea2 ("bpf: Add probe_read_{user, kernel} and probe_read_{user, > > kernel}_str helpers") introduced a subtle bug where > > bpf_probe_read_user_str() would potentially copy a few extra bytes after > > the NUL terminator. > > > > This issue is particularly nefarious when strings are used as map keys, > > as seemingly identical strings can occupy multiple entries in a map. > > > > This patchset fixes the issue and introduces a selftest to prevent > > future regressions. > > > > v4 -> v5: > > * don't read potentially uninitialized memory > > I think the bigger problem was that it could overwrite unintended > memory. E.g., in BPF program, if you had something like: > > char my_buf[8 + 3]; > char my_precious_data[5] = {1, 2, 3, 4, 5}; How does that happen? The while (max >= sizeof(unsigned long)) { /* copy 4 bytes */ max -= sizeof(unsigned long) } /* copy byte at a time */ where `max` is the user supplied length should prevent that kind of corruption, right? [...]
On Thu, Nov 12, 2020 at 11:13 AM Daniel Xu <dxu@dxuuu.xyz> wrote: > > On Wed Nov 11, 2020 at 3:22 PM PST, Andrii Nakryiko wrote: > > On Wed, Nov 11, 2020 at 2:46 PM Daniel Xu <dxu@dxuuu.xyz> wrote: > > > > > > 6ae08ae3dea2 ("bpf: Add probe_read_{user, kernel} and probe_read_{user, > > > kernel}_str helpers") introduced a subtle bug where > > > bpf_probe_read_user_str() would potentially copy a few extra bytes after > > > the NUL terminator. > > > > > > This issue is particularly nefarious when strings are used as map keys, > > > as seemingly identical strings can occupy multiple entries in a map. > > > > > > This patchset fixes the issue and introduces a selftest to prevent > > > future regressions. > > > > > > v4 -> v5: > > > * don't read potentially uninitialized memory > > > > I think the bigger problem was that it could overwrite unintended > > memory. E.g., in BPF program, if you had something like: > > > > char my_buf[8 + 3]; > > char my_precious_data[5] = {1, 2, 3, 4, 5}; > > How does that happen? > > The > > while (max >= sizeof(unsigned long)) { > /* copy 4 bytes */ > > max -= sizeof(unsigned long) > } > > /* copy byte at a time */ > > where `max` is the user supplied length should prevent that kind of > corruption, right? Yes, you are right, I got confused. If the user specified the correct max, then this would have never happened. Never mind. > > [...]
6ae08ae3dea2 ("bpf: Add probe_read_{user, kernel} and probe_read_{user, kernel}_str helpers") introduced a subtle bug where bpf_probe_read_user_str() would potentially copy a few extra bytes after the NUL terminator. This issue is particularly nefarious when strings are used as map keys, as seemingly identical strings can occupy multiple entries in a map. This patchset fixes the issue and introduces a selftest to prevent future regressions. v4 -> v5: * don't read potentially uninitialized memory v3 -> v4: * directly pass userspace pointer to prog * test more strings of different length v2 -> v3: * set pid filter before attaching prog in selftest * use long instead of int as bpf_probe_read_user_str() retval * style changes v1 -> v2: * add Fixes: tag * add selftest Daniel Xu (2): lib/strncpy_from_user.c: Don't overcopy bytes after NUL terminator selftest/bpf: Test bpf_probe_read_user_str() strips trailing bytes after NUL lib/strncpy_from_user.c | 9 ++- .../bpf/prog_tests/probe_read_user_str.c | 71 +++++++++++++++++++ .../bpf/progs/test_probe_read_user_str.c | 25 +++++++ 3 files changed, 100 insertions(+), 5 deletions(-) create mode 100644 tools/testing/selftests/bpf/prog_tests/probe_read_user_str.c create mode 100644 tools/testing/selftests/bpf/progs/test_probe_read_user_str.c