From patchwork Tue Nov 17 20:05:45 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Xu X-Patchwork-Id: 11913447 X-Patchwork-Delegate: bpf@iogearbox.net Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-18.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_GIT autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5A1A2C63798 for ; Tue, 17 Nov 2020 20:06:21 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id DD15220707 for ; Tue, 17 Nov 2020 20:06:20 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=dxuuu.xyz header.i=@dxuuu.xyz header.b="AHxtWFtf"; dkim=temperror (0-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b="H5Pn+6Ow" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729991AbgKQUF6 (ORCPT ); Tue, 17 Nov 2020 15:05:58 -0500 Received: from wout1-smtp.messagingengine.com ([64.147.123.24]:47855 "EHLO wout1-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726685AbgKQUF6 (ORCPT ); Tue, 17 Nov 2020 15:05:58 -0500 Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailout.west.internal (Postfix) with ESMTP id A4D24B92; Tue, 17 Nov 2020 15:05:56 -0500 (EST) Received: from mailfrontend2 ([10.202.2.163]) by compute3.internal (MEProxy); Tue, 17 Nov 2020 15:05:57 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=dxuuu.xyz; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; s=fm2; bh=eSrCn2HpzgXUZ DlHfeI/vnOmvS5w6Z6yLr13Z/fL+Og=; b=AHxtWFtfXAKfEvwWOflAJeZiEwwAo zYsKXfL/mvWHpySnPzJeAvWWUQIlyTQ1jnJkUI+tBjJOkyamfdpRJYy9Shx7+Wic EFILX7wL2hsWfTmpQrhBWljuz4SCyyuPxGZ5x19zNchAzX8nBlabZqfgJanL17wj Nju1m+2Ax/HI4ysrPp9tBAlQdf63A1CA97BmIYmcQqlogf2wJ5VOkoTgqNe2PZ5/ xs5HRBbZi1ZGAV6gkmA8KeaqA5XUR8QRLAR2ACZRq6iG3+moRNrPyq5rz4X7h08T 8gqfjDEszpk0gLy//B6ghkhLNa5Lohw9M6TjL5QZzMdj6wg1cj97fYbtw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:date:from :in-reply-to:message-id:mime-version:references:subject:to :x-me-proxy:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s= fm1; bh=eSrCn2HpzgXUZDlHfeI/vnOmvS5w6Z6yLr13Z/fL+Og=; b=H5Pn+6Ow /DfWfGJeBrz45PCJ//gYnDNnovIV/ichQAKJAeuKT2jxvjUl1+VV7cEhxX8QcRqF ILCeNVL6Gi/CzqwBGOJzJW/tssYqSZVMYGFS+yyGfiy1RBkAAPXotyE1HxlSLdhA KRLQGJ5/wdc9aHtYdRFf7xsf98EbokWzABiGb+oq+ZxLnBORwfdEytf2kY00zETd UHwKvl2GMzEGVltHsDeRof4ecqrE4b4QnllE8pe/ejGdu0ubmSvvpKJS1Ji3iYK8 d1T/UJjkohWb7i0j5hQHdA/r/j4PBeeSpTThLeuGpvNDqc4VSrIsjGMA0WgYg/0p qUtYybgdoYnHBA== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedujedrudeffedgudeffecutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecufghrlhcuvffnffculdejtddmnecujfgurhephf fvufffkffojghfggfgsedtkeertdertddtnecuhfhrohhmpeffrghnihgvlhcuighuuceo ugiguhesugiguhhuuhdrgiihiieqnecuggftrfgrthhtvghrnhepgfekudelkefhteevhf eggfdvgeefjeefgfeuvddutdfhgffghfehtdeuueetfeeinecukfhppeeiledrudekuddr uddthedrieegnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrh homhepugiguhesugiguhhuuhdrgiihii X-ME-Proxy: Received: from localhost.localdomain (c-69-181-105-64.hsd1.ca.comcast.net [69.181.105.64]) by mail.messagingengine.com (Postfix) with ESMTPA id 35CA83064AB3; Tue, 17 Nov 2020 15:05:55 -0500 (EST) From: Daniel Xu To: bpf@vger.kernel.org, linux-kernel@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net, songliubraving@fb.com, andrii.nakryiko@gmail.com, torvalds@linux-foundation.org Cc: Daniel Xu , kernel-team@fb.com Subject: [PATCH bpf v7 1/2] lib/strncpy_from_user.c: Don't overcopy bytes after NUL terminator Date: Tue, 17 Nov 2020 12:05:45 -0800 Message-Id: <21efc982b3e9f2f7b0379eed642294caaa0c27a7.1605642949.git.dxu@dxuuu.xyz> X-Mailer: git-send-email 2.29.2 In-Reply-To: References: MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org X-Patchwork-Delegate: bpf@iogearbox.net 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. Fixes: 6ae08ae3dea2 ("bpf: Add probe_read_{user, kernel} and probe_read_{user, kernel}_str helpers") Signed-off-by: Daniel Xu --- As mentioned in the v6 discussion, I didn't think it would make a lot of sense to put a comment in kernel/bpf/hashtab.c:alloc_htab_elem . I opted to add the comment to bpf_probe_read_user_str_common() b/c it seems like the next best place. Just let me know if you want it somewhere else. kernel/trace/bpf_trace.c | 10 ++++++++++ lib/strncpy_from_user.c | 19 +++++++++++++++++-- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index 5113fd423cdf..048c655315f1 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -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); diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c index e6d5fcc2cdf3..122d8d0e253c 100644 --- a/lib/strncpy_from_user.c +++ b/lib/strncpy_from_user.c @@ -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); } From patchwork Tue Nov 17 20:05:46 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Xu X-Patchwork-Id: 11913449 X-Patchwork-Delegate: bpf@iogearbox.net Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-18.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_GIT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E4E32C6379D for ; Tue, 17 Nov 2020 20:06:20 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 99639206C0 for ; Tue, 17 Nov 2020 20:06:20 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=dxuuu.xyz header.i=@dxuuu.xyz header.b="USaN3eDe"; dkim=temperror (0-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b="kKjnQ9UI" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726685AbgKQUF7 (ORCPT ); Tue, 17 Nov 2020 15:05:59 -0500 Received: from wout1-smtp.messagingengine.com ([64.147.123.24]:53751 "EHLO wout1-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730094AbgKQUF7 (ORCPT ); Tue, 17 Nov 2020 15:05:59 -0500 Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailout.west.internal (Postfix) with ESMTP id B9A71C4E; Tue, 17 Nov 2020 15:05:57 -0500 (EST) Received: from mailfrontend2 ([10.202.2.163]) by compute3.internal (MEProxy); Tue, 17 Nov 2020 15:05:58 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=dxuuu.xyz; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; s=fm2; bh=e+edu/6Q4wBbY YEXH8t+dOvgpbiWbWHOudleytAG4Gc=; b=USaN3eDeLtIP4HGV8RR1/MyzaMSxC TInaSQX7xI55CMmbEgf2H5Xo1XMmuQN/xslcU+2Bopac/NKaVHOX/QnWU4Doe/Kr HbMWMOu7QJy7VCi+VR/SbeMrS5nDx3OTjK5IOK0M1xUO/S6kixMqzwySs2znu3/t l+/7Z1OhjowI6uYzDX3s+qWtw7sHcpEIZe+pkzWHXyXcfxZ9z+n+ABdAj26z/OYO oGCHsC2P5llqIlfpewSRsHPOcUu84+We479H5ZFh/XgDut5jkjANrLJN11/l4Qii D1U+QLnmYNS+1nd2rjLiuzlffloz9lUHO0CtWfcBBoMVVfwadKzcZVuPQ== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:date:from :in-reply-to:message-id:mime-version:references:subject:to :x-me-proxy:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s= fm1; bh=e+edu/6Q4wBbYYEXH8t+dOvgpbiWbWHOudleytAG4Gc=; b=kKjnQ9UI SXPOD6i/SbXLTwrVgcszqONFoEpIm1T7ep8MVz3GHwYDoBRcOBtzRBLL+dpJi78b 1RafK+zEe2ZzGjz7U80GtWieYkuacGGWzsfuE0HrSVME3I3aZBXQ9LTXmwqHynu9 zmVlN0z60yodrZhm+PHRPXf/F/+CLtJ8zKDsSotSY02Z+CqMa2YsJ1b3x6EKb3HM H0OoPLX2hvrruQlKk8AG0AuxxPpuf9MDxupXmVY3niajHcW2JJ9EFJB5WUZt6ySu SGheovvJhHrntSMeruKh3+4Z5vLy9v3lblOAXEW17nv6PoVEQkx3lgXmNn/GrK70 BhLRh8oCVcaz9g== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedujedrudeffedgudeffecutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd enfghrlhcuvffnffculddvfedmnecujfgurhephffvufffkffojghfggfgsedtkeertder tddtnecuhfhrohhmpeffrghnihgvlhcuighuuceougiguhesugiguhhuuhdrgiihiieqne cuggftrfgrthhtvghrnhepgfekudelkefhteevhfeggfdvgeefjeefgfeuvddutdfhgffg hfehtdeuueetfeeinecukfhppeeiledrudekuddruddthedrieegnecuvehluhhsthgvrh fuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepugiguhesugiguhhuuhdrgiih ii X-ME-Proxy: Received: from localhost.localdomain (c-69-181-105-64.hsd1.ca.comcast.net [69.181.105.64]) by mail.messagingengine.com (Postfix) with ESMTPA id 3AE5C3064AB0; Tue, 17 Nov 2020 15:05:56 -0500 (EST) From: Daniel Xu To: bpf@vger.kernel.org, linux-kernel@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net, songliubraving@fb.com, andrii.nakryiko@gmail.com, torvalds@linux-foundation.org Cc: Daniel Xu , kernel-team@fb.com, Andrii Nakryiko Subject: [PATCH bpf v7 2/2] selftest/bpf: Test bpf_probe_read_user_str() strips trailing bytes after NUL Date: Tue, 17 Nov 2020 12:05:46 -0800 Message-Id: <4d977508fab4ec5b7b574b85bdf8b398868b6ee9.1605642949.git.dxu@dxuuu.xyz> X-Mailer: git-send-email 2.29.2 In-Reply-To: References: MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org X-Patchwork-Delegate: bpf@iogearbox.net Previously, bpf_probe_read_user_str() could potentially overcopy the trailing bytes after the NUL due to how do_strncpy_from_user() does the copy in long-sized strides. The issue has been fixed in the previous commit. This commit adds a selftest that ensures we don't regress bpf_probe_read_user_str() again. Acked-by: Song Liu Acked-by: Andrii Nakryiko Signed-off-by: Daniel Xu --- .../bpf/prog_tests/probe_read_user_str.c | 71 +++++++++++++++++++ .../bpf/progs/test_probe_read_user_str.c | 25 +++++++ 2 files changed, 96 insertions(+) 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 diff --git a/tools/testing/selftests/bpf/prog_tests/probe_read_user_str.c b/tools/testing/selftests/bpf/prog_tests/probe_read_user_str.c new file mode 100644 index 000000000000..e419298132b5 --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/probe_read_user_str.c @@ -0,0 +1,71 @@ +// SPDX-License-Identifier: GPL-2.0 +#include +#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); +} diff --git a/tools/testing/selftests/bpf/progs/test_probe_read_user_str.c b/tools/testing/selftests/bpf/progs/test_probe_read_user_str.c new file mode 100644 index 000000000000..3ae398b75dcd --- /dev/null +++ b/tools/testing/selftests/bpf/progs/test_probe_read_user_str.c @@ -0,0 +1,25 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include +#include +#include + +#include + +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";