Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Song Liu <songliubraving@fb.com>
To:     Daniel Xu <dxu@dxuuu.xyz>
CC:     bpf <bpf@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "ast@kernel.org" <ast@kernel.org>,
        "daniel@iogearbox.net" <daniel@iogearbox.net>,
        Kernel Team <Kernel-team@fb.com>
Subject: Re: [PATCH bpf v2 2/2] selftest/bpf: Test bpf_probe_read_user_str()
 strips trailing bytes after NUL
Thread-Topic: [PATCH bpf v2 2/2] selftest/bpf: Test bpf_probe_read_user_str()
 strips trailing bytes after NUL
Thread-Index: AQHWsxsKExRyW1CBMkWOL7EY14SiVKm53SiA
Date:   Thu, 5 Nov 2020 18:30:08 +0000
Message-ID: <B9A62DF7-8C1B-448C-8672-0AF6FC1773BE@fb.com>
References: <cover.1604542786.git.dxu@dxuuu.xyz>
 <4e3e8b9b525c8bed39c0ee2aa68f2dff701f56a4.1604542786.git.dxu@dxuuu.xyz>
In-Reply-To: <4e3e8b9b525c8bed39c0ee2aa68f2dff701f56a4.1604542786.git.dxu@dxuuu.xyz>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <F7980FD980A33A48A137B5D06A39C0CB@namprd15.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: e47254ed-1771-43ea-24ba-08d881b8d311
X-MS-Exchange-CrossTenant-originalarrivaltime: 05 Nov 2020 18:30:08.6907
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: buF8G2BIjncz9SNmPZRnFdL1aPs8r08s4shrxds04P/+FSCESrhBq8CxCqlXqdN7rDK4q/ba+k8Ak9G2sslY8A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR15MB2646
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.312,18.0.737
 definitions=2020-11-05_11:2020-11-05,2020-11-05 signatures=0
X-Proofpoint-Spam-Details: rule=fb_default_notspam policy=fb_default score=0 malwarescore=0
 adultscore=0 mlxlogscore=999 lowpriorityscore=0 clxscore=1015 mlxscore=0
 bulkscore=0 impostorscore=0 phishscore=0 spamscore=0 suspectscore=0
 priorityscore=1501 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2009150000 definitions=main-2011050121
X-FB-Internal: deliver
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> On Nov 4, 2020, at 6:25 PM, Daniel Xu <dxu@dxuuu.xyz> wrote:
>=20
> 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.
>=20
> This commit adds a selftest that ensures we don't regress
> bpf_probe_read_user_str() again.
>=20
> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> ---
> .../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
>=20
> 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..597a166e6c8d
> --- /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 <test_progs.h>
> +#include "test_probe_read_user_str.skel.h"
> +
> +static const char str[] =3D "mestring";
> +
> +void test_probe_read_user_str(void)
> +{
> +	struct test_probe_read_user_str *skel;
> +	int fd, err, duration =3D 0;
> +	char buf[256];
> +	ssize_t n;
> +
> +	skel =3D 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"))
> +		goto out;

nit: we can just return here.=20

> +
> +	err =3D 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 =3D open("/dev/null", O_WRONLY);
> +	if (CHECK(fd < 0, "open", "open /dev/null failed: %d\n", fd))
> +		goto out;
> +
> +	/* Give pid to bpf prog so it doesn't read from anyone else */
> +	skel->bss->pid =3D getpid();

It is better to set pid before attaching skel.=20

> +
> +	/* Ensure bytes after string are ones */
> +	memset(buf, 1, sizeof(buf));
> +	memcpy(buf, str, sizeof(str));
> +
> +	/* Trigger tracepoint */
> +	n =3D write(fd, buf, sizeof(buf));
> +	if (CHECK(n !=3D sizeof(buf), "write", "write failed: %ld\n", n))
> +		goto fd_out;
> +
> +	/* Did helper fail? */
> +	if (CHECK(skel->bss->ret < 0, "prog ret", "prog returned: %d\n",

In most cases, we use underscore instead of spaces in the second argument=20
of CHECK(). IOW, please use "prog_ret" instead of "prog ret".=20

> +		  skel->bss->ret))
> +		goto fd_out;
> +
> +	/* Check that string was copied correctly */
> +	err =3D 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 =3D 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..41c3e296566e
> --- /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 <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +#include <sys/types.h>
> +
> +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 =3D 0;
> +int ret =3D 0;
> +char buf[256] =3D {};
> +
> +SEC("tracepoint/syscalls/sys_enter_write")
> +int on_write(struct sys_enter_write_args *ctx)
> +{
> +	if (pid !=3D (bpf_get_current_pid_tgid() >> 32))
> +		return 0;
> +
> +	ret =3D bpf_probe_read_user_str(buf, sizeof(buf), ctx->buf);

bpf_probe_read_user_str() returns "long". Let's use "long ret;"

> +
> +	return 0;
> +}
> +
> +char _license[] SEC("license") =3D "GPL";
> --=20
> 2.28.0
>=20