[PATCH bpf-next v2 0/3] bpf: Add user memory access kfuncs for linux_binprm

[PATCH bpf-next v2 0/3] bpf: Add user memory access kfuncs for linux_binprm

[Anastasios Papagiannis]

During exec, argument and environment strings are copied into the new
address space held by struct linux_binprm before that address space is
installed on the task_struct. Existing eBPF user memory helpers only
support reading from the old address space (i.e. current process)
and for this reason programs cannot access these strings from the
bprm_check_security LSM hook.

This series adds two sleepable BPF kfuncs for copying bytes or
NUL-terminated strings from the address space held by struct linux_binprm.
This allows BPF LSM programs to inspect exec arguments before allowing
the exec to continue.

The series also adds selftests covering both kfuncs when reading argument
and environment strings.

Changes in v2:
- Register the kfuncs on NOMMU systems and return -EOPNOTSUPP when called,
  as suggested by Justin Suess.
- Add selftest coverage for reading environment strings, as suggested by
  Justin Suess.
- Clarify that copy_remote_mm_str() leaves the destination untouched when
  called with a zero-length buffer.
- Use sizeof() instead of hardcoded argument lengths in the selftests.
- Use ~0ULL for invalid-flags checks in the selftests.

v1:
https://lore.kernel.org/bpf/20260812111140.7762-1-tasos.papagiannnis@gmail.com/

Anastasios Papagiannis (3):
  mm: Add copy_remote_mm_str()
  bpf: Add user memory access kfuncs for linux_binprm
  selftests/bpf: Test linux_binprm user memory kfuncs

 fs/bpf_fs_kfuncs.c                            | 128 ++++++++++++++++++
 include/linux/mm.h                            |   2 +
 mm/memory.c                                   |  26 +++-
 mm/nommu.c                                    |  26 +++-
 .../bpf/prog_tests/copy_from_user_bprm.c      |  62 +++++++++
 .../selftests/bpf/progs/copy_from_user_bprm.c | 115 ++++++++++++++++
 6 files changed, 357 insertions(+), 2 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/copy_from_user_bprm.c
 create mode 100644 tools/testing/selftests/bpf/progs/copy_from_user_bprm.c


base-commit: f79066c784022fda83f5936559a1af414e41b603
-- 
2.55.0

[PATCH bpf-next v2 1/3] mm: Add copy_remote_mm_str()

[Anastasios Papagiannis]

copy_remote_vm_str() gets the target address space from a struct
task_struct. This does not work for an address space that exists but is
not yet associated with a task_struct, such as the mm held by struct
linux_binprm during exec.

Add copy_remote_mm_str(), which operates directly on a struct mm_struct.

Signed-off-by: Anastasios Papagiannis <tasos.papagiannnis@gmail.com>
---
 include/linux/mm.h |  2 ++
 mm/memory.c        | 26 +++++++++++++++++++++++++-
 mm/nommu.c         | 26 +++++++++++++++++++++++++-
 3 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 485df9c2dbdd..eede435bf4a3 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -3222,6 +3222,8 @@ extern int access_remote_vm(struct mm_struct *mm, unsigned long addr,
 		void *buf, int len, unsigned int gup_flags);
 
 #ifdef CONFIG_BPF_SYSCALL
+extern int copy_remote_mm_str(struct mm_struct *mm, unsigned long addr,
+			      void *buf, int len, unsigned int gup_flags);
 extern int copy_remote_vm_str(struct task_struct *tsk, unsigned long addr,
 			      void *buf, int len, unsigned int gup_flags);
 #endif
diff --git a/mm/memory.c b/mm/memory.c
index 6b8280cfc1db..da0010c4475d 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -7218,6 +7218,30 @@ static int __copy_remote_vm_str(struct mm_struct *mm, unsigned long addr,
 	return buf - old_buf;
 }
 
+/**
+ * copy_remote_mm_str - copy a string from a remote address space.
+ * @mm:	the remote address space
+ * @addr:	start address to read from
+ * @buf:	destination buffer
+ * @len:	number of bytes to copy
+ * @gup_flags:	flags modifying lookup behaviour
+ *
+ * The caller must hold a reference on @mm.
+ *
+ * Return: number of bytes copied from @addr (source) to @buf (destination),
+ * not including the trailing NUL. If @len is zero, return 0 without accessing
+ * @buf. Otherwise, @buf is always NUL-terminated. On any error, return
+ * -EFAULT.
+ */
+int copy_remote_mm_str(struct mm_struct *mm, unsigned long addr,
+		       void *buf, int len, unsigned int gup_flags)
+{
+	if (unlikely(len == 0))
+		return 0;
+
+	return __copy_remote_vm_str(mm, addr, buf, len, gup_flags);
+}
+
 /**
  * copy_remote_vm_str - copy a string from another process's address space.
  * @tsk:	the task of the target address space
@@ -7247,7 +7271,7 @@ int copy_remote_vm_str(struct task_struct *tsk, unsigned long addr,
 		return -EFAULT;
 	}
 
-	ret = __copy_remote_vm_str(mm, addr, buf, len, gup_flags);
+	ret = copy_remote_mm_str(mm, addr, buf, len, gup_flags);
 
 	mmput(mm);
 
diff --git a/mm/nommu.c b/mm/nommu.c
index ed3934bc2de4..1fca0bb7f042 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -1752,6 +1752,30 @@ static int __copy_remote_vm_str(struct mm_struct *mm, unsigned long addr,
 	return ret;
 }
 
+/**
+ * copy_remote_mm_str - copy a string from a remote address space.
+ * @mm:         the remote address space
+ * @addr:       start address to read from
+ * @buf:        destination buffer
+ * @len:        number of bytes to copy
+ * @gup_flags:  flags modifying lookup behaviour (unused)
+ *
+ * The caller must hold a reference on @mm.
+ *
+ * Return: number of bytes copied from @addr (source) to @buf (destination),
+ * not including the trailing NUL. If @len is zero, return 0 without accessing
+ * @buf. Otherwise, @buf is always NUL-terminated. On any error, return
+ * -EFAULT.
+ */
+int copy_remote_mm_str(struct mm_struct *mm, unsigned long addr,
+		       void *buf, int len, unsigned int gup_flags)
+{
+	if (unlikely(len == 0))
+		return 0;
+
+	return __copy_remote_vm_str(mm, addr, buf, len);
+}
+
 /**
  * copy_remote_vm_str - copy a string from another process's address space.
  * @tsk:	the task of the target address space
@@ -1781,7 +1805,7 @@ int copy_remote_vm_str(struct task_struct *tsk, unsigned long addr,
 		return -EFAULT;
 	}
 
-	ret = __copy_remote_vm_str(mm, addr, buf, len);
+	ret = copy_remote_mm_str(mm, addr, buf, len, gup_flags);
 
 	mmput(mm);
 
-- 
2.55.0

Re: [PATCH bpf-next v2 1/3] mm: Add copy_remote_mm_str()

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 2837 bytes --]

> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 485df9c2dbdd..eede435bf4a3 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -3222,6 +3222,8 @@ extern int access_remote_vm(struct mm_struct *mm, unsigned long addr,
>  		void *buf, int len, unsigned int gup_flags);
>
>  #ifdef CONFIG_BPF_SYSCALL
> +extern int copy_remote_mm_str(struct mm_struct *mm, unsigned long addr,
> +			      void *buf, int len, unsigned int gup_flags);
>  extern int copy_remote_vm_str(struct task_struct *tsk, unsigned long addr,
>  			      void *buf, int len, unsigned int gup_flags);
>  #endif

This isn't a bug, but does copy_remote_mm_str() read as expected next to
copy_remote_vm_str(tsk, ...)? In this area of mm/memory.c, the
established split is access_process_vm(struct task_struct *tsk, ...) for
the task-based entry point and access_remote_vm(struct mm_struct *mm,
...) for the mm-based one. By that convention _remote_vm already denotes
the variant that takes an mm, so having copy_remote_mm_str(mm, ...)
sitting next to copy_remote_vm_str(tsk, ...) inverts the expectation.

> diff --git a/mm/memory.c b/mm/memory.c
> index 6b8280cfc1db..da0010c4475d 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -7218,6 +7218,30 @@ static int __copy_remote_vm_str(struct mm_struct *mm, unsigned long addr,
>  	return buf - old_buf;
>  }
>
> +/**
> + * copy_remote_mm_str - copy a string from a remote address space.
> + * @mm:	the remote address space
> + * @addr:	start address to read from
> + * @buf:	destination buffer
> + * @len:	number of bytes to copy
> + * @gup_flags:	flags modifying lookup behaviour
> + *
> + * The caller must hold a reference on @mm.
> + *
> + * Return: number of bytes copied from @addr (source) to @buf (destination),
> + * not including the trailing NUL. If @len is zero, return 0 without accessing
> + * @buf. Otherwise, @buf is always NUL-terminated. On any error, return
> + * -EFAULT.
> + */
> +int copy_remote_mm_str(struct mm_struct *mm, unsigned long addr,
> +		       void *buf, int len, unsigned int gup_flags)
> +{
> +	if (unlikely(len == 0))
> +		return 0;
> +
> +	return __copy_remote_vm_str(mm, addr, buf, len, gup_flags);
> +}
> +
>  /**
>   * copy_remote_vm_str - copy a string from another process's address space.
>   * @tsk:	the task of the target address space
> @@ -7247,7 +7271,7 @@ int copy_remote_vm_str(struct task_struct *tsk, unsigned long addr,
>  		return -EFAULT;
>  	}
>
> -	ret = __copy_remote_vm_str(mm, addr, buf, len, gup_flags);
> +	ret = copy_remote_mm_str(mm, addr, buf, len, gup_flags);
>
>  	mmput(mm);
>

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32375104040

Re: [PATCH bpf-next v2 1/3] mm: Add copy_remote_mm_str()

[Andrii Nakryiko]

On Thu, Aug 20, 2026 at 6:18 AM Anastasios Papagiannis
<tasos.papagiannnis@gmail.com> wrote:
>
> copy_remote_vm_str() gets the target address space from a struct
> task_struct. This does not work for an address space that exists but is
> not yet associated with a task_struct, such as the mm held by struct
> linux_binprm during exec.
>
> Add copy_remote_mm_str(), which operates directly on a struct mm_struct.
>
> Signed-off-by: Anastasios Papagiannis <tasos.papagiannnis@gmail.com>
> ---
>  include/linux/mm.h |  2 ++
>  mm/memory.c        | 26 +++++++++++++++++++++++++-
>  mm/nommu.c         | 26 +++++++++++++++++++++++++-
>  3 files changed, 52 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 485df9c2dbdd..eede435bf4a3 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -3222,6 +3222,8 @@ extern int access_remote_vm(struct mm_struct *mm, unsigned long addr,
>                 void *buf, int len, unsigned int gup_flags);
>
>  #ifdef CONFIG_BPF_SYSCALL
> +extern int copy_remote_mm_str(struct mm_struct *mm, unsigned long addr,
> +                             void *buf, int len, unsigned int gup_flags);
>  extern int copy_remote_vm_str(struct task_struct *tsk, unsigned long addr,
>                               void *buf, int len, unsigned int gup_flags);
>  #endif
> diff --git a/mm/memory.c b/mm/memory.c
> index 6b8280cfc1db..da0010c4475d 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -7218,6 +7218,30 @@ static int __copy_remote_vm_str(struct mm_struct *mm, unsigned long addr,
>         return buf - old_buf;
>  }
>
> +/**
> + * copy_remote_mm_str - copy a string from a remote address space.
> + * @mm:        the remote address space
> + * @addr:      start address to read from
> + * @buf:       destination buffer
> + * @len:       number of bytes to copy
> + * @gup_flags: flags modifying lookup behaviour
> + *
> + * The caller must hold a reference on @mm.
> + *
> + * Return: number of bytes copied from @addr (source) to @buf (destination),
> + * not including the trailing NUL. If @len is zero, return 0 without accessing
> + * @buf. Otherwise, @buf is always NUL-terminated. On any error, return
> + * -EFAULT.
> + */
> +int copy_remote_mm_str(struct mm_struct *mm, unsigned long addr,
> +                      void *buf, int len, unsigned int gup_flags)
> +{
> +       if (unlikely(len == 0))
> +               return 0;
> +
> +       return __copy_remote_vm_str(mm, addr, buf, len, gup_flags);

we should rename __copy_remote_vm_str into __copy_remote_mm_str

> +}
> +
>  /**
>   * copy_remote_vm_str - copy a string from another process's address space.
>   * @tsk:       the task of the target address space
> @@ -7247,7 +7271,7 @@ int copy_remote_vm_str(struct task_struct *tsk, unsigned long addr,
>                 return -EFAULT;
>         }
>
> -       ret = __copy_remote_vm_str(mm, addr, buf, len, gup_flags);
> +       ret = copy_remote_mm_str(mm, addr, buf, len, gup_flags);
>
>         mmput(mm);
>
> diff --git a/mm/nommu.c b/mm/nommu.c
> index ed3934bc2de4..1fca0bb7f042 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -1752,6 +1752,30 @@ static int __copy_remote_vm_str(struct mm_struct *mm, unsigned long addr,
>         return ret;
>  }
>
> +/**
> + * copy_remote_mm_str - copy a string from a remote address space.
> + * @mm:         the remote address space
> + * @addr:       start address to read from
> + * @buf:        destination buffer
> + * @len:        number of bytes to copy
> + * @gup_flags:  flags modifying lookup behaviour (unused)
> + *
> + * The caller must hold a reference on @mm.
> + *
> + * Return: number of bytes copied from @addr (source) to @buf (destination),
> + * not including the trailing NUL. If @len is zero, return 0 without accessing
> + * @buf. Otherwise, @buf is always NUL-terminated. On any error, return
> + * -EFAULT.
> + */
> +int copy_remote_mm_str(struct mm_struct *mm, unsigned long addr,
> +                      void *buf, int len, unsigned int gup_flags)
> +{
> +       if (unlikely(len == 0))
> +               return 0;

I'd just move this len check into __copy_remote_mm_str()


> +
> +       return __copy_remote_vm_str(mm, addr, buf, len);
> +}
> +
>  /**
>   * copy_remote_vm_str - copy a string from another process's address space.
>   * @tsk:       the task of the target address space
> @@ -1781,7 +1805,7 @@ int copy_remote_vm_str(struct task_struct *tsk, unsigned long addr,
>                 return -EFAULT;
>         }
>
> -       ret = __copy_remote_vm_str(mm, addr, buf, len);
> +       ret = copy_remote_mm_str(mm, addr, buf, len, gup_flags);
>
>         mmput(mm);
>
> --
> 2.55.0
>

[PATCH bpf-next v2 2/3] bpf: Add user memory access kfuncs for linux_binprm

[Anastasios Papagiannis]

When security_bprm_check runs, the arg and env strings for the exec have
been copied into bprm->mm. The new address space has not been associated
yet with a task_struct until exec_mmap(), so existing BPF user memory
helpers can only read from the calling task's old address space.

This patch adds bpf_copy_from_user_bprm() and
bpf_copy_from_user_bprm_str() kfuncs. Both use the mm_struct provided by
struct linux_binprm.

Register these kfuncs on both MMU and NOMMU systems. On NOMMU systems,
exec arguments are staged in bprm->page[] rather than mapped in bprm->mm,
so the kfuncs return -EOPNOTSUPP.

bpf_copy_from_user_bprm() has similar semantics as
bpf_copy_from_user_task(). bpf_copy_from_user_bprm_str() copies one
NUL-terminated string and returns its size including the NUL terminator.
It accepts BPF_F_PAD_ZEROS to clear unused destination bytes on success.

This patch registers both kfuncs with KF_SLEEPABLE because accessing the
remote address space can fault. This allows BPF LSM programs attached to
security_bprm_check to read arguments beginning at bprm->p and reject an
exec based on its command-line arguments.

Signed-off-by: Anastasios Papagiannis <tasos.papagiannnis@gmail.com>
---
 fs/bpf_fs_kfuncs.c | 128 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 128 insertions(+)

diff --git a/fs/bpf_fs_kfuncs.c b/fs/bpf_fs_kfuncs.c
index f1863a891db6..1cd06f9e4890 100644
--- a/fs/bpf_fs_kfuncs.c
+++ b/fs/bpf_fs_kfuncs.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright (c) 2024 Google LLC. */
 
+#include <linux/binfmts.h>
 #include <linux/bpf.h>
 #include <linux/bpf_lsm.h>
 #include <linux/btf.h>
@@ -379,6 +380,131 @@ __bpf_kfunc struct inode *bpf_real_data_inode(struct file *file)
 	return d_real_inode(file_dentry(file));
 }
 
+/**
+ * bpf_copy_from_user_bprm - Copy data from a binary parameter address space
+ * @dst:             Destination address, in kernel space
+ * @dst__sz:         Number of bytes to copy
+ * @unsafe_ptr__ign: Source address in the binary parameter address space
+ * @bprm:            Binary parameters whose address space will be used
+ * @flags:           Reserved for future use; must be zero
+ *
+ * Copies data from the nascent address space associated with @bprm. This is
+ * useful for reading the argument and environment strings before the new
+ * address space is installed by exec_mmap(). For example, at the
+ * bprm_check_security LSM hook, @bprm->p points at the first argument string.
+ *
+ * The destination is zeroed if the requested number of bytes cannot be copied
+ * in full.
+ *
+ * Return: 0 on success, -EINVAL if @flags is non-zero, -EOPNOTSUPP on
+ * NOMMU systems, or -EFAULT if the copy fails or is partial.
+ */
+__bpf_kfunc int bpf_copy_from_user_bprm(void *dst, u32 dst__sz,
+					const void __user *unsafe_ptr__ign,
+					const struct linux_binprm *bprm, u64 flags)
+{
+#ifdef CONFIG_MMU
+	struct mm_struct *mm;
+	int ret;
+#endif
+
+	if (unlikely(flags))
+		return -EINVAL;
+
+	if (unlikely(!dst__sz))
+		return 0;
+
+#ifdef CONFIG_MMU
+	mm = bprm->mm;
+	if (!mm) {
+		memset(dst, 0, dst__sz);
+		return -EFAULT;
+	}
+
+	ret = access_remote_vm(mm, (unsigned long)unsafe_ptr__ign,
+			       dst, dst__sz, 0);
+	if (ret != dst__sz) {
+		memset(dst, 0, dst__sz);
+		return -EFAULT;
+	}
+
+	return 0;
+#else
+	memset(dst, 0, dst__sz);
+	return -EOPNOTSUPP;
+#endif
+}
+
+/**
+ * bpf_copy_from_user_bprm_str - Copy a string from binary parameter memory
+ * @dst:             Destination address, in kernel space. This buffer must be
+ *                   at least @dst__sz bytes long
+ * @dst__sz:         Maximum number of bytes to copy, including the trailing NUL
+ * @unsafe_ptr__ign: Source address in the binary parameter address space
+ * @bprm:            Binary parameters whose address space will be used
+ * @flags:           The only supported flag is BPF_F_PAD_ZEROS
+ *
+ * Copies a NUL-terminated string from the nascent address space associated
+ * with @bprm. If the string is too long, @dst is still NUL-terminated unless
+ * @dst__sz is zero.
+ *
+ * If BPF_F_PAD_ZEROS is set, the unused portion of @dst is cleared on success
+ * and all of @dst is cleared on failure.
+ *
+ * Return: The number of copied bytes including the NUL terminator on success,
+ * or a negative error code on failure. On NOMMU systems, -EOPNOTSUPP is
+ * returned.
+ */
+__bpf_kfunc int bpf_copy_from_user_bprm_str(void *dst, u32 dst__sz,
+					    const void __user *unsafe_ptr__ign,
+					    const struct linux_binprm *bprm,
+					    u64 flags)
+{
+#ifdef CONFIG_MMU
+	struct mm_struct *mm;
+	int ret;
+#endif
+
+	if (unlikely(flags & ~BPF_F_PAD_ZEROS))
+		return -EINVAL;
+
+	if (unlikely(!dst__sz))
+		return 0;
+
+#ifdef CONFIG_MMU
+	mm = bprm->mm;
+	if (!mm) {
+		if (flags & BPF_F_PAD_ZEROS)
+			memset(dst, 0, dst__sz);
+		else
+			*(char *)dst = '\0';
+
+		return -EFAULT;
+	}
+
+	ret = copy_remote_mm_str(mm, (unsigned long)unsafe_ptr__ign,
+				 dst, dst__sz, 0);
+	if (ret < 0) {
+		if (flags & BPF_F_PAD_ZEROS)
+			memset(dst, 0, dst__sz);
+
+		return ret;
+	}
+
+	if (flags & BPF_F_PAD_ZEROS)
+		memset(dst + ret, 0, dst__sz - ret);
+
+	return ret + 1;
+#else
+	if (flags & BPF_F_PAD_ZEROS)
+		memset(dst, 0, dst__sz);
+	else
+		*(char *)dst = '\0';
+
+	return -EOPNOTSUPP;
+#endif
+}
+
 __bpf_kfunc_end_defs();
 
 BTF_KFUNCS_START(bpf_fs_kfunc_set_ids)
@@ -390,6 +516,8 @@ BTF_ID_FLAGS(func, bpf_get_file_xattr, KF_SLEEPABLE)
 BTF_ID_FLAGS(func, bpf_set_dentry_xattr, KF_SLEEPABLE)
 BTF_ID_FLAGS(func, bpf_remove_dentry_xattr, KF_SLEEPABLE)
 BTF_ID_FLAGS(func, bpf_real_data_inode, KF_SLEEPABLE | KF_RET_NULL)
+BTF_ID_FLAGS(func, bpf_copy_from_user_bprm, KF_SLEEPABLE)
+BTF_ID_FLAGS(func, bpf_copy_from_user_bprm_str, KF_SLEEPABLE)
 BTF_KFUNCS_END(bpf_fs_kfunc_set_ids)
 
 static int bpf_fs_kfuncs_filter(const struct bpf_prog *prog, u32 kfunc_id)
-- 
2.55.0

Re: [PATCH bpf-next v2 2/3] bpf: Add user memory access kfuncs for linux_binprm

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 1831 bytes --]

> bpf: Add user memory access kfuncs for linux_binprm
>
> When security_bprm_check runs, the arg and env strings for the exec have
> been copied into bprm->mm. The new address space has not been associated
> yet with a task_struct until exec_mmap(), so existing BPF user memory
> helpers can only read from the calling task's old address space.
>
> This patch adds bpf_copy_from_user_bprm() and
> bpf_copy_from_user_bprm_str() kfuncs. Both use the mm_struct provided by
> struct linux_binprm.

This isn't a bug, but could the opener be reworded to avoid "This patch
adds"? Documentation/process/submitting-patches.rst asks for imperative mood
("Add bpf_copy_from_user_bprm() ...").

> Register these kfuncs on both MMU and NOMMU systems. On NOMMU systems,
> exec arguments are staged in bprm->page[] rather than mapped in bprm->mm,
> so the kfuncs return -EOPNOTSUPP.
>
> bpf_copy_from_user_bprm() has similar semantics as
> bpf_copy_from_user_task(). bpf_copy_from_user_bprm_str() copies one
> NUL-terminated string and returns its size including the NUL terminator.
> It accepts BPF_F_PAD_ZEROS to clear unused destination bytes on success.
>
> This patch registers both kfuncs with KF_SLEEPABLE because accessing the
> remote address space can fault. This allows BPF LSM programs attached to
> security_bprm_check to read arguments beginning at bprm->p and reject an
> exec based on its command-line arguments.

Same for "This patch registers" here -- would "Register both kfuncs with
KF_SLEEPABLE ..." fit better?

> Signed-off-by: Anastasios Papagiannis <tasos.papagiannnis@gmail.com>


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32375104040

Re: [PATCH bpf-next v2 2/3] bpf: Add user memory access kfuncs for linux_binprm

[Andrii Nakryiko]

On Thu, Aug 20, 2026 at 6:18 AM Anastasios Papagiannis
<tasos.papagiannnis@gmail.com> wrote:
>
> When security_bprm_check runs, the arg and env strings for the exec have
> been copied into bprm->mm. The new address space has not been associated
> yet with a task_struct until exec_mmap(), so existing BPF user memory
> helpers can only read from the calling task's old address space.
>
> This patch adds bpf_copy_from_user_bprm() and
> bpf_copy_from_user_bprm_str() kfuncs. Both use the mm_struct provided by
> struct linux_binprm.
>
> Register these kfuncs on both MMU and NOMMU systems. On NOMMU systems,
> exec arguments are staged in bprm->page[] rather than mapped in bprm->mm,
> so the kfuncs return -EOPNOTSUPP.
>
> bpf_copy_from_user_bprm() has similar semantics as
> bpf_copy_from_user_task(). bpf_copy_from_user_bprm_str() copies one
> NUL-terminated string and returns its size including the NUL terminator.
> It accepts BPF_F_PAD_ZEROS to clear unused destination bytes on success.
>
> This patch registers both kfuncs with KF_SLEEPABLE because accessing the
> remote address space can fault. This allows BPF LSM programs attached to
> security_bprm_check to read arguments beginning at bprm->p and reject an
> exec based on its command-line arguments.
>
> Signed-off-by: Anastasios Papagiannis <tasos.papagiannnis@gmail.com>
> ---
>  fs/bpf_fs_kfuncs.c | 128 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 128 insertions(+)
>
> diff --git a/fs/bpf_fs_kfuncs.c b/fs/bpf_fs_kfuncs.c
> index f1863a891db6..1cd06f9e4890 100644
> --- a/fs/bpf_fs_kfuncs.c
> +++ b/fs/bpf_fs_kfuncs.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /* Copyright (c) 2024 Google LLC. */
>
> +#include <linux/binfmts.h>
>  #include <linux/bpf.h>
>  #include <linux/bpf_lsm.h>
>  #include <linux/btf.h>
> @@ -379,6 +380,131 @@ __bpf_kfunc struct inode *bpf_real_data_inode(struct file *file)
>         return d_real_inode(file_dentry(file));
>  }
>
> +/**
> + * bpf_copy_from_user_bprm - Copy data from a binary parameter address space
> + * @dst:             Destination address, in kernel space
> + * @dst__sz:         Number of bytes to copy
> + * @unsafe_ptr__ign: Source address in the binary parameter address space
> + * @bprm:            Binary parameters whose address space will be used
> + * @flags:           Reserved for future use; must be zero
> + *
> + * Copies data from the nascent address space associated with @bprm. This is
> + * useful for reading the argument and environment strings before the new
> + * address space is installed by exec_mmap(). For example, at the
> + * bprm_check_security LSM hook, @bprm->p points at the first argument string.
> + *
> + * The destination is zeroed if the requested number of bytes cannot be copied
> + * in full.
> + *
> + * Return: 0 on success, -EINVAL if @flags is non-zero, -EOPNOTSUPP on
> + * NOMMU systems, or -EFAULT if the copy fails or is partial.
> + */
> +__bpf_kfunc int bpf_copy_from_user_bprm(void *dst, u32 dst__sz,
> +                                       const void __user *unsafe_ptr__ign,
> +                                       const struct linux_binprm *bprm, u64 flags)

making this linux_binprm-specific seems short-sighted and overly
specialized, why not pass `const struct mm *` and call new kfuncs
bpf_copy_from_user_mm[_str] ?

and this seems to belong right next to bpf_copy_from_user_task_str()
and others in kernel/bpf/helpers.c (and bpf_copy_from_user_task
variants should just delegate to this mm-based APIs and minimize code
duplication)

pw-bot: cr

> +{
> +#ifdef CONFIG_MMU
> +       struct mm_struct *mm;
> +       int ret;
> +#endif
> +
> +       if (unlikely(flags))
> +               return -EINVAL;
> +
> +       if (unlikely(!dst__sz))
> +               return 0;
> +
> +#ifdef CONFIG_MMU
> +       mm = bprm->mm;
> +       if (!mm) {
> +               memset(dst, 0, dst__sz);
> +               return -EFAULT;
> +       }
> +
> +       ret = access_remote_vm(mm, (unsigned long)unsafe_ptr__ign,
> +                              dst, dst__sz, 0);
> +       if (ret != dst__sz) {
> +               memset(dst, 0, dst__sz);
> +               return -EFAULT;
> +       }
> +
> +       return 0;
> +#else
> +       memset(dst, 0, dst__sz);
> +       return -EOPNOTSUPP;
> +#endif
> +}
> +
> +/**
> + * bpf_copy_from_user_bprm_str - Copy a string from binary parameter memory
> + * @dst:             Destination address, in kernel space. This buffer must be
> + *                   at least @dst__sz bytes long
> + * @dst__sz:         Maximum number of bytes to copy, including the trailing NUL
> + * @unsafe_ptr__ign: Source address in the binary parameter address space
> + * @bprm:            Binary parameters whose address space will be used
> + * @flags:           The only supported flag is BPF_F_PAD_ZEROS
> + *
> + * Copies a NUL-terminated string from the nascent address space associated
> + * with @bprm. If the string is too long, @dst is still NUL-terminated unless
> + * @dst__sz is zero.
> + *
> + * If BPF_F_PAD_ZEROS is set, the unused portion of @dst is cleared on success
> + * and all of @dst is cleared on failure.
> + *
> + * Return: The number of copied bytes including the NUL terminator on success,
> + * or a negative error code on failure. On NOMMU systems, -EOPNOTSUPP is
> + * returned.
> + */
> +__bpf_kfunc int bpf_copy_from_user_bprm_str(void *dst, u32 dst__sz,
> +                                           const void __user *unsafe_ptr__ign,
> +                                           const struct linux_binprm *bprm,
> +                                           u64 flags)
> +{
> +#ifdef CONFIG_MMU
> +       struct mm_struct *mm;
> +       int ret;
> +#endif
> +
> +       if (unlikely(flags & ~BPF_F_PAD_ZEROS))
> +               return -EINVAL;
> +
> +       if (unlikely(!dst__sz))
> +               return 0;
> +
> +#ifdef CONFIG_MMU
> +       mm = bprm->mm;
> +       if (!mm) {
> +               if (flags & BPF_F_PAD_ZEROS)
> +                       memset(dst, 0, dst__sz);
> +               else
> +                       *(char *)dst = '\0';
> +
> +               return -EFAULT;
> +       }
> +
> +       ret = copy_remote_mm_str(mm, (unsigned long)unsafe_ptr__ign,
> +                                dst, dst__sz, 0);
> +       if (ret < 0) {
> +               if (flags & BPF_F_PAD_ZEROS)
> +                       memset(dst, 0, dst__sz);
> +
> +               return ret;
> +       }
> +
> +       if (flags & BPF_F_PAD_ZEROS)
> +               memset(dst + ret, 0, dst__sz - ret);
> +
> +       return ret + 1;
> +#else
> +       if (flags & BPF_F_PAD_ZEROS)
> +               memset(dst, 0, dst__sz);
> +       else
> +               *(char *)dst = '\0';
> +
> +       return -EOPNOTSUPP;
> +#endif
> +}
> +
>  __bpf_kfunc_end_defs();
>
>  BTF_KFUNCS_START(bpf_fs_kfunc_set_ids)
> @@ -390,6 +516,8 @@ BTF_ID_FLAGS(func, bpf_get_file_xattr, KF_SLEEPABLE)
>  BTF_ID_FLAGS(func, bpf_set_dentry_xattr, KF_SLEEPABLE)
>  BTF_ID_FLAGS(func, bpf_remove_dentry_xattr, KF_SLEEPABLE)
>  BTF_ID_FLAGS(func, bpf_real_data_inode, KF_SLEEPABLE | KF_RET_NULL)
> +BTF_ID_FLAGS(func, bpf_copy_from_user_bprm, KF_SLEEPABLE)
> +BTF_ID_FLAGS(func, bpf_copy_from_user_bprm_str, KF_SLEEPABLE)
>  BTF_KFUNCS_END(bpf_fs_kfunc_set_ids)
>
>  static int bpf_fs_kfuncs_filter(const struct bpf_prog *prog, u32 kfunc_id)
> --
> 2.55.0
>

Re: [PATCH bpf-next 2/3] bpf: Add user memory access kfuncs for linux_binprm

[Anastasios Papagiannis]

Thanks for the review!

> making this linux_binprm-specific seems short-sighted and overly
> specialized, why not pass `const struct mm *` and call new kfuncs
> bpf_copy_from_user_mm[_str] ?

> and this seems to belong right next to bpf_copy_from_user_task_str()
> and others in kernel/bpf/helpers.c (and bpf_copy_from_user_task
> variants should just delegate to this mm-based APIs and minimize code
> duplication)

This is exactly what I did in my previous set of patches:
https://lore.kernel.org/bpf/20260811112154.94053-1-tasos.papagiannnis@gmail.com/T/
https://patchwork.kernel.org/project/netdevbpf/list/?series=1144046&state=*
https://sashiko.dev/#/patchset/20260811112154.94053-1-tasos.papagiannnis%40gmail.com
https://github.com/kernel-patches/bpf/pull/13211

where I got AI reviews with 2 main issues:

1.  Does marking bprm->mm as trusted-or-null here create a use-after-free
    vulnerability on the execve error path?
    Looking at free_bprm() in fs/exec.c, when an error occurs:
    fs/exec.c:free_bprm() {
        ...
        if (bprm->mm) {
            acct_arg_size(bprm, 0);
            mmput(bprm->mm);
        }
        ...
    }
    The object is freed via mmput(), but the bprm->mm pointer is not set to NULL.
    Could subsequent traceable cleanup functions (such as free_arg_pages) expose
    this dangling reference to BPF programs, bypassing the verifier's safety
    guarantees?

This is something that I believe is easy to fix.

2.  Does this break the BPF load-compatibility guarantee documented in
    Documentation/bpf/bpf_design_QA.rst?

    Before this patch, walking bprm->mm produced a dereferenceable register
    in every program type. In sleepable programs, in_rcu_cs() is false, so
    check_ptr_to_btf_access() sets flag = PTR_UNTRUSTED. In non-sleepable
    programs, in_rcu_cs() is true but mm is not in any __safe_rcu* list, so
    clear_trusted_flags() produces a plain PTR_TO_BTF_ID.

    Both PTR_TO_BTF_ID and PTR_TO_BTF_ID|PTR_UNTRUSTED satisfy the guard in
    check_mem_access():

    } else if (base_type(reg->type) == PTR_TO_BTF_ID &&
                !type_may_be_null(reg->type)) {
            err = check_ptr_to_btf_access(env, regs, reg, argno, off, size, t,
                                            value_regno);

    so bprm->mm->arg_start loads with no NULL check.

    After this patch, btf_nested_type_is_trusted() matches
    linux_binprm__safe_trusted_or_null.mm, so check_ptr_to_btf_access() sets
    flag |= PTR_TRUSTED | PTR_MAYBE_NULL. Now type_may_be_null() is true, the
    arm above is skipped, and control falls to the final else:

    } else {
            verbose(env, "%s invalid mem access '%s'\n", reg_arg_name(env, argno),
                    reg_type_str(env, reg->type));
            return -EACCES;
    }

    Any existing program that reads through bprm->mm without an explicit NULL
    check now fails bpf_prog_load() with -EACCES.

This sounds to me a bit more complicated. In a prog attached to lsm/security_bprm_check,
bprm is trusted but bprm->mm is not trusted. So we need a way to have bprm->mm trusted
to pass it as an argument to the helpers. In that series, I marked linux_binprm->mm as
trusted-or-null, which breaks all existing programs that use
linux_binprm->mm->something.

Another approach would be to add one more kfunc (i.e. bpf_get_bprm_mm) to get
the mm from bprm and have it trusted.

The reason that I didn't push that further was that mm in general is
bound to either linux_binprm or task_struct during its lifetime. We already
have helpers for task_struct, and I thought that adding helpers for linux_binprm
would be enough. Other than that, there is only one security_*() function that
takes mm_struct as an argument (i.e. security_vm_enough_memory_mm), and possibly
in that case we do not need to read the memory contents, but only metadata
(possibly I am oversimplifying here, but this is to show my thinking).

Unless I am missing something obvious, I can see two possible approaches here:
  1. Keep the bprm-specific kfuncs as they are now.
  2. Rework this around the mm variants that you proposed and add one
     more kfunc (i.e. bpf_get_bprm_mm) to make that work.

Does the second approach sound reasonable to you?

Thanks,
Anastasios

Re: [PATCH bpf-next 2/3] bpf: Add user memory access kfuncs for linux_binprm

[Andrii Nakryiko]

On Mon, Aug 24, 2026 at 3:49 AM Anastasios Papagiannis
<tasos.papagiannnis@gmail.com> wrote:
>
> Thanks for the review!
>
> > making this linux_binprm-specific seems short-sighted and overly
> > specialized, why not pass `const struct mm *` and call new kfuncs
> > bpf_copy_from_user_mm[_str] ?
>
> > and this seems to belong right next to bpf_copy_from_user_task_str()
> > and others in kernel/bpf/helpers.c (and bpf_copy_from_user_task
> > variants should just delegate to this mm-based APIs and minimize code
> > duplication)
>
> This is exactly what I did in my previous set of patches:
> https://lore.kernel.org/bpf/20260811112154.94053-1-tasos.papagiannnis@gmail.com/T/
> https://patchwork.kernel.org/project/netdevbpf/list/?series=1144046&state=*
> https://sashiko.dev/#/patchset/20260811112154.94053-1-tasos.papagiannnis%40gmail.com
> https://github.com/kernel-patches/bpf/pull/13211
>
> where I got AI reviews with 2 main issues:
>
> 1.  Does marking bprm->mm as trusted-or-null here create a use-after-free
>     vulnerability on the execve error path?
>     Looking at free_bprm() in fs/exec.c, when an error occurs:
>     fs/exec.c:free_bprm() {
>         ...
>         if (bprm->mm) {
>             acct_arg_size(bprm, 0);
>             mmput(bprm->mm);
>         }
>         ...
>     }
>     The object is freed via mmput(), but the bprm->mm pointer is not set to NULL.
>     Could subsequent traceable cleanup functions (such as free_arg_pages) expose
>     this dangling reference to BPF programs, bypassing the verifier's safety
>     guarantees?
>
> This is something that I believe is easy to fix.
>
> 2.  Does this break the BPF load-compatibility guarantee documented in
>     Documentation/bpf/bpf_design_QA.rst?
>
>     Before this patch, walking bprm->mm produced a dereferenceable register
>     in every program type. In sleepable programs, in_rcu_cs() is false, so
>     check_ptr_to_btf_access() sets flag = PTR_UNTRUSTED. In non-sleepable
>     programs, in_rcu_cs() is true but mm is not in any __safe_rcu* list, so
>     clear_trusted_flags() produces a plain PTR_TO_BTF_ID.
>
>     Both PTR_TO_BTF_ID and PTR_TO_BTF_ID|PTR_UNTRUSTED satisfy the guard in
>     check_mem_access():
>
>     } else if (base_type(reg->type) == PTR_TO_BTF_ID &&
>                 !type_may_be_null(reg->type)) {
>             err = check_ptr_to_btf_access(env, regs, reg, argno, off, size, t,
>                                             value_regno);
>
>     so bprm->mm->arg_start loads with no NULL check.
>
>     After this patch, btf_nested_type_is_trusted() matches
>     linux_binprm__safe_trusted_or_null.mm, so check_ptr_to_btf_access() sets
>     flag |= PTR_TRUSTED | PTR_MAYBE_NULL. Now type_may_be_null() is true, the
>     arm above is skipped, and control falls to the final else:
>
>     } else {
>             verbose(env, "%s invalid mem access '%s'\n", reg_arg_name(env, argno),
>                     reg_type_str(env, reg->type));
>             return -EACCES;
>     }
>
>     Any existing program that reads through bprm->mm without an explicit NULL
>     check now fails bpf_prog_load() with -EACCES.
>
> This sounds to me a bit more complicated. In a prog attached to lsm/security_bprm_check,
> bprm is trusted but bprm->mm is not trusted. So we need a way to have bprm->mm trusted
> to pass it as an argument to the helpers. In that series, I marked linux_binprm->mm as
> trusted-or-null, which breaks all existing programs that use
> linux_binprm->mm->something.

this is probably ok, but also we might want to look into having
verifier still allow to dereference trusted ptr-or-null into untrusted
ptr_to_btf_id to help with smooth transitions like this.

Not sure how complicated that would be in the verifier, but that would
be a nice backwards compat preserving behavior.

But to keep things simple today, I'd just mark that bprm->mm pointer
as TRUSTED_OR_NULL.

>
> Another approach would be to add one more kfunc (i.e. bpf_get_bprm_mm) to get
> the mm from bprm and have it trusted.

sound quite specialized, but I don't have a strong opinion

>
> The reason that I didn't push that further was that mm in general is
> bound to either linux_binprm or task_struct during its lifetime. We already
> have helpers for task_struct, and I thought that adding helpers for linux_binprm
> would be enough. Other than that, there is only one security_*() function that
> takes mm_struct as an argument (i.e. security_vm_enough_memory_mm), and possibly
> in that case we do not need to read the memory contents, but only metadata
> (possibly I am oversimplifying here, but this is to show my thinking).
>
> Unless I am missing something obvious, I can see two possible approaches here:
>   1. Keep the bprm-specific kfuncs as they are now.

no, I think that's a bad idea

>   2. Rework this around the mm variants that you proposed and add one
>      more kfunc (i.e. bpf_get_bprm_mm) to make that work.
>
> Does the second approach sound reasonable to you?

yes

>
> Thanks,
> Anastasios

Re: [PATCH bpf-next 2/3] bpf: Add user memory access kfuncs for linux_binprm

[Anastasios Papagiannis]

> this is probably ok, but also we might want to look into having
> verifier still allow to dereference trusted ptr-or-null into untrusted
> ptr_to_btf_id to help with smooth transitions like this.

> Not sure how complicated that would be in the verifier, but that would
> be a nice backwards compat preserving behavior.

I have just submitted an updated version of this series that also allows
to dereference trusted ptr-or-null into untrusted ptr_to_btf_id in
https://lore.kernel.org/bpf/20260831092305.42062-1-tasos.papagiannnis@gmail.com/T/#t.

> But to keep things simple today, I'd just mark that bprm->mm pointer
> as TRUSTED_OR_NULL.

If you think v3 is too complicated, or that the trusted-or-null
dereference support should be split into a separate series, please let
me know. In that case, I will simply mark the bprm->mm pointer as
TRUSTED_OR_NULL.

Thanks,
Anastasios

[PATCH bpf-next v2 3/3] selftests/bpf: Test linux_binprm user memory kfuncs

[Anastasios Papagiannis]

This patch adds a sleepable BPF LSM program attached to
bprm_check_security to test bpf_copy_from_user_bprm() and
bpf_copy_from_user_bprm_str().

Starting at bprm->p, verify that bpf_copy_from_user_bprm() can copy the
contiguous NUL-separated argument and environment data. Then
use bpf_copy_from_user_bprm_str() to read each argument and environment
string separately, by advancing the offset based on the returned length.

Signed-off-by: Anastasios Papagiannis <tasos.papagiannnis@gmail.com>
---
 .../bpf/prog_tests/copy_from_user_bprm.c      |  62 ++++++++++
 .../selftests/bpf/progs/copy_from_user_bprm.c | 115 ++++++++++++++++++
 2 files changed, 177 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/copy_from_user_bprm.c
 create mode 100644 tools/testing/selftests/bpf/progs/copy_from_user_bprm.c

diff --git a/tools/testing/selftests/bpf/prog_tests/copy_from_user_bprm.c b/tools/testing/selftests/bpf/prog_tests/copy_from_user_bprm.c
new file mode 100644
index 000000000000..1007e3aec054
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/copy_from_user_bprm.c
@@ -0,0 +1,62 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <errno.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+#include <test_progs.h>
+
+#include "copy_from_user_bprm.skel.h"
+
+void test_copy_from_user_bprm(void)
+{
+	char arg0[] = "first";
+	char arg1[] = "second-argument";
+	char env0[] = "SOME_ENV=a";
+	char env1[] = "OTHER_ENV=something";
+	struct copy_from_user_bprm *skel;
+	pid_t child;
+	int status;
+
+	skel = copy_from_user_bprm__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "open_and_load"))
+		return;
+
+	if (!ASSERT_OK(copy_from_user_bprm__attach(skel), "attach"))
+		goto out;
+
+	child = fork();
+	if (!ASSERT_GE(child, 0, "fork"))
+		goto out;
+
+	if (!child) {
+		char *const argv[] = { arg0, arg1, NULL };
+		char *const envp[] = { env0, env1, NULL };
+
+		skel->bss->monitored_pid = getpid();
+		execve("/bin/true", argv, envp);
+		_exit(errno);
+	}
+
+	if (!ASSERT_EQ(waitpid(child, &status, 0), child, "waitpid"))
+		goto out;
+
+	if (ASSERT_TRUE(WIFEXITED(status), "child_exited"))
+		ASSERT_EQ(WEXITSTATUS(status), EPERM, "exec_errno");
+
+	ASSERT_EQ(skel->bss->bprm_argc, 2, "bprm_argc");
+	ASSERT_EQ(skel->bss->bprm_envc, 2, "bprm_envc");
+	ASSERT_EQ(skel->bss->data_len_match, 1, "data_len_match");
+	ASSERT_EQ(skel->bss->invalid_flags_ret, -EINVAL, "invalid_flags_ret");
+	ASSERT_EQ(skel->bss->copy_ret, 0, "copy_ret");
+	ASSERT_EQ(skel->bss->str_arg0_ret, sizeof(arg0), "str_arg0_ret");
+	ASSERT_EQ(skel->bss->str_arg1_ret, sizeof(arg1), "str_arg1_ret");
+	ASSERT_EQ(skel->bss->str_env0_ret, sizeof(env0), "str_env0_ret");
+	ASSERT_EQ(skel->bss->str_env1_ret, sizeof(env1), "str_env1_ret");
+	ASSERT_EQ(skel->bss->data_match, 1, "data_match");
+	ASSERT_EQ(skel->bss->str_args_match, 1, "str_args_match");
+	ASSERT_EQ(skel->bss->str_envs_match, 1, "str_envs_match");
+
+out:
+	copy_from_user_bprm__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/copy_from_user_bprm.c b/tools/testing/selftests/bpf/progs/copy_from_user_bprm.c
new file mode 100644
index 000000000000..4d7e9002b45b
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/copy_from_user_bprm.c
@@ -0,0 +1,115 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include "vmlinux.h"
+
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include <errno.h>
+#include "bpf_misc.h"
+
+char _license[] SEC("license") = "GPL";
+
+static const char expected_data[] = "first\0second-argument\0"
+				    "SOME_ENV=a\0OTHER_ENV=something";
+static const char expected_arg0[] = "first";
+static const char expected_arg1[] = "second-argument";
+static const char expected_env0[] = "SOME_ENV=a";
+static const char expected_env1[] = "OTHER_ENV=something";
+
+int monitored_pid;
+int bprm_argc;
+int bprm_envc;
+int data_len_match;
+int invalid_flags_ret;
+int copy_ret;
+int str_arg0_ret;
+int str_arg1_ret;
+int str_env0_ret;
+int str_env1_ret;
+int data_match;
+int str_args_match;
+int str_envs_match;
+
+extern int bpf_copy_from_user_bprm(void *dst, u32 dst__sz,
+				   const void *unsafe_ptr__ign,
+				   const struct linux_binprm *bprm,
+				   u64 flags) __ksym;
+
+extern int bpf_copy_from_user_bprm_str(void *dst, u32 dst__sz,
+				       const void *unsafe_ptr__ign,
+				       const struct linux_binprm *bprm,
+				       u64 flags) __ksym;
+
+SEC("lsm.s/bprm_check_security")
+int BPF_PROG(check_exec_args, struct linux_binprm *bprm)
+{
+	u32 pid = bpf_get_current_pid_tgid() >> 32;
+	char data[sizeof(expected_data)];
+	char arg0[32];
+	char arg1[32];
+	char env0[32];
+	char env1[32];
+	u64 offset = 0;
+	u64 data_len;
+
+	if (pid != monitored_pid)
+		return 0;
+
+	bprm_argc = bprm->argc;
+	bprm_envc = bprm->envc;
+
+	/* this is the total size of args and envs starting from bprm->p */
+	data_len = bprm->exec - bprm->p;
+	data_len_match = data_len == sizeof(expected_data);
+
+	invalid_flags_ret = bpf_copy_from_user_bprm(data,
+						    sizeof(data), (void *)bprm->p, bprm, ~0ULL);
+
+	copy_ret = bpf_copy_from_user_bprm(data, sizeof(data), (void *)bprm->p,
+					   bprm, 0);
+	if (copy_ret)
+		return 0;
+
+	data_match =
+		!__builtin_memcmp(data, expected_data, sizeof(expected_data));
+
+	/* arg0 is at bprm->p */
+	str_arg0_ret = bpf_copy_from_user_bprm_str(arg0, sizeof(arg0),
+						   (void *)(bprm->p + offset),
+						   bprm, BPF_F_PAD_ZEROS);
+	if (str_arg0_ret != sizeof(expected_arg0))
+		return 0;
+	offset += str_arg0_ret;
+
+	/* arg1 is at bprm->p + sizeof(arg0) */
+	str_arg1_ret = bpf_copy_from_user_bprm_str(arg1, sizeof(arg1),
+						   (void *)(bprm->p + offset),
+						   bprm, BPF_F_PAD_ZEROS);
+	if (str_arg1_ret != sizeof(expected_arg1))
+		return 0;
+	offset += str_arg1_ret;
+
+	/* env0 is at bprm->p + sizeof(arg0) + sizeof(arg1) */
+	str_env0_ret = bpf_copy_from_user_bprm_str(env0, sizeof(env0),
+						   (void *)(bprm->p + offset),
+						   bprm, BPF_F_PAD_ZEROS);
+	if (str_env0_ret != sizeof(expected_env0))
+		return 0;
+	offset += str_env0_ret;
+
+	/* env1 is at bprm->p + sizeof(arg0) + sizeof(arg1) + sizeof(env0) */
+	str_env1_ret = bpf_copy_from_user_bprm_str(env1, sizeof(env1),
+						   (void *)(bprm->p + offset),
+						   bprm, BPF_F_PAD_ZEROS);
+	if (str_env1_ret != sizeof(expected_env1))
+		return 0;
+
+	str_args_match =
+		!__builtin_memcmp(arg0, expected_arg0, sizeof(expected_arg0)) &&
+		!__builtin_memcmp(arg1, expected_arg1, sizeof(expected_arg1));
+	str_envs_match =
+		!__builtin_memcmp(env0, expected_env0, sizeof(expected_env0)) &&
+		!__builtin_memcmp(env1, expected_env1, sizeof(expected_env1));
+
+	return data_match && str_args_match && str_envs_match ? -EPERM : 0;
+}
-- 
2.55.0

Re: [PATCH bpf-next v2 3/3] selftests/bpf: Test linux_binprm user memory kfuncs

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 8272 bytes --]

> selftests/bpf: Test linux_binprm user memory kfuncs
>
> This patch adds a sleepable BPF LSM program attached to
> bprm_check_security to test bpf_copy_from_user_bprm() and
> bpf_copy_from_user_bprm_str().

This isn't a bug, but could the changelogs in this series use the
imperative mood the kernel asks for, e.g. "Add a sleepable BPF LSM
program attached to bprm_check_security ..." instead of "This patch
adds ..."?

> diff --git a/tools/testing/selftests/bpf/prog_tests/copy_from_user_bprm.c b/tools/testing/selftests/bpf/prog_tests/copy_from_user_bprm.c
> new file mode 100644
> index 000000000000..1007e3aec054
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/copy_from_user_bprm.c
> @@ -0,0 +1,62 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <errno.h>
> +#include <sys/wait.h>
> +#include <unistd.h>
> +
> +#include <test_progs.h>
> +
> +#include "copy_from_user_bprm.skel.h"
> +
> +void test_copy_from_user_bprm(void)
> +{
> +	char arg0[] = "first";
> +	char arg1[] = "second-argument";
> +	char env0[] = "SOME_ENV=a";
> +	char env1[] = "OTHER_ENV=something";
> +	struct copy_from_user_bprm *skel;
> +	pid_t child;
> +	int status;
> +
> +	skel = copy_from_user_bprm__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "open_and_load"))
> +		return;
> +
> +	if (!ASSERT_OK(copy_from_user_bprm__attach(skel), "attach"))
> +		goto out;
> +
> +	child = fork();
> +	if (!ASSERT_GE(child, 0, "fork"))
> +		goto out;
> +
> +	if (!child) {
> +		char *const argv[] = { arg0, arg1, NULL };
> +		char *const envp[] = { env0, env1, NULL };
> +
> +		skel->bss->monitored_pid = getpid();
> +		execve("/bin/true", argv, envp);
> +		_exit(errno);
> +	}
> +
> +	if (!ASSERT_EQ(waitpid(child, &status, 0), child, "waitpid"))
> +		goto out;
> +
> +	if (ASSERT_TRUE(WIFEXITED(status), "child_exited"))
> +		ASSERT_EQ(WEXITSTATUS(status), EPERM, "exec_errno");
> +
> +	ASSERT_EQ(skel->bss->bprm_argc, 2, "bprm_argc");
> +	ASSERT_EQ(skel->bss->bprm_envc, 2, "bprm_envc");
> +	ASSERT_EQ(skel->bss->data_len_match, 1, "data_len_match");
> +	ASSERT_EQ(skel->bss->invalid_flags_ret, -EINVAL, "invalid_flags_ret");
> +	ASSERT_EQ(skel->bss->copy_ret, 0, "copy_ret");
> +	ASSERT_EQ(skel->bss->str_arg0_ret, sizeof(arg0), "str_arg0_ret");
> +	ASSERT_EQ(skel->bss->str_arg1_ret, sizeof(arg1), "str_arg1_ret");
> +	ASSERT_EQ(skel->bss->str_env0_ret, sizeof(env0), "str_env0_ret");
> +	ASSERT_EQ(skel->bss->str_env1_ret, sizeof(env1), "str_env1_ret");

A subsystem pattern flags this as potentially concerning: the test has
no skip path for the configuration in which the kfuncs it exercises are
deliberately unavailable.

The immediately preceding commit in this series (6073b8df0008, "bpf: Add
user memory access kfuncs for linux_binprm") registers both kfuncs on
NOMMU as well, where they are documented and implemented to return
-EOPNOTSUPP:

fs/bpf_fs_kfuncs.c, bpf_copy_from_user_bprm():
  #else
      memset(dst, 0, dst__sz);
      return -EOPNOTSUPP;
  #endif

fs/bpf_fs_kfuncs.c, bpf_copy_from_user_bprm_str():
  #else
      if (flags & BPF_F_PAD_ZEROS)
          memset(dst, 0, dst__sz);
      else
          *(char *)dst = '\0';
      return -EOPNOTSUPP;
  #endif

The changelog of that commit states this explicitly: "On NOMMU systems,
exec arguments are staged in bprm->page[] rather than mapped in bprm->mm,
so the kfuncs return -EOPNOTSUPP."

The BPF program records those return values verbatim into copy_ret,
str_arg0_ret, str_arg1_ret, str_env0_ret, and str_env1_ret
(progs/copy_from_user_bprm.c), and the userspace half then asserts the
success values unconditionally, plus data_len_match, data_match,
str_args_match, and str_envs_match. On a !CONFIG_MMU kernel every one of
those assertions fails and the test is reported as a regression rather
than skipped. Note that invalid_flags_ret (the assertion above) is the
one assertion that would still pass, because the flags check precedes the
#ifdef CONFIG_MMU block.

Would it make sense to use test__skip() here with a reason string when
the kfuncs return -EOPNOTSUPP? test_progs provides test__skip() for
exactly this (123 existing call sites under
tools/testing/selftests/bpf/prog_tests/).

> +	ASSERT_EQ(skel->bss->data_match, 1, "data_match");
> +	ASSERT_EQ(skel->bss->str_args_match, 1, "str_args_match");
> +	ASSERT_EQ(skel->bss->str_envs_match, 1, "str_envs_match");
> +
> +out:
> +	copy_from_user_bprm__destroy(skel);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/copy_from_user_bprm.c b/tools/testing/selftests/bpf/progs/copy_from_user_bprm.c
> new file mode 100644
> index 000000000000..4d7e9002b45b
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/copy_from_user_bprm.c
> @@ -0,0 +1,115 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include "vmlinux.h"
> +
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +#include <errno.h>
> +#include "bpf_misc.h"

This isn't a bug, but is "bpf_misc.h" needed here? I could not spot
anything in the program that comes from it, so the include may be
droppable.

> +
> +char _license[] SEC("license") = "GPL";
> +
> +static const char expected_data[] = "first\0second-argument\0"
> +				    "SOME_ENV=a\0OTHER_ENV=something";

[ ... ]

> +SEC("lsm.s/bprm_check_security")
> +int BPF_PROG(check_exec_args, struct linux_binprm *bprm)
> +{
> +	u32 pid = bpf_get_current_pid_tgid() >> 32;
> +	char data[sizeof(expected_data)];
> +	char arg0[32];
> +	char arg1[32];
> +	char env0[32];
> +	char env1[32];
> +	u64 offset = 0;
> +	u64 data_len;
> +
> +	if (pid != monitored_pid)
> +		return 0;
> +
> +	bprm_argc = bprm->argc;
> +	bprm_envc = bprm->envc;
> +
> +	/* this is the total size of args and envs starting from bprm->p */
> +	data_len = bprm->exec - bprm->p;
> +	data_len_match = data_len == sizeof(expected_data);
> +
> +	invalid_flags_ret = bpf_copy_from_user_bprm(data,
> +						    sizeof(data), (void *)bprm->p, bprm, ~0ULL);
> +
> +	copy_ret = bpf_copy_from_user_bprm(data, sizeof(data), (void *)bprm->p,
> +					   bprm, 0);
> +	if (copy_ret)
> +		return 0;
> +
> +	data_match =
> +		!__builtin_memcmp(data, expected_data, sizeof(expected_data));
> +
> +	/* arg0 is at bprm->p */
> +	str_arg0_ret = bpf_copy_from_user_bprm_str(arg0, sizeof(arg0),
> +						   (void *)(bprm->p + offset),
> +						   bprm, BPF_F_PAD_ZEROS);
> +	if (str_arg0_ret != sizeof(expected_arg0))
> +		return 0;
> +	offset += str_arg0_ret;
> +
> +	/* arg1 is at bprm->p + sizeof(arg0) */
> +	str_arg1_ret = bpf_copy_from_user_bprm_str(arg1, sizeof(arg1),
> +						   (void *)(bprm->p + offset),
> +						   bprm, BPF_F_PAD_ZEROS);

This isn't a bug, but would these comments read more precisely as
something like "arg1 follows arg0's NUL terminator", or by referring to
the expected_* strings? In this scope arg0, arg1, env0 are the 32-byte
destination buffers, so sizeof(arg0) here is the 32-byte destination
buffer rather than the string length. The code actually advances offset
by the value the kfunc returned (6, 22, 33).

> +	if (str_arg1_ret != sizeof(expected_arg1))
> +		return 0;
> +	offset += str_arg1_ret;
> +
> +	/* env0 is at bprm->p + sizeof(arg0) + sizeof(arg1) */
> +	str_env0_ret = bpf_copy_from_user_bprm_str(env0, sizeof(env0),
> +						   (void *)(bprm->p + offset),
> +						   bprm, BPF_F_PAD_ZEROS);
> +	if (str_env0_ret != sizeof(expected_env0))
> +		return 0;
> +	offset += str_env0_ret;
> +
> +	/* env1 is at bprm->p + sizeof(arg0) + sizeof(arg1) + sizeof(env0) */
> +	str_env1_ret = bpf_copy_from_user_bprm_str(env1, sizeof(env1),
> +						   (void *)(bprm->p + offset),
> +						   bprm, BPF_F_PAD_ZEROS);
> +	if (str_env1_ret != sizeof(expected_env1))
> +		return 0;
> +
> +	str_args_match =
> +		!__builtin_memcmp(arg0, expected_arg0, sizeof(expected_arg0)) &&
> +		!__builtin_memcmp(arg1, expected_arg1, sizeof(expected_arg1));
> +	str_envs_match =
> +		!__builtin_memcmp(env0, expected_env0, sizeof(expected_env0)) &&
> +		!__builtin_memcmp(env1, expected_env1, sizeof(expected_env1));
> +
> +	return data_match && str_args_match && str_envs_match ? -EPERM : 0;
> +}


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32375104040