* [bpf-next v6 1/3] mm: add copy_remote_vm_str
@ 2025-01-28 22:43 Jordan Rome
2025-01-28 22:43 ` [bpf-next v6 2/3] bpf: Add bpf_copy_from_user_task_str kfunc Jordan Rome
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Jordan Rome @ 2025-01-28 22:43 UTC (permalink / raw)
To: bpf
Cc: linux-mm, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Kernel Team, Andrew Morton, Shakeel Butt, Alexander Potapenko
Similar to `access_process_vm` but specific to strings.
Also chunks reads by page and utilizes `strscpy`
for handling null termination.
Signed-off-by: Jordan Rome <linux@jordanrome.com>
---
include/linux/mm.h | 3 ++
mm/memory.c | 119 +++++++++++++++++++++++++++++++++++++++++++++
mm/nommu.c | 74 ++++++++++++++++++++++++++++
3 files changed, 196 insertions(+)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index f02925447e59..f3a05b3eb2f2 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2485,6 +2485,9 @@ extern int access_process_vm(struct task_struct *tsk, unsigned long addr,
extern int access_remote_vm(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);
+
long get_user_pages_remote(struct mm_struct *mm,
unsigned long start, unsigned long nr_pages,
unsigned int gup_flags, struct page **pages,
diff --git a/mm/memory.c b/mm/memory.c
index 398c031be9ba..7f6e74a99984 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -6714,6 +6714,125 @@ int access_process_vm(struct task_struct *tsk, unsigned long addr,
}
EXPORT_SYMBOL_GPL(access_process_vm);
+/*
+ * Copy a string from another process's address space as given in mm.
+ * If there is any error return -EFAULT.
+ */
+static int __copy_remote_vm_str(struct mm_struct *mm, unsigned long addr,
+ void *buf, int len, unsigned int gup_flags)
+{
+ void *old_buf = buf;
+ int err = 0;
+ ((char *)buf)[0] = '\0';
+
+ if (mmap_read_lock_killable(mm))
+ return -EFAULT;
+
+ /* Untag the address before looking up the VMA */
+ addr = untagged_addr_remote(mm, addr);
+
+ /* Avoid triggering the temporary warning in __get_user_pages */
+ if (!vma_lookup(mm, addr)) {
+ err = -EFAULT;
+ goto out;
+ }
+
+ while (len) {
+ int bytes, offset, retval;
+ void *maddr;
+ struct page *page;
+ struct vm_area_struct *vma = NULL;
+
+ page = get_user_page_vma_remote(mm, addr, gup_flags, &vma);
+
+ if (IS_ERR(page)) {
+ /*
+ * Treat as a total failure for now until we decide how
+ * to handle the CONFIG_HAVE_IOREMAP_PROT case and
+ * stack expansion.
+ */
+ ((char *)buf)[0] = '\0';
+ err = -EFAULT;
+ goto out;
+ }
+
+ bytes = len;
+ offset = addr & (PAGE_SIZE - 1);
+ if (bytes > PAGE_SIZE - offset)
+ bytes = PAGE_SIZE - offset;
+
+ maddr = kmap_local_page(page);
+ retval = strscpy(buf, maddr + offset, bytes);
+
+ if (retval < 0) {
+ buf += (bytes - 1);
+ /*
+ * Because strscpy always NUL terminates we need to
+ * copy the last byte in the page if we are going to
+ * load more pages
+ */
+ if (bytes != len) {
+ addr += (bytes - 1);
+ copy_from_user_page(vma, page, addr, buf,
+ maddr + (PAGE_SIZE - 1), 1);
+
+ buf += 1;
+ addr += 1;
+ }
+ len -= bytes;
+ }
+
+ unmap_and_put_page(page, maddr);
+
+ if (retval >= 0) {
+ /* Found the end of the string */
+ buf += retval;
+ goto out;
+ }
+ }
+
+out:
+ mmap_read_unlock(mm);
+ if (err)
+ return err;
+
+ return buf - old_buf;
+}
+
+/**
+ * copy_remote_vm_str - copy a string from another process's address space.
+ * @tsk: the task of the target 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. Always guaranteed to leave NUL-terminated
+ * buffer. On any error, return -EFAULT.
+ */
+int copy_remote_vm_str(struct task_struct *tsk, unsigned long addr,
+ void *buf, int len, unsigned int gup_flags)
+{
+ struct mm_struct *mm;
+ int ret;
+
+ mm = get_task_mm(tsk);
+ if (!mm) {
+ ((char *)buf)[0] = '\0';
+ return -EFAULT;
+ }
+
+ ret = __copy_remote_vm_str(mm, addr, buf, len, gup_flags);
+
+ mmput(mm);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(copy_remote_vm_str);
+
/*
* Print the name of a VMA.
*/
diff --git a/mm/nommu.c b/mm/nommu.c
index 9cb6e99215e2..4d83d0813eb8 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -1701,6 +1701,80 @@ int access_process_vm(struct task_struct *tsk, unsigned long addr, void *buf, in
}
EXPORT_SYMBOL_GPL(access_process_vm);
+/*
+ * Copy a string from another process's address space as given in mm.
+ * If there is any error return -EFAULT.
+ */
+static int __copy_remote_vm_str(struct mm_struct *mm, unsigned long addr,
+ void *buf, int len)
+{
+ uint64_t tmp;
+ struct vm_area_struct *vma;
+
+ int ret = -EFAULT;
+
+ ((char *)buf)[0] = '\0';
+
+ if (mmap_read_lock_killable(mm))
+ return ret;
+
+ /* the access must start within one of the target process's mappings */
+ vma = find_vma(mm, addr);
+ if (!vma)
+ goto out;
+
+ if (check_add_overflow(addr, len, &tmp))
+ goto out;
+ /* don't overrun this mapping */
+ if (tmp >= vma->vm_end)
+ len = vma->vm_end - addr;
+
+ /* only read mappings where it is permitted */
+ if (vma->vm_flags & VM_MAYREAD) {
+ ret = strscpy(buf, (char *)addr, len);
+ if (ret < 0)
+ ret = len - 1;
+ }
+
+out:
+ mmap_read_unlock(mm);
+ return ret;
+}
+
+/**
+ * copy_remote_vm_str - copy a string from another process's address space.
+ * @tsk: the task of the target 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. Always guaranteed to leave NUL-terminated
+ * buffer. On any error, return -EFAULT.
+ */
+int copy_remote_vm_str(struct task_struct *tsk, unsigned long addr,
+ void *buf, int len, unsigned int gup_flags)
+{
+ struct mm_struct *mm;
+ int ret;
+
+ mm = get_task_mm(tsk);
+ if (!mm) {
+ ((char *)buf)[0] = '\0';
+ return -EFAULT;
+ }
+
+ ret = __copy_remote_vm_str(mm, addr, buf, len);
+
+ mmput(mm);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(copy_remote_vm_str);
+
/**
* nommu_shrink_inode_mappings - Shrink the shared mappings on an inode
* @inode: The inode to check
--
2.43.5
^ permalink raw reply [flat|nested] 4+ messages in thread
* [bpf-next v6 2/3] bpf: Add bpf_copy_from_user_task_str kfunc
2025-01-28 22:43 [bpf-next v6 1/3] mm: add copy_remote_vm_str Jordan Rome
@ 2025-01-28 22:43 ` Jordan Rome
2025-01-28 22:43 ` [bpf-next v6 3/3] selftests/bpf: Add tests for bpf_copy_from_user_task_str Jordan Rome
2025-01-30 0:19 ` [bpf-next v6 1/3] mm: add copy_remote_vm_str Andrii Nakryiko
2 siblings, 0 replies; 4+ messages in thread
From: Jordan Rome @ 2025-01-28 22:43 UTC (permalink / raw)
To: bpf
Cc: linux-mm, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Kernel Team, Andrew Morton, Shakeel Butt, Alexander Potapenko
This new kfunc will be able to copy a string
from another process's/task's address space.
This is similar to `bpf_copy_from_user_str`
but accepts a `struct task_struct*` argument.
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Jordan Rome <linux@jordanrome.com>
---
kernel/bpf/helpers.c | 48 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 48 insertions(+)
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index f27ce162427a..a33f72a4c31f 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -3082,6 +3082,53 @@ __bpf_kfunc void bpf_local_irq_restore(unsigned long *flags__irq_flag)
local_irq_restore(*flags__irq_flag);
}
+/**
+ * bpf_copy_from_user_task_str() - Copy a string from an task's address space
+ * @dst: Destination address, in kernel space. This buffer must be
+ * at least @dst__sz bytes long.
+ * @dst__sz: Maximum number of bytes to copy, includes the trailing NUL.
+ * @unsafe_ptr__ign: Source address in the task's address space.
+ * @tsk: The task whose address space will be used
+ * @flags: The only supported flag is BPF_F_PAD_ZEROS
+ *
+ * Copies a NUL terminated string from a task's address space to @dst__sz
+ * buffer. If user string is too long this will still ensure zero termination
+ * in the @dst__sz buffer unless buffer size is 0.
+ *
+ * If BPF_F_PAD_ZEROS flag is set, memset the tail of @dst__sz to 0 on success
+ * and memset all of @dst__sz on failure.
+ *
+ * Return: The number of copied bytes on success including the NUL terminator.
+ * A negative error code on failure.
+ */
+__bpf_kfunc int bpf_copy_from_user_task_str(void *dst,
+ u32 dst__sz,
+ const void __user *unsafe_ptr__ign,
+ struct task_struct *tsk,
+ u64 flags)
+{
+ int ret;
+
+ if (unlikely(flags & ~BPF_F_PAD_ZEROS))
+ return -EINVAL;
+
+ if (unlikely(!dst__sz))
+ return 0;
+
+ ret = copy_remote_vm_str(tsk, (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;
+}
+
__bpf_kfunc_end_defs();
BTF_KFUNCS_START(generic_btf_ids)
@@ -3174,6 +3221,7 @@ BTF_ID_FLAGS(func, bpf_iter_bits_new, KF_ITER_NEW)
BTF_ID_FLAGS(func, bpf_iter_bits_next, KF_ITER_NEXT | KF_RET_NULL)
BTF_ID_FLAGS(func, bpf_iter_bits_destroy, KF_ITER_DESTROY)
BTF_ID_FLAGS(func, bpf_copy_from_user_str, KF_SLEEPABLE)
+BTF_ID_FLAGS(func, bpf_copy_from_user_task_str, KF_SLEEPABLE)
BTF_ID_FLAGS(func, bpf_get_kmem_cache)
BTF_ID_FLAGS(func, bpf_iter_kmem_cache_new, KF_ITER_NEW | KF_SLEEPABLE)
BTF_ID_FLAGS(func, bpf_iter_kmem_cache_next, KF_ITER_NEXT | KF_RET_NULL | KF_SLEEPABLE)
--
2.43.5
^ permalink raw reply [flat|nested] 4+ messages in thread
* [bpf-next v6 3/3] selftests/bpf: Add tests for bpf_copy_from_user_task_str
2025-01-28 22:43 [bpf-next v6 1/3] mm: add copy_remote_vm_str Jordan Rome
2025-01-28 22:43 ` [bpf-next v6 2/3] bpf: Add bpf_copy_from_user_task_str kfunc Jordan Rome
@ 2025-01-28 22:43 ` Jordan Rome
2025-01-30 0:19 ` [bpf-next v6 1/3] mm: add copy_remote_vm_str Andrii Nakryiko
2 siblings, 0 replies; 4+ messages in thread
From: Jordan Rome @ 2025-01-28 22:43 UTC (permalink / raw)
To: bpf
Cc: linux-mm, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Kernel Team, Andrew Morton, Shakeel Butt, Alexander Potapenko
This adds tests for both the happy path and the
error path (with and without the BPF_F_PAD_ZEROS flag).
Signed-off-by: Jordan Rome <linux@jordanrome.com>
---
.../selftests/bpf/prog_tests/bpf_iter.c | 68 +++++++++++
.../selftests/bpf/prog_tests/read_vsyscall.c | 1 +
.../selftests/bpf/progs/bpf_iter_tasks.c | 110 ++++++++++++++++++
.../selftests/bpf/progs/read_vsyscall.c | 11 +-
4 files changed, 188 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
index 6f1bfacd7375..add4a18c33bd 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
@@ -323,19 +323,87 @@ static void test_task_pidfd(void)
static void test_task_sleepable(void)
{
struct bpf_iter_tasks *skel;
+ int pid, status, err, data_pipe[2], finish_pipe[2], c;
+ char *test_data = NULL;
+ char *test_data_long = NULL;
+ char *data[2];
+
+ if (!ASSERT_OK(pipe(data_pipe), "data_pipe") ||
+ !ASSERT_OK(pipe(finish_pipe), "finish_pipe"))
+ return;
skel = bpf_iter_tasks__open_and_load();
if (!ASSERT_OK_PTR(skel, "bpf_iter_tasks__open_and_load"))
return;
+ pid = fork();
+ if (!ASSERT_GE(pid, 0, "fork"))
+ return;
+
+ if (pid == 0) {
+ /* child */
+ close(data_pipe[0]);
+ close(finish_pipe[1]);
+
+ test_data = malloc(sizeof(char) * 10);
+ strncpy(test_data, "test_data", 10);
+ test_data[9] = '\0';
+
+ test_data_long = malloc(sizeof(char) * 5000);
+ for (int i = 0; i < 5000; ++i) {
+ if (i % 2 == 0)
+ test_data_long[i] = 'b';
+ else
+ test_data_long[i] = 'a';
+ }
+ test_data_long[4999] = '\0';
+
+ data[0] = test_data;
+ data[1] = test_data_long;
+
+ write(data_pipe[1], &data, sizeof(data));
+
+ /* keep child alive until after the test */
+ err = read(finish_pipe[0], &c, 1);
+ if (err != 1)
+ exit(-1);
+
+ close(data_pipe[1]);
+ close(finish_pipe[0]);
+ _exit(0);
+ }
+
+ /* parent */
+ close(data_pipe[1]);
+ close(finish_pipe[0]);
+
+ err = read(data_pipe[0], &data, sizeof(data));
+ ASSERT_EQ(err, sizeof(data), "read_check");
+
+ skel->bss->user_ptr = data[0];
+ skel->bss->user_ptr_long = data[1];
+ skel->bss->pid = pid;
+
do_dummy_read(skel->progs.dump_task_sleepable);
ASSERT_GT(skel->bss->num_expected_failure_copy_from_user_task, 0,
"num_expected_failure_copy_from_user_task");
ASSERT_GT(skel->bss->num_success_copy_from_user_task, 0,
"num_success_copy_from_user_task");
+ ASSERT_GT(skel->bss->num_expected_failure_copy_from_user_task_str, 0,
+ "num_expected_failure_copy_from_user_task_str");
+ ASSERT_GT(skel->bss->num_success_copy_from_user_task_str, 0,
+ "num_success_copy_from_user_task_str");
bpf_iter_tasks__destroy(skel);
+
+ write(finish_pipe[1], &c, 1);
+ err = waitpid(pid, &status, 0);
+ ASSERT_EQ(err, pid, "waitpid");
+ ASSERT_EQ(status, 0, "zero_child_exit");
+
+ close(data_pipe[0]);
+ close(finish_pipe[1]);
}
static void test_task_stack(void)
diff --git a/tools/testing/selftests/bpf/prog_tests/read_vsyscall.c b/tools/testing/selftests/bpf/prog_tests/read_vsyscall.c
index c7b9ba8b1d06..a8d1eaa67020 100644
--- a/tools/testing/selftests/bpf/prog_tests/read_vsyscall.c
+++ b/tools/testing/selftests/bpf/prog_tests/read_vsyscall.c
@@ -24,6 +24,7 @@ struct read_ret_desc {
{ .name = "copy_from_user", .ret = -EFAULT },
{ .name = "copy_from_user_task", .ret = -EFAULT },
{ .name = "copy_from_user_str", .ret = -EFAULT },
+ { .name = "copy_from_user_task_str", .ret = -EFAULT },
};
void test_read_vsyscall(void)
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_tasks.c b/tools/testing/selftests/bpf/progs/bpf_iter_tasks.c
index bc10c4e4b4fa..966ee5a7b066 100644
--- a/tools/testing/selftests/bpf/progs/bpf_iter_tasks.c
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_tasks.c
@@ -9,6 +9,13 @@ char _license[] SEC("license") = "GPL";
uint32_t tid = 0;
int num_unknown_tid = 0;
int num_known_tid = 0;
+void *user_ptr = 0;
+void *user_ptr_long = 0;
+uint32_t pid = 0;
+
+static char big_str1[5000];
+static char big_str2[5005];
+static char big_str3[4996];
SEC("iter/task")
int dump_task(struct bpf_iter__task *ctx)
@@ -35,7 +42,9 @@ int dump_task(struct bpf_iter__task *ctx)
}
int num_expected_failure_copy_from_user_task = 0;
+int num_expected_failure_copy_from_user_task_str = 0;
int num_success_copy_from_user_task = 0;
+int num_success_copy_from_user_task_str = 0;
SEC("iter.s/task")
int dump_task_sleepable(struct bpf_iter__task *ctx)
@@ -44,6 +53,9 @@ int dump_task_sleepable(struct bpf_iter__task *ctx)
struct task_struct *task = ctx->task;
static const char info[] = " === END ===";
struct pt_regs *regs;
+ char task_str1[10] = "aaaaaaaaaa";
+ char task_str2[10], task_str3[10];
+ char task_str4[20] = "aaaaaaaaaaaaaaaaaaaa";
void *ptr;
uint32_t user_data = 0;
int ret;
@@ -78,8 +90,106 @@ int dump_task_sleepable(struct bpf_iter__task *ctx)
BPF_SEQ_PRINTF(seq, "%s\n", info);
return 0;
}
+
++num_success_copy_from_user_task;
+ /* Read an invalid pointer and ensure we get an error */
+ ptr = NULL;
+ ret = bpf_copy_from_user_task_str((char *)task_str1, sizeof(task_str1), ptr, task, 0);
+ if (ret >= 0 || task_str1[9] != 'a' || task_str1[0] != '\0') {
+ BPF_SEQ_PRINTF(seq, "%s\n", info);
+ return 0;
+ }
+
+ /* Read an invalid pointer and ensure we get error with pad zeros flag */
+ ptr = NULL;
+ ret = bpf_copy_from_user_task_str((char *)task_str1, sizeof(task_str1),
+ ptr, task, BPF_F_PAD_ZEROS);
+ if (ret >= 0 || task_str1[9] != '\0' || task_str1[0] != '\0') {
+ BPF_SEQ_PRINTF(seq, "%s\n", info);
+ return 0;
+ }
+
+ ++num_expected_failure_copy_from_user_task_str;
+
+ /* Same length as the string */
+ ret = bpf_copy_from_user_task_str((char *)task_str2, 10, user_ptr, task, 0);
+ /* only need to do the task pid check once */
+ if (bpf_strncmp(task_str2, 10, "test_data\0") != 0 || ret != 10 || task->tgid != pid) {
+ BPF_SEQ_PRINTF(seq, "%s\n", info);
+ return 0;
+ }
+
+ /* Shorter length than the string */
+ ret = bpf_copy_from_user_task_str((char *)task_str3, 2, user_ptr, task, 0);
+ if (bpf_strncmp(task_str3, 2, "t\0") != 0 || ret != 2) {
+ BPF_SEQ_PRINTF(seq, "%s\n", info);
+ return 0;
+ }
+
+ /* Longer length than the string */
+ ret = bpf_copy_from_user_task_str((char *)task_str4, 20, user_ptr, task, 0);
+ if (bpf_strncmp(task_str4, 10, "test_data\0") != 0 || ret != 10
+ || task_str4[sizeof(task_str4) - 1] != 'a') {
+ BPF_SEQ_PRINTF(seq, "%s\n", info);
+ return 0;
+ }
+
+ /* Longer length than the string with pad zeros flag */
+ ret = bpf_copy_from_user_task_str((char *)task_str4, 20, user_ptr, task, BPF_F_PAD_ZEROS);
+ if (bpf_strncmp(task_str4, 10, "test_data\0") != 0 || ret != 10
+ || task_str4[sizeof(task_str4) - 1] != '\0') {
+ BPF_SEQ_PRINTF(seq, "%s\n", info);
+ return 0;
+ }
+
+ /* Longer length than the string past a page boundary */
+ ret = bpf_copy_from_user_task_str(big_str1, 5000, user_ptr, task, 0);
+ if (bpf_strncmp(big_str1, 10, "test_data\0") != 0 || ret != 10) {
+ BPF_SEQ_PRINTF(seq, "%s\n", info);
+ return 0;
+ }
+
+ /* String that crosses a page boundary */
+ ret = bpf_copy_from_user_task_str(big_str1, 5000, user_ptr_long, task, BPF_F_PAD_ZEROS);
+ if (bpf_strncmp(big_str1, 4, "baba") != 0 || ret != 5000
+ || bpf_strncmp(big_str1 + 4996, 4, "bab\0") != 0) {
+ BPF_SEQ_PRINTF(seq, "%s\n", info);
+ return 0;
+ }
+
+ for (int i = 0; i < 4999; ++i) {
+ if (i % 2 == 0) {
+ if (big_str1[i] != 'b') {
+ BPF_SEQ_PRINTF(seq, "%s\n", info);
+ return 0;
+ }
+ } else {
+ if (big_str1[i] != 'a') {
+ BPF_SEQ_PRINTF(seq, "%s\n", info);
+ return 0;
+ }
+ }
+ }
+
+ /* Longer length than the string that crosses a page boundary */
+ ret = bpf_copy_from_user_task_str(big_str2, 5005, user_ptr_long, task, BPF_F_PAD_ZEROS);
+ if (bpf_strncmp(big_str2, 4, "baba") != 0 || ret != 5000
+ || bpf_strncmp(big_str2 + 4996, 5, "bab\0\0") != 0) {
+ BPF_SEQ_PRINTF(seq, "%s\n", info);
+ return 0;
+ }
+
+ /* Shorter length than the string that crosses a page boundary */
+ ret = bpf_copy_from_user_task_str(big_str3, 4996, user_ptr_long, task, 0);
+ if (bpf_strncmp(big_str3, 4, "baba") != 0 || ret != 4996
+ || bpf_strncmp(big_str3 + 4992, 4, "bab\0") != 0) {
+ BPF_SEQ_PRINTF(seq, "%s\n", info);
+ return 0;
+ }
+
+ ++num_success_copy_from_user_task_str;
+
if (ctx->meta->seq_num == 0)
BPF_SEQ_PRINTF(seq, " tgid gid data\n");
diff --git a/tools/testing/selftests/bpf/progs/read_vsyscall.c b/tools/testing/selftests/bpf/progs/read_vsyscall.c
index 39ebef430059..395591374d4f 100644
--- a/tools/testing/selftests/bpf/progs/read_vsyscall.c
+++ b/tools/testing/selftests/bpf/progs/read_vsyscall.c
@@ -8,14 +8,16 @@
int target_pid = 0;
void *user_ptr = 0;
-int read_ret[9];
+int read_ret[10];
char _license[] SEC("license") = "GPL";
/*
- * This is the only kfunc, the others are helpers
+ * These are the kfuncs, the others are helpers
*/
int bpf_copy_from_user_str(void *dst, u32, const void *, u64) __weak __ksym;
+int bpf_copy_from_user_task_str(void *dst, u32, const void *,
+ struct task_struct *, u64) __weak __ksym;
SEC("fentry/" SYS_PREFIX "sys_nanosleep")
int do_probe_read(void *ctx)
@@ -47,6 +49,11 @@ int do_copy_from_user(void *ctx)
read_ret[7] = bpf_copy_from_user_task(buf, sizeof(buf), user_ptr,
bpf_get_current_task_btf(), 0);
read_ret[8] = bpf_copy_from_user_str((char *)buf, sizeof(buf), user_ptr, 0);
+ read_ret[9] = bpf_copy_from_user_task_str((char *)buf,
+ sizeof(buf),
+ user_ptr,
+ bpf_get_current_task_btf(),
+ 0);
return 0;
}
--
2.43.5
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bpf-next v6 1/3] mm: add copy_remote_vm_str
2025-01-28 22:43 [bpf-next v6 1/3] mm: add copy_remote_vm_str Jordan Rome
2025-01-28 22:43 ` [bpf-next v6 2/3] bpf: Add bpf_copy_from_user_task_str kfunc Jordan Rome
2025-01-28 22:43 ` [bpf-next v6 3/3] selftests/bpf: Add tests for bpf_copy_from_user_task_str Jordan Rome
@ 2025-01-30 0:19 ` Andrii Nakryiko
2 siblings, 0 replies; 4+ messages in thread
From: Andrii Nakryiko @ 2025-01-30 0:19 UTC (permalink / raw)
To: Jordan Rome
Cc: bpf, linux-mm, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Kernel Team, Andrew Morton, Shakeel Butt,
Alexander Potapenko
On Tue, Jan 28, 2025 at 2:44 PM Jordan Rome <linux@jordanrome.com> wrote:
>
> Similar to `access_process_vm` but specific to strings.
> Also chunks reads by page and utilizes `strscpy`
> for handling null termination.
>
> Signed-off-by: Jordan Rome <linux@jordanrome.com>
> ---
> include/linux/mm.h | 3 ++
> mm/memory.c | 119 +++++++++++++++++++++++++++++++++++++++++++++
> mm/nommu.c | 74 ++++++++++++++++++++++++++++
> 3 files changed, 196 insertions(+)
>
The logic looks good, but I have a bunch of stylistic nits below. It
would be nice for someone from mm side to take a look as well.
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index f02925447e59..f3a05b3eb2f2 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2485,6 +2485,9 @@ extern int access_process_vm(struct task_struct *tsk, unsigned long addr,
> extern int access_remote_vm(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);
> +
> long get_user_pages_remote(struct mm_struct *mm,
> unsigned long start, unsigned long nr_pages,
> unsigned int gup_flags, struct page **pages,
> diff --git a/mm/memory.c b/mm/memory.c
> index 398c031be9ba..7f6e74a99984 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -6714,6 +6714,125 @@ int access_process_vm(struct task_struct *tsk, unsigned long addr,
> }
> EXPORT_SYMBOL_GPL(access_process_vm);
>
> +/*
> + * Copy a string from another process's address space as given in mm.
> + * If there is any error return -EFAULT.
> + */
> +static int __copy_remote_vm_str(struct mm_struct *mm, unsigned long addr,
> + void *buf, int len, unsigned int gup_flags)
> +{
> + void *old_buf = buf;
> + int err = 0;
empty line between variables and statements
> + ((char *)buf)[0] = '\0';
nit: this would be probably a bit more "canonical": *(char *)buf = '\0';
> +
> + if (mmap_read_lock_killable(mm))
> + return -EFAULT;
> +
> + /* Untag the address before looking up the VMA */
> + addr = untagged_addr_remote(mm, addr);
> +
> + /* Avoid triggering the temporary warning in __get_user_pages */
> + if (!vma_lookup(mm, addr)) {
> + err = -EFAULT;
> + goto out;
> + }
> +
> + while (len) {
> + int bytes, offset, retval;
> + void *maddr;
> + struct page *page;
> + struct vm_area_struct *vma = NULL;
> +
> + page = get_user_page_vma_remote(mm, addr, gup_flags, &vma);
> +
> + if (IS_ERR(page)) {
> + /*
> + * Treat as a total failure for now until we decide how
> + * to handle the CONFIG_HAVE_IOREMAP_PROT case and
> + * stack expansion.
> + */
> + ((char *)buf)[0] = '\0';
> + err = -EFAULT;
> + goto out;
> + }
> +
> + bytes = len;
> + offset = addr & (PAGE_SIZE - 1);
> + if (bytes > PAGE_SIZE - offset)
> + bytes = PAGE_SIZE - offset;
> +
> + maddr = kmap_local_page(page);
> + retval = strscpy(buf, maddr + offset, bytes);
> +
> + if (retval < 0) {
> + buf += (bytes - 1);
nit: unnecessary ()
another nit: you could have had `addr += bytes - 1;` here, to keep
addr and buf adjustment code close
> + /*
> + * Because strscpy always NUL terminates we need to
> + * copy the last byte in the page if we are going to
> + * load more pages
> + */
> + if (bytes != len) {
> + addr += (bytes - 1);
> + copy_from_user_page(vma, page, addr, buf,
> + maddr + (PAGE_SIZE - 1), 1);
> +
> + buf += 1;
> + addr += 1;
> + }
> + len -= bytes;
> + }
> +
> + unmap_and_put_page(page, maddr);
> +
> + if (retval >= 0) {
> + /* Found the end of the string */
> + buf += retval;
> + goto out;
> + }
it's not incorrect, but it would be nice not to have to re-check
retval twice. Why not this structure:
ret = strscpy(...)
if (retval >= 0) {
unmap_and_put_page(page, maddr);
buf += retval;
break;
}
/* this is -E2BIG case */
buf += bytes - 1;
addr += bytes - 1;
if (bytes != len) { copy, buf += 1, addr += 1 }
unmap_and_put_page(page, maddr);
Note that you don't need goto, break is fine. And yes, I don't think
duplicating unmap_and_put_page() is a problem.
> + }
> +
> +out:
> + mmap_read_unlock(mm);
> + if (err)
> + return err;
> +
> + return buf - old_buf;
> +}
> +
> +/**
> + * copy_remote_vm_str - copy a string from another process's address space.
> + * @tsk: the task of the target 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. Always guaranteed to leave NUL-terminated
> + * buffer. On any error, return -EFAULT.
> + */
> +int copy_remote_vm_str(struct task_struct *tsk, unsigned long addr,
> + void *buf, int len, unsigned int gup_flags)
> +{
> + struct mm_struct *mm;
> + int ret;
> +
> + mm = get_task_mm(tsk);
> + if (!mm) {
> + ((char *)buf)[0] = '\0';
> + return -EFAULT;
> + }
> +
> + ret = __copy_remote_vm_str(mm, addr, buf, len, gup_flags);
> +
> + mmput(mm);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(copy_remote_vm_str);
> +
> /*
> * Print the name of a VMA.
> */
> diff --git a/mm/nommu.c b/mm/nommu.c
> index 9cb6e99215e2..4d83d0813eb8 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -1701,6 +1701,80 @@ int access_process_vm(struct task_struct *tsk, unsigned long addr, void *buf, in
> }
> EXPORT_SYMBOL_GPL(access_process_vm);
>
> +/*
> + * Copy a string from another process's address space as given in mm.
> + * If there is any error return -EFAULT.
> + */
> +static int __copy_remote_vm_str(struct mm_struct *mm, unsigned long addr,
> + void *buf, int len)
> +{
> + uint64_t tmp;
s/uint64_t/unsigned long/
also tmp -> addr_end ?
> + struct vm_area_struct *vma;
> +
nit: no empty line here, why?
> + int ret = -EFAULT;
> +
> + ((char *)buf)[0] = '\0';
> +
> + if (mmap_read_lock_killable(mm))
> + return ret;
> +
> + /* the access must start within one of the target process's mappings */
> + vma = find_vma(mm, addr);
> + if (!vma)
> + goto out;
> +
> + if (check_add_overflow(addr, len, &tmp))
> + goto out;
> + /* don't overrun this mapping */
> + if (tmp >= vma->vm_end)
nit: strictly speaking only `tmp > vma->vm_end` needs special handling
> + len = vma->vm_end - addr;
> +
> + /* only read mappings where it is permitted */
> + if (vma->vm_flags & VM_MAYREAD) {
> + ret = strscpy(buf, (char *)addr, len);
> + if (ret < 0)
> + ret = len - 1;
> + }
> +
> +out:
> + mmap_read_unlock(mm);
> + return ret;
> +}
> +
> +/**
> + * copy_remote_vm_str - copy a string from another process's address space.
> + * @tsk: the task of the target 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. Always guaranteed to leave NUL-terminated
> + * buffer. On any error, return -EFAULT.
> + */
> +int copy_remote_vm_str(struct task_struct *tsk, unsigned long addr,
> + void *buf, int len, unsigned int gup_flags)
> +{
> + struct mm_struct *mm;
> + int ret;
> +
> + mm = get_task_mm(tsk);
> + if (!mm) {
> + ((char *)buf)[0] = '\0';
> + return -EFAULT;
> + }
> +
> + ret = __copy_remote_vm_str(mm, addr, buf, len);
> +
> + mmput(mm);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(copy_remote_vm_str);
> +
> /**
> * nommu_shrink_inode_mappings - Shrink the shared mappings on an inode
> * @inode: The inode to check
> --
> 2.43.5
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-01-30 0:20 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-01-28 22:43 [bpf-next v6 1/3] mm: add copy_remote_vm_str Jordan Rome
2025-01-28 22:43 ` [bpf-next v6 2/3] bpf: Add bpf_copy_from_user_task_str kfunc Jordan Rome
2025-01-28 22:43 ` [bpf-next v6 3/3] selftests/bpf: Add tests for bpf_copy_from_user_task_str Jordan Rome
2025-01-30 0:19 ` [bpf-next v6 1/3] mm: add copy_remote_vm_str Andrii Nakryiko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox