All of lore.kernel.org
 help / color / mirror / Atom feed
From: Matt Bobrowski <mattbobrowski@google.com>
To: Shuran Liu <electronlsr@gmail.com>
Cc: song@kernel.org, bpf@vger.kernel.org, ast@kernel.org,
	daniel@iogearbox.net, andrii@kernel.org, martin.lau@linux.dev,
	eddyz87@gmail.com, yonghong.song@linux.dev,
	john.fastabend@gmail.com, kpsingh@kernel.org, sdf@fomichev.me,
	haoluo@google.com, jolsa@kernel.org, rostedt@goodmis.org,
	mhiramat@kernel.org, mathieu.desnoyers@efficios.com,
	linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org
Subject: Re: [PATCH bpf 0/2] bpf: fix bpf_d_path() helper prototype
Date: Mon, 1 Dec 2025 19:22:35 +0000	[thread overview]
Message-ID: <aS3q-wLcRFuCGuUG@google.com> (raw)
In-Reply-To: <20251201143813.5212-1-electronlsr@gmail.com>

On Mon, Dec 01, 2025 at 10:38:11PM +0800, Shuran Liu wrote:
> Hi,
> 
> this series fixes a verifier regression for bpf_d_path() introduced by
> commit 37cce22dbd51 ("bpf: verifier: Refactor helper access type
> tracking") and adds a small selftest to exercise the helper from an
> LSM program.
> 
> Commit 37cce22dbd51 started distinguishing read vs write accesses
> performed by helpers. bpf_d_path()'s buffer argument was left as
> ARG_PTR_TO_MEM without MEM_WRITE, so the verifier could incorrectly
> assume that the buffer contents are unchanged across the helper call
> and base its optimizations on this wrong assumption.
> 
> In practice this showed up as a misbehaving LSM BPF program that calls
> bpf_d_path() and then does a simple prefix comparison on the returned
> path: the program would sometimes take the "mismatch" branch even
> though both bytes being compared were actually equal.

FTR, I strongly encourage any new BPF LSM implementation to consider
using the newer BPF kfunc alternative instead, being
bpf_path_d_path().

> Patch 1 fixes bpf_d_path()'s helper prototype by marking the buffer
> argument as ARG_PTR_TO_MEM | MEM_WRITE, so that the verifier correctly
> models the write to the caller-provided buffer.

This is the correct thing to do, appreciate you sending through the
fix.

> Patch 2 adds a minimal selftest under tools/testing/selftests/bpf that
> hooks bprm_check_security, calls bpf_d_path() on a binary under /tmp/,
> and verifies that the prefix comparison on the returned path keeps
> working.

Makes sense to add a test for this regression, but please also see my
comments against this patch.

> On my local setup, tools/testing/selftests/bpf does not build fully
> due to unrelated tests using newer helpers. I validated this series by
> manually reproducing the issue with a small LSM program and by
> building and running only the new d_path_lsm test on kernels with and
> without patch 1 applied.
> 
> Thanks,
> Shuran Liu
> 
> Shuran Liu (2):
>   bpf: mark bpf_d_path() buffer as writeable
>   selftests/bpf: add regression test for bpf_d_path()
> 
>  kernel/trace/bpf_trace.c                      |  2 +-
>  .../selftests/bpf/prog_tests/d_path_lsm.c     | 27 ++++++++++++
>  .../selftests/bpf/progs/d_path_lsm.bpf.c      | 43 +++++++++++++++++++
>  3 files changed, 71 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/d_path_lsm.c
>  create mode 100644 tools/testing/selftests/bpf/progs/d_path_lsm.bpf.c
> 
> -- 
> 2.52.0
> 

      parent reply	other threads:[~2025-12-01 19:22 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-12-01 14:38 [PATCH bpf 0/2] bpf: fix bpf_d_path() helper prototype Shuran Liu
2025-12-01 14:38 ` [PATCH bpf 1/2] bpf: mark bpf_d_path() buffer as writeable Shuran Liu
2025-12-01 18:48   ` Matt Bobrowski
2025-12-01 14:38 ` [PATCH bpf 2/2] selftests/bpf: add regression test for bpf_d_path() Shuran Liu
2025-12-01 19:16   ` Matt Bobrowski
2025-12-01 19:22 ` Matt Bobrowski [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=aS3q-wLcRFuCGuUG@google.com \
    --to=mattbobrowski@google.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=eddyz87@gmail.com \
    --cc=electronlsr@gmail.com \
    --cc=haoluo@google.com \
    --cc=john.fastabend@gmail.com \
    --cc=jolsa@kernel.org \
    --cc=kpsingh@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-trace-kernel@vger.kernel.org \
    --cc=martin.lau@linux.dev \
    --cc=mathieu.desnoyers@efficios.com \
    --cc=mhiramat@kernel.org \
    --cc=rostedt@goodmis.org \
    --cc=sdf@fomichev.me \
    --cc=song@kernel.org \
    --cc=yonghong.song@linux.dev \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.