Commit 14d6d86c authored by Alexei Starovoitov's avatar Alexei Starovoitov
Browse files

Merge branch 'Fix bpf_probe_read_user_str() overcopying' 



Daniel Xu says:

====================

6ae08ae3 ("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.

v6 -> v7:
* Add comments

v5 -> v6:
* zero-pad up to sizeof(unsigned long) after NUL

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

Signed-off-by: default avatarAlexei Starovoitov <ast@kernel.org>
parents 1fd6cee1 c8a36aed

kernel/trace/bpf_trace.c

+10 −0
Original line number Diff line number Diff line
@@ -181,6 +181,16 @@ bpf_probe_read_user_str_common(void *dst, u32 size, 
{

	int ret;



	/*

	 * NB: We rely on strncpy_from_user() not copying junk past the NUL

	 * terminator into `dst`.

	 *

	 * strncpy_from_user() does long-sized strides in the fast path. If the

	 * strncpy does not mask out the bytes after the NUL in `unsafe_ptr`,

	 * then there could be junk after the NUL in `dst`. If user takes `dst`

	 * and keys a hash map with it, then semantically identical strings can

	 * occupy multiple entries in the map.

	 */

	ret = strncpy_from_user_nofault(dst, unsafe_ptr, size);

	if (unlikely(ret < 0))

		memset(dst, 0, size);

lib/strncpy_from_user.c

+17 −2
Original line number Diff line number Diff line
@@ -35,17 +35,32 @@ static inline long do_strncpy_from_user(char *dst, const char __user *src, 
		goto byte_at_a_time;



	while (max >= sizeof(unsigned long)) {

		unsigned long c, data;

		unsigned long c, data, mask;



		/* 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);



		*(unsigned long *)(dst+res) = c;

		/*

		 * Note that we mask out the bytes following the NUL. This is

		 * important to do because string oblivious code may read past

		 * the NUL. For those routines, we don't want to give them

		 * potentially random bytes after the NUL in `src`.

		 *

		 * One example of such code is BPF map keys. BPF treats map keys

		 * as an opaque set of bytes. Without the post-NUL mask, any BPF

		 * maps keyed by strings returned from strncpy_from_user() may

		 * have multiple entries for semantically identical strings.

		 */

		if (has_zero(c, &data, &constants)) {

			data = prep_zero_mask(c, data, &constants);

			data = create_zero_mask(data);

			mask = zero_bytemask(data);

			*(unsigned long *)(dst+res) = c & mask;

			return res + find_zero(data);

		}



		*(unsigned long *)(dst+res) = c;



		res += sizeof(unsigned long);

		max -= sizeof(unsigned long);

	}

tools/testing/selftests/bpf/prog_tests/probe_read_user_str.c

0 → 100644
+71 −0
Original line number Diff line number Diff line 
// SPDX-License-Identifier: GPL-2.0

#include <test_progs.h>

#include "test_probe_read_user_str.skel.h"



static const char str1[] = "mestring";

static const char str2[] = "mestringalittlebigger";

static const char str3[] = "mestringblubblubblubblubblub";



static int test_one_str(struct test_probe_read_user_str *skel, const char *str,

			size_t len)

{

	int err, duration = 0;

	char buf[256];



	/* Ensure bytes after string are ones */

	memset(buf, 1, sizeof(buf));

	memcpy(buf, str, len);



	/* Give prog our userspace pointer */

	skel->bss->user_ptr = buf;



	/* Trigger tracepoint */

	usleep(1);



	/* Did helper fail? */

	if (CHECK(skel->bss->ret < 0, "prog_ret", "prog returned: %ld\n",

		  skel->bss->ret))

		return 1;



	/* Check that string was copied correctly */

	err = memcmp(skel->bss->buf, str, len);

	if (CHECK(err, "memcmp", "prog copied wrong string"))

		return 1;



	/* Now check that no extra trailing bytes were copied */

	memset(buf, 0, sizeof(buf));

	err = memcmp(skel->bss->buf + len, buf, sizeof(buf) - len);

	if (CHECK(err, "memcmp", "trailing bytes were not stripped"))

		return 1;



	return 0;

}



void test_probe_read_user_str(void)

{

	struct test_probe_read_user_str *skel;

	int err, duration = 0;



	skel = test_probe_read_user_str__open_and_load();

	if (CHECK(!skel, "test_probe_read_user_str__open_and_load",

		  "skeleton open and load failed\n"))

		return;



	/* Give pid to bpf prog so it doesn't read from anyone else */

	skel->bss->pid = getpid();



	err = test_probe_read_user_str__attach(skel);

	if (CHECK(err, "test_probe_read_user_str__attach",

		  "skeleton attach failed: %d\n", err))

		goto out;



	if (test_one_str(skel, str1, sizeof(str1)))

		goto out;

	if (test_one_str(skel, str2, sizeof(str2)))

		goto out;

	if (test_one_str(skel, str3, sizeof(str3)))

		goto out;



out:

	test_probe_read_user_str__destroy(skel);

}

tools/testing/selftests/bpf/progs/test_probe_read_user_str.c

0 → 100644
+25 −0
Original line number Diff line number Diff line 
// SPDX-License-Identifier: GPL-2.0



#include <linux/bpf.h>

#include <bpf/bpf_helpers.h>

#include <bpf/bpf_tracing.h>



#include <sys/types.h>



pid_t pid = 0;

long ret = 0;

void *user_ptr = 0;

char buf[256] = {};



SEC("tracepoint/syscalls/sys_enter_nanosleep")

int on_write(void *ctx)

{

	if (pid != (bpf_get_current_pid_tgid() >> 32))

		return 0;



	ret = bpf_probe_read_user_str(buf, sizeof(buf), user_ptr);



	return 0;

}



char _license[] SEC("license") = "GPL";

CRA Git | Maintained and supported by SUSTech CRA and CCSE