* [bpf-next v5 2/3] bpf: Add bpf_copy_from_user_task_str kfunc
2025-01-26 16:38 [bpf-next v5 1/3] mm: add copy_remote_vm_str Jordan Rome
@ 2025-01-26 16:38 ` Jordan Rome
2025-01-27 23:53 ` Andrii Nakryiko
2025-01-26 16:38 ` [bpf-next v5 3/3] selftests/bpf: Add tests for bpf_copy_from_user_task_str Jordan Rome
2025-01-27 23:49 ` [bpf-next v5 1/3] mm: add copy_remote_vm_str Andrii Nakryiko
2 siblings, 1 reply; 6+ messages in thread
From: Jordan Rome @ 2025-01-26 16:38 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.
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] 6+ messages in thread* [bpf-next v5 3/3] selftests/bpf: Add tests for bpf_copy_from_user_task_str
2025-01-26 16:38 [bpf-next v5 1/3] mm: add copy_remote_vm_str Jordan Rome
2025-01-26 16:38 ` [bpf-next v5 2/3] bpf: Add bpf_copy_from_user_task_str kfunc Jordan Rome
@ 2025-01-26 16:38 ` Jordan Rome
2025-01-27 23:49 ` [bpf-next v5 1/3] mm: add copy_remote_vm_str Andrii Nakryiko
2 siblings, 0 replies; 6+ messages in thread
From: Jordan Rome @ 2025-01-26 16:38 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..d28d642a64c3 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') {
+ 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') {
+ 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] 6+ messages in thread* Re: [bpf-next v5 1/3] mm: add copy_remote_vm_str
2025-01-26 16:38 [bpf-next v5 1/3] mm: add copy_remote_vm_str Jordan Rome
2025-01-26 16:38 ` [bpf-next v5 2/3] bpf: Add bpf_copy_from_user_task_str kfunc Jordan Rome
2025-01-26 16:38 ` [bpf-next v5 3/3] selftests/bpf: Add tests for bpf_copy_from_user_task_str Jordan Rome
@ 2025-01-27 23:49 ` Andrii Nakryiko
2025-01-28 15:23 ` Jordan Rome
2 siblings, 1 reply; 6+ messages in thread
From: Andrii Nakryiko @ 2025-01-27 23:49 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 Sun, Jan 26, 2025 at 8:39 AM 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 | 118 +++++++++++++++++++++++++++++++++++++++++++++
> mm/nommu.c | 67 +++++++++++++++++++++++++
> 3 files changed, 188 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..e1ed5095b258 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -6714,6 +6714,124 @@ 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;
> +
> + 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.
> + */
> + 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);
> + unmap_and_put_page(page, maddr);
> +
> + if (retval > -1) {
nit: retval >= 0 is more conventional, -1 has no special meaning here
(it's -EPERM, if anything), zero does
> + /* Found the end of the string */
> + buf += retval;
> + goto out;
> + }
> +
> + retval = bytes - 1;
> + buf += retval;
> +
> + if (bytes == len)
> + goto out;
> +
> + /*
> + * Because strscpy always NUL terminates we need to
> + * copy the last byte in the page if we are going to
> + * load more pages
> + */
> + addr += retval;
> + len -= retval;
> + copy_from_user_page(vma,
> + page,
> + addr,
> + buf,
> + maddr + (PAGE_SIZE - 1),
> + 1);
just realized, you've already unmap_and_put_page(), and yet you are
trying to access it here again. Seems like you'll need to delay that
unmap_and_put
also, stylistical nit: you have tons of short arguments, keep them on
smaller number of lines, it's taking way too much vertical space for
what it is
Also, I was worried about non-zero terminated buf here. It still can
happen if subsequent get_user_page_vma_remote() fails and we exit
early, but we'll end up returning -EFAULT, so perhaps that's not a
problem. On the other hand, it's trivial to do buf[1] = '\0'; to keep
that buf always zero-terminated, so maybe I'd do that... And if we do
buf[0] = '\0'; at the very beginning, we can document that
copy_remote_vm_str() leaves valid zero-terminated buffer of whatever
it managed to read before EFAULT, which isn't a bad property that
basically comes for free, no?
pw-bot: cr
> + len -= 1;
> + buf += 1;
> + addr += 1;
> + }
> +
> +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. On any error, return -EFAULT.
"On success, leaves a properly NUL-terminated buffer." (or if we do
adjustments I suggested above we can even say "Even on error will
leave properly NUL-terminated buffer with contents that was
successfully read before -EFAULT", or something along those lines).
> + */
> +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)
> + 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..ce24ea829c73 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -1701,6 +1701,73 @@ 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)
> +{
> + int ret;
> + struct vm_area_struct *vma;
> +
> + if (mmap_read_lock_killable(mm))
> + return -EFAULT;
> +
> + /* the access must start within one of the target process's mappings */
> + vma = find_vma(mm, addr);
> + if (vma) {
> + /* don't overrun this mapping */
> + if (addr + len >= vma->vm_end)
Should we worry about overflows here? check_add_overflow() maybe?
> + 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;
> + } else {
> + ret = -EFAULT;
> + }
> + } else {
> + ret = -EFAULT;
> + }
> +
nit: might be cleaner to have `ret = -EFAULT;` before `if (vma)` and
only set ret on success/overflow?
> + 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. 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)
> + 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] 6+ messages in thread