From patchwork Mon Nov 16 21:17:31 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Xu X-Patchwork-Id: 11910869 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 09A0DC3E8C5 for ; Mon, 16 Nov 2020 21:17:53 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 855572224F for ; Mon, 16 Nov 2020 21:17:52 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=dxuuu.xyz header.i=@dxuuu.xyz header.b="e0W/3HeM"; dkim=temperror (0-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b="oLxX6gvW" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727899AbgKPVRv (ORCPT ); Mon, 16 Nov 2020 16:17:51 -0500 Received: from wout3-smtp.messagingengine.com ([64.147.123.19]:54381 "EHLO wout3-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726944AbgKPVRt (ORCPT ); Mon, 16 Nov 2020 16:17:49 -0500 Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailout.west.internal (Postfix) with ESMTP id 6B056D38; Mon, 16 Nov 2020 16:17:48 -0500 (EST) Received: from mailfrontend1 ([10.202.2.162]) by compute3.internal (MEProxy); Mon, 16 Nov 2020 16:17:49 -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=Qu8S2T3pVw4yE Yxi6pqulgrwimeoqQCtqPxsZtatJAU=; b=e0W/3HeM7MaZhZ11m2ld7VGTWtrS5 e7BUTcm8/qYG7CVJWLugC4658iMPvjdX2kyemwNqhTqVcUG091vc/wKBtMj2OLNN EVvww6y9X+5g32kx7+O71XfipAERJCEdR9g5cSE5u0js0RuKSIRPpyv+FNtjAC3p XkhZ79XzymErC+rzRYiAO/bvVJHuXy/R+ryS2JdiZCVpcrzPGB+Mnwf8pzyF/HWw N+dnxMDROA1DG10Od2/FOVSx4uEmmsk1rNGWg/joBLMAbMHm+BhFlNfjsByvPcXF 621njcAYMVBfbdSHz9R1h5+scny86CCAjEs3K5QhiweOiBlN6BZys61ag== 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=Qu8S2T3pVw4yEYxi6pqulgrwimeoqQCtqPxsZtatJAU=; b=oLxX6gvW +O3o4bD5A3iXwATBrOI8Mqc2pzQKGtUS85piFuvizP5Pnl2GSZZOZ5a9vLvB33Mt +77BVlbBxcNt0mziXresxeLmvzgevXm1IFet3JzylyfxzPbKg+QM2x5iCZ/ZDYEi QOv9ny1NBZ5Ppcqh3/23zg5sLG71Og9jbK8wE2xdM4XRt9RhC8nJlm7gOpRH7fD5 WXwIVgw7kMyyBrDVV24KxxyyjYgkGrE+aQzFcIW9XKrAyVZQcoi7tFMOeVg3sdYt yStRpIKEvD3BH8MwyH3HsukRRu8xQrIoQ6lW6LcYiPWSXmZymolI8LSuiqmuBt9f lUIipcZPDOH8iQ== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedujedrudefuddgudegkecutefuodetggdotefrod 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 013A03280066; Mon, 16 Nov 2020 16:17:46 -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 v6 1/2] lib/strncpy_from_user.c: Don't overcopy bytes after NUL terminator Date: Mon, 16 Nov 2020 13:17:31 -0800 Message-Id: <470ffc3c76414443fc359b884080a5394dcccec3.1605560917.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 --- Based on on-list discussion and some off-list discussion with Alexei, I'd like to propose the v4-style patch without the `(*out & ~mask)` bit. We can't really zero out the rest of the buffer due to ABI issues. The bpf docs state for bpf_probe_read_user_str(): > In case the string length is smaller than *size*, the target is not > padded with further NUL bytes. lib/strncpy_from_user.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c index e6d5fcc2cdf3..de084f04e50d 100644 --- a/lib/strncpy_from_user.c +++ b/lib/strncpy_from_user.c @@ -35,17 +35,21 @@ 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; 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 Mon Nov 16 21:17:32 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Xu X-Patchwork-Id: 11910867 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 A8BCDC63697 for ; Mon, 16 Nov 2020 21:17:53 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 5CC412224F for ; Mon, 16 Nov 2020 21:17:53 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=dxuuu.xyz header.i=@dxuuu.xyz header.b="ZyeRn45K"; dkim=temperror (0-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b="Ro8PCpDM" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728018AbgKPVRw (ORCPT ); Mon, 16 Nov 2020 16:17:52 -0500 Received: from wout3-smtp.messagingengine.com ([64.147.123.19]:56501 "EHLO wout3-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727796AbgKPVRu (ORCPT ); Mon, 16 Nov 2020 16:17:50 -0500 Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailout.west.internal (Postfix) with ESMTP id 865D6D3D; Mon, 16 Nov 2020 16:17:49 -0500 (EST) Received: from mailfrontend1 ([10.202.2.162]) by compute3.internal (MEProxy); Mon, 16 Nov 2020 16:17:50 -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=ZyeRn45KhNZPyKfrFOUYMW2Z1tIcd G/AdXINYADc+YmzDKlYxZ1rjCgpV9GsEKOGjPd0OLHIlbkwo5JbtmcAayg9+F1H0 IQyrffwlNAjSFayyN4TibHMUxTMBjT5e6BFcMjhcwgjwXkZ4A9RGNzQ53EiKNXRo Cpeybi7jhD90EzeuUfK4WjHY0iF7OO9/6lzbPFy8sjM1yPditmiiVn3Ul0OBg4No KRHAJBCrH6D/DBczToqxLHVXfOvuASicidfwUmnjpxbzskXy9zff+O960DJYkvA0 WLmFEF88wKlGGy/iVKSCcRtiT1To+/mEnxgAFpJvg6CWh+6aSNrgkTWSQ== 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=Ro8PCpDM ll0q3vvStJBNmj7mx0Bj3AdKGhT/5eMakCu+ZIaV2LIsvJOxJQ+2S49denIBBGjz 6W+nxbIPXlHRiaMqEtki5ne04tnI6qeZenBTNFrLpW0bWFtcJz/vHC8JfiTIzCew EhvgMuvZHOwM/DM7r6m7eiYFq2xyOrPYySs6TGfeEN0Qzh8fZWxqH1pvhLPC7vB3 ByFhhgjtA551KC5QXCcxNta+P9kSl82g1H5Y8NiH6ucAdTGAm2iiQdH5FV5GWzr7 X0XK2LUB1yj/CJwzR1v6nzvX7/41RqS8Ym4cgtOIYW4Uy5Q11W7CvxFgAhPPu2KH jcLZJ5ozgfHGrw== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedujedrudefuddgudegkecutefuodetggdotefrod 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 0A9383280067; Mon, 16 Nov 2020 16:17:47 -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 v6 2/2] selftest/bpf: Test bpf_probe_read_user_str() strips trailing bytes after NUL Date: Mon, 16 Nov 2020 13:17:32 -0800 Message-Id: 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";