From mboxrd@z Thu Jan 1 00:00:00 1970 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=-9.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 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 47567C388F7 for ; Thu, 5 Nov 2020 19:28:31 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D440E2078E for ; Thu, 5 Nov 2020 19:28:30 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=dxuuu.xyz header.i=@dxuuu.xyz header.b="buYHyesk"; dkim=temperror (0-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b="IwHq6wkE" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731788AbgKET2a (ORCPT ); Thu, 5 Nov 2020 14:28:30 -0500 Received: from out4-smtp.messagingengine.com ([66.111.4.28]:50283 "EHLO out4-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727851AbgKET23 (ORCPT ); Thu, 5 Nov 2020 14:28:29 -0500 Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailout.nyi.internal (Postfix) with ESMTP id 31C695C019E; Thu, 5 Nov 2020 14:28:28 -0500 (EST) Received: from mailfrontend2 ([10.202.2.163]) by compute3.internal (MEProxy); Thu, 05 Nov 2020 14:28:28 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=dxuuu.xyz; h= mime-version:content-transfer-encoding:content-type:cc:subject :from:to:date:message-id:in-reply-to; s=fm1; bh=ehC9x8cE+ZGBKrco BMypR8Z8YvOTab6tJ2SFOzXL27c=; b=buYHyesk/b1Nhdb3//UKsAarJZURlfHZ F/wV+7xwvlPad2CqI+XXEJnwDwSfBbfK2clyrsERL8ja10WrEXyyxd98z8s1Mxot Vu+59CRVUrfL7JW0TBVtsIXlrxz0uimzHQvG+V3SyJ637ixDFLGGTtFyDj+KipWz AliPeqzttPrA46IfN258rVjpBByFqIS3Ve6mk6Mqo7WKj0T6i/IqiEZt/vaSEiXO FV46HiL89z2Ctv8MmL0kKK0UxOodkkG/bOQ8Kx+6HnhJLj6s6OpEj37pjGQCwuBd F1sE/Sx518OcZf5sbjfPPyuEfrcJE0vuXJmdKN5tL9TzgqdLjpndgg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:subject:to :x-me-proxy:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s= fm1; bh=ehC9x8cE+ZGBKrcoBMypR8Z8YvOTab6tJ2SFOzXL27c=; b=IwHq6wkE aW6yx9Kk0ROQy4/bAVB7oW7ygq4wCpa49jISgt1REXVjVtDAnAaHGLU+OHkHmQyA aaEsKYOAkeWYdHHghbztFpegJxbeRVv8OHVnUouiaWV2kVrOGYCKzWfgsVICO1uD NOZ0U3DeTuYmt0+R70PQ5ryPgIZqP3XNOqSRhtWbiLULcaHJzOsMgpaOeO0l3vJC oC1BEAubWbEMw2MVtnkjy9avgEcvv60tAXDA6F2Y7PAy7JN78Eaz0NcGWfj37H5I cA5tfRw/BPm8x6WFUzyT825hBv4/cE54bUZpwyeqdeGQii2kddAqOKWt6cIRkZLy P0AjlnqCUwATUQ== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedujedruddtjedguddvhecutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd enfghrlhcuvffnffculdejtddmnecujfgurhepggfgtgfuhffvfffkjgesthhqredttddt jeenucfhrhhomhepfdffrghnihgvlhcuighufdcuoegugihusegugihuuhhurdighiiiqe enucggtffrrghtthgvrhhnpeejfefhudeffefhjedvvefhheduledtueejvedugedvjedv jeeljefggedtjeejveenucfkphepieelrddukedurddutdehrdeigeenucevlhhushhtvg hrufhiiigvpedunecurfgrrhgrmhepmhgrihhlfhhrohhmpegugihusegugihuuhhurdig hiii X-ME-Proxy: Received: from localhost (c-69-181-105-64.hsd1.ca.comcast.net [69.181.105.64]) by mail.messagingengine.com (Postfix) with ESMTPA id 6F22D3060060; Thu, 5 Nov 2020 14:28:27 -0500 (EST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Cc: "bpf" , "linux-kernel@vger.kernel.org" , "ast@kernel.org" , "daniel@iogearbox.net" , "Kernel Team" Subject: Re: [PATCH bpf v2 1/2] lib/strncpy_from_user.c: Don't overcopy bytes after NUL terminator From: "Daniel Xu" To: "Song Liu" Date: Thu, 05 Nov 2020 11:28:11 -0800 Message-Id: In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org On Thu Nov 5, 2020 at 10:16 AM PST, Song Liu wrote: > > > > On Nov 4, 2020, at 6:25 PM, Daniel Xu wrote: > >=20 > > do_strncpy_from_user() may copy some extra bytes after the NUL > > We have multiple use of "NUL" here, should be "NULL"? > > > terminator into the destination buffer. This usually does not matter fo= r > > normal string operations. However, when BPF programs key BPF maps with > > strings, this matters a lot. > >=20 > > 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. > >=20 > > 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. > >=20 > > This commit uses the proper word-at-a-time APIs to avoid overcopying. > >=20 > > 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(-) > >=20 > > diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c > > index e6d5fcc2cdf3..d084189eb05c 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; > >=20 > > while (max >=3D sizeof(unsigned long)) { > > - unsigned long c, data; > > + unsigned long c, data, mask, *out; > >=20 > > /* 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)= ; > >=20 > > - *(unsigned long *)(dst+res) =3D c; > > if (has_zero(c, &data, &constants)) { > > data =3D prep_zero_mask(c, data, &constants); > > data =3D create_zero_mask(data); > > + mask =3D zero_bytemask(data); > > + out =3D (unsigned long *)(dst+res); > > + *out =3D (*out & ~mask) | (c & mask); > > return res + find_zero(data); > > + } else { > > This else clause is not needed, as we return in the if clause. Thanks, will change in v3. [..]