From patchwork Thu Nov 5 19:29:21 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Xu X-Patchwork-Id: 11885001 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=-12.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, 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 CFF2FC4741F for ; Thu, 5 Nov 2020 19:30:08 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 764A52078E for ; Thu, 5 Nov 2020 19:30:07 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=dxuuu.xyz header.i=@dxuuu.xyz header.b="cR6iYAy+"; dkim=temperror (0-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b="eO+k7zxF" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1732259AbgKETaD (ORCPT ); Thu, 5 Nov 2020 14:30:03 -0500 Received: from out4-smtp.messagingengine.com ([66.111.4.28]:50953 "EHLO out4-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1731060AbgKETaB (ORCPT ); Thu, 5 Nov 2020 14:30:01 -0500 Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailout.nyi.internal (Postfix) with ESMTP id 60AAB5C00A1; Thu, 5 Nov 2020 14:30:00 -0500 (EST) Received: from mailfrontend1 ([10.202.2.162]) by compute3.internal (MEProxy); Thu, 05 Nov 2020 14:30:00 -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=fm1; bh=I06IP8OMpVQGH HEecVW9tziaqLhe1IrGmcpFFL3aS9o=; b=cR6iYAy+br7+lDd423zg27fWvLnUM kd2z8kqCTMaaiYfA5FR0NjciSy/05qEDKN/5mOR7UNPloSPw2dQbk+u1lJhIkoAL pfviUgAFRzgivpLVjLYAXPGKrX71vG20bIr8u1dvPZTkNk4J/22wj6Kvi8kdNgmE erFcKdACewkkcfkhqqtZLdPsIxQMobJWZbwjW+hKHRhEehiMK0wmU9Pd0vjYA7q5 L06gofM3e516gjQSr41Fj6l9Kwn/e7s+9CzKFE7DlfmiFX/NmKeATzOKZHYFbzlz NiI3NrT2n3UcHnN3Ga1VjncRLEhkRiMZ1U+9mtboahekEWpGmW0wYMKRA== 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=I06IP8OMpVQGHHEecVW9tziaqLhe1IrGmcpFFL3aS9o=; b=eO+k7zxF QobUpQgTXQuSnbhb2h3ms9tNcwVs3UPUjsDu0TvoWFwnoxj1ds3nHkuj15+HJJ19 F1q0vPx786VQCB/QYThS7varOuupG050OBaW3ji3BCtgLBcOYD0UYdySe1+vPQlw qF1o/nwaoB3fvZlyMJH/abqgCx6O0jIub+JTpcytDF3bLBucP+iY0ZhN17VxZlna aAokDoJslK4IOSV3rVPlC7r6uXZrQ2gcRAhct8Z/pMlvQhb9f7IxQenNgAvYvxUA HJW+fAE+5TjyprH47m6mWGGwRF/22S1yeVXSH5mfo53VrvtT7HSTmu3MNbIWCfgQ q7EqiChiekwjbA== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedujedruddtjedguddvhecutefuodetggdotefrod 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 74DE73280391; Thu, 5 Nov 2020 14:29:59 -0500 (EST) From: Daniel Xu To: bpf@vger.kernel.org, linux-kernel@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net, songliubraving@fb.com Cc: Daniel Xu , kernel-team@fb.com Subject: [PATCH bpf v3 1/2] lib/strncpy_from_user.c: Don't overcopy bytes after NUL terminator Date: Thu, 5 Nov 2020 11:29:21 -0800 Message-Id: <4ff12d0c19de63e7172d25922adfb83ae7c8691f.1604604240.git.dxu@dxuuu.xyz> X-Mailer: git-send-email 2.28.0 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 --- lib/strncpy_from_user.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c index e6d5fcc2cdf3..82a67dde136b 100644 --- a/lib/strncpy_from_user.c +++ b/lib/strncpy_from_user.c @@ -35,17 +35,22 @@ 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, *out; /* 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; if (has_zero(c, &data, &constants)) { data = prep_zero_mask(c, data, &constants); data = create_zero_mask(data); + mask = zero_bytemask(data); + out = (unsigned long *)(dst+res); + *out = (*out & ~mask) | (c & mask); return res + find_zero(data); } + + *(unsigned long *)(dst+res) = c; + res += sizeof(unsigned long); max -= sizeof(unsigned long); } From patchwork Thu Nov 5 19:29:22 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Xu X-Patchwork-Id: 11884999 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=-12.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,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 89787C5517A for ; Thu, 5 Nov 2020 19:30:09 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 2A7F12078E for ; Thu, 5 Nov 2020 19:30:09 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=dxuuu.xyz header.i=@dxuuu.xyz header.b="VPiHMAX8"; dkim=temperror (0-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b="VNo5c8pL" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1732257AbgKETaI (ORCPT ); Thu, 5 Nov 2020 14:30:08 -0500 Received: from out4-smtp.messagingengine.com ([66.111.4.28]:42139 "EHLO out4-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1732221AbgKETaC (ORCPT ); Thu, 5 Nov 2020 14:30:02 -0500 Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailout.nyi.internal (Postfix) with ESMTP id 584F75C00EE; Thu, 5 Nov 2020 14:30:01 -0500 (EST) Received: from mailfrontend1 ([10.202.2.162]) by compute3.internal (MEProxy); Thu, 05 Nov 2020 14:30:01 -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=fm1; bh=UaRy2DWi1Qyi7 0CmV76MDvFDVaBQWoSa3U4IeMYvayk=; b=VPiHMAX8AxaRcDsnYJuMYh6LJ8OmO tJlAFYbBdcKOC30FEd8RlfQeBTUCo10ognGTdCcfdF7nBDqhOUXUx63uA/0Hgny3 cwHx4LUfyHkB3FMLy2RzumApgQNl7hnPqmir61iF3nyX3Uyko/fnNGBvkSXQDj0u NOvYgu0apKy4efOLWG/4wM9dAAfNQMOqRhISUQktZfTtk0waLjZUgp+Um+7wq+rU 3juRJP9zS0p46cn2jSiKiyr72DO7SLrshGvRYHwGwJbg1Gsi1eaDxhBHxEOpeFQi 0FQEsNPfptj7V5spo9E0+kzsxK4gWZVsyae/zTETokUMcDINeVeTFwjfw== 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=UaRy2DWi1Qyi70CmV76MDvFDVaBQWoSa3U4IeMYvayk=; b=VNo5c8pL Ti21zzMd69rfdSckNc74sNZtRjD3nj0AvNVZW4jlEkaxCG1fHhroyP+bcOLkyBHk UWszZ2DvP1oYWs/Hg2UmMt/kPLit1oBsQDN8yGGeFoCRIFgq8MBCOdz4w7e71E+N yMg82nXTITzEJB4ViUjzkeIqUtpW1FHrnO3NYmG4RIOSaEuwMnZ7pBo6BqYyzCtE bB0D56E42HT0lR1bKyJclWsabVQh7ScojjEGpRwxBSs/dQbIX7IDr3lpeCqDajYc JM5O/svObJQxL4xArS7GrLkz4LsfJ86kzrT5vdgh13uH3WF9zsRJKmv2DlpFl8I/ DJQBKFzw8pTKoA== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedujedruddtjedguddvhecutefuodetggdotefrod 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 578A53280394; Thu, 5 Nov 2020 14:30:00 -0500 (EST) From: Daniel Xu To: bpf@vger.kernel.org, linux-kernel@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net, songliubraving@fb.com Cc: Daniel Xu , kernel-team@fb.com Subject: [PATCH bpf v3 2/2] selftest/bpf: Test bpf_probe_read_user_str() strips trailing bytes after NUL Date: Thu, 5 Nov 2020 11:29:22 -0800 Message-Id: <1d76e240e7b4fa264b3a50ebd391c4f0cc814919.1604604240.git.dxu@dxuuu.xyz> X-Mailer: git-send-email 2.28.0 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. Signed-off-by: Daniel Xu --- .../bpf/prog_tests/probe_read_user_str.c | 60 +++++++++++++++++++ .../bpf/progs/test_probe_read_user_str.c | 34 +++++++++++ 2 files changed, 94 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..7c6422901b78 --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/probe_read_user_str.c @@ -0,0 +1,60 @@ +// SPDX-License-Identifier: GPL-2.0 +#include +#include "test_probe_read_user_str.skel.h" + +static const char str[] = "mestring"; + +void test_probe_read_user_str(void) +{ + struct test_probe_read_user_str *skel; + int fd, err, duration = 0; + char buf[256]; + ssize_t n; + + 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; + + fd = open("/dev/null", O_WRONLY); + if (CHECK(fd < 0, "open", "open /dev/null failed: %d\n", fd)) + goto out; + + /* Ensure bytes after string are ones */ + memset(buf, 1, sizeof(buf)); + memcpy(buf, str, sizeof(str)); + + /* Trigger tracepoint */ + n = write(fd, buf, sizeof(buf)); + if (CHECK(n != sizeof(buf), "write", "write failed: %ld\n", n)) + goto fd_out; + + /* Did helper fail? */ + if (CHECK(skel->bss->ret < 0, "prog_ret", "prog returned: %ld\n", + skel->bss->ret)) + goto fd_out; + + /* Check that string was copied correctly */ + err = memcmp(skel->bss->buf, str, sizeof(str)); + if (CHECK(err, "memcmp", "prog copied wrong string")) + goto fd_out; + + /* Now check that no extra trailing bytes were copied */ + memset(buf, 0, sizeof(buf)); + err = memcmp(skel->bss->buf + sizeof(str), buf, sizeof(buf) - sizeof(str)); + if (CHECK(err, "memcmp", "trailing bytes were not stripped")) + goto fd_out; + +fd_out: + close(fd); +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..5da764a8bf85 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/test_probe_read_user_str.c @@ -0,0 +1,34 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include +#include +#include + +#include + +struct sys_enter_write_args { + unsigned long long pad; + int syscall_nr; + int pad1; /* 4 byte hole */ + unsigned int fd; + int pad2; /* 4 byte hole */ + const char *buf; + size_t count; +}; + +pid_t pid = 0; +long ret = 0; +char buf[256] = {}; + +SEC("tracepoint/syscalls/sys_enter_write") +int on_write(struct sys_enter_write_args *ctx) +{ + if (pid != (bpf_get_current_pid_tgid() >> 32)) + return 0; + + ret = bpf_probe_read_user_str(buf, sizeof(buf), ctx->buf); + + return 0; +} + +char _license[] SEC("license") = "GPL";