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=-3.6 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=no 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 98322C388F9 for ; Thu, 12 Nov 2020 01:51:33 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 3F7122068E for ; Thu, 12 Nov 2020 01:51:33 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="O4LUkFtR" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728294AbgKLBcJ (ORCPT ); Wed, 11 Nov 2020 20:32:09 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60360 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727890AbgKKXWc (ORCPT ); Wed, 11 Nov 2020 18:22:32 -0500 Received: from mail-yb1-xb42.google.com (mail-yb1-xb42.google.com [IPv6:2607:f8b0:4864:20::b42]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A7D0FC0613D1; Wed, 11 Nov 2020 15:22:32 -0800 (PST) Received: by mail-yb1-xb42.google.com with SMTP id c129so3506998yba.8; Wed, 11 Nov 2020 15:22:32 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=zLAINA497HzN+bAWRfYH3EEhA2UAQMIiVvTDxBBnWcw=; b=O4LUkFtRH65lwdUKgb7EIgdnmlBMdqIGl8bAIWrYe5aI8zx80wpnKIpdKFlNNytchF 3NoApHtOhlaVCBNmiV2czbYlGCnr15zBF+T/nChufjPUK7xM6AQDt2ObCND+LWPHDMFA QSEceKpzcbbxQ5px2r/mr5qk2b6wKT4VjCKlUXJkP/eMpH3GbBKVLuopBHgKM730byA0 9MPVGTmGRkJuqDwV7lO/SyegzKVEnMAw7KLh+L8MqyNgYOyTmuEFOiJWryx8Yh3BlFgX uc8vjsE0Zq3vduAWdSYW9y4bOkCgo8XSVWUdWRP1ZCtvXkeJWBVlmID4h+tMf6/kk7Qz Q42w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=zLAINA497HzN+bAWRfYH3EEhA2UAQMIiVvTDxBBnWcw=; b=HsaW6rzTRJkGyDOOPYvGEq+fwd2oNJpc5YcJTBD//lhqgZGtY/1uHZgISw32lrbWt9 2TH2l5tHIhDmyGLIgXN4BBLT+rglVN8n2/dWXoEQiUltkJcrt+b9WF77xQCG+zZjJLrK Kg54oXPuBi6hTrW8Dcu9eY+LCeW/J8+39yd80qOdU7U5/eleq4awIA5zFPcQvG+nxzeG W4D9UkFptsslnb0ptsBj0vwU7C+kEZfxqbYaujxZABIyLnUkj1xrTbpQLZzMbFv9u/Mn gSCPFWsrdUlUIeEiu5iMsb8htGQxAt4XK9ZmXSj5yY+6lwWUYCqVGyH68AgfXJp1B/hA QkhA== X-Gm-Message-State: AOAM5333lK8ZFXkG5as+WhADXqkErGCIAwHe9afAmiguFQFMHleuOhYK PCEKXH4zRV5z2cSSVZfD9Bh+LRiCKPqeRylstUP0sqdRFOxTDg== X-Google-Smtp-Source: ABdhPJx5/7pvemFIXy4D77xCw09+6AsnWpxxD7ASXsoX25rFgkB9QVSo2d/BG0rGiU6YQiUqvCPFo7dA7ht7QcwGsr8= X-Received: by 2002:a25:df8e:: with SMTP id w136mr10307254ybg.230.1605136951985; Wed, 11 Nov 2020 15:22:31 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Andrii Nakryiko Date: Wed, 11 Nov 2020 15:22:21 -0800 Message-ID: Subject: Re: [PATCH bpf v5 0/2] Fix bpf_probe_read_user_str() overcopying To: Daniel Xu Cc: bpf , open list , Alexei Starovoitov , Daniel Borkmann , Song Liu , Kernel Team Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org On Wed, Nov 11, 2020 at 2:46 PM Daniel Xu wrote: > > 6ae08ae3dea2 ("bpf: Add probe_read_{user, kernel} and probe_read_{user, > kernel}_str helpers") introduced a subtle bug where > bpf_probe_read_user_str() would potentially copy a few extra bytes after > the NUL terminator. > > This issue is particularly nefarious when strings are used as map keys, > as seemingly identical strings can occupy multiple entries in a map. > > This patchset fixes the issue and introduces a selftest to prevent > future regressions. > > v4 -> v5: > * don't read potentially uninitialized memory I think the bigger problem was that it could overwrite unintended memory. E.g., in BPF program, if you had something like: char my_buf[8 + 3]; char my_precious_data[5] = {1, 2, 3, 4, 5}; With previous version you'd overwrite my_precious data. BTW, do you test such scenario in the selftests you added? If not, we should have something like this as well and validate 1, 2, 3, 4, 5 stay intact. > > v3 -> v4: > * directly pass userspace pointer to prog > * test more strings of different length > > v2 -> v3: > * set pid filter before attaching prog in selftest > * use long instead of int as bpf_probe_read_user_str() retval > * style changes > > v1 -> v2: > * add Fixes: tag > * add selftest > > Daniel Xu (2): > lib/strncpy_from_user.c: Don't overcopy bytes after NUL terminator > selftest/bpf: Test bpf_probe_read_user_str() strips trailing bytes > after NUL > > lib/strncpy_from_user.c | 9 ++- > .../bpf/prog_tests/probe_read_user_str.c | 71 +++++++++++++++++++ > .../bpf/progs/test_probe_read_user_str.c | 25 +++++++ > 3 files changed, 100 insertions(+), 5 deletions(-) > 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 > > -- > 2.29.2 >