| Message ID | 20250210221626.2098522-1-linux@jordanrome.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [bpf-next,v7,1/3] mm: add copy_remote_vm_str | expand |
On Mon, Feb 10, 2025 at 2:23 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 | 73 +++++++++++++++++++++++++++ > 3 files changed, 195 insertions(+) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 7b1068ddcbb7..aee23d84ce01 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -2486,6 +2486,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 539c0f7c6d54..e9d8584a7f56 100644 > --- a/mm/memory.c > +++ b/mm/memory.c > @@ -6803,6 +6803,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'; LGTM overall: Acked-by: Andrii Nakryiko <andrii@kernel.org> But note that all this unconditional buf access will be incorrect if len == 0. So either all of that has to be guarded with `if (len)`, just dropped, or declared unsupported, depending on what mm folks think. BPF helper won't ever call with len == 0, so that's why my ack. (And yes, it would be nice to hear from someone from the MM side at this point, thank you!). > + > + 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; > + } > + [...]
On Tue, Feb 11, 2025 at 02:07:31PM -0800, Andrii Nakryiko wrote: > On Mon, Feb 10, 2025 at 2:23 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 | 73 +++++++++++++++++++++++++++ > > 3 files changed, 195 insertions(+) > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > index 7b1068ddcbb7..aee23d84ce01 100644 > > --- a/include/linux/mm.h > > +++ b/include/linux/mm.h > > @@ -2486,6 +2486,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 539c0f7c6d54..e9d8584a7f56 100644 > > --- a/mm/memory.c > > +++ b/mm/memory.c > > @@ -6803,6 +6803,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'; > > LGTM overall: > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > > But note that all this unconditional buf access will be incorrect if > len == 0. So either all of that has to be guarded with `if (len)`, > just dropped, or declared unsupported, depending on what mm folks > think. BPF helper won't ever call with len == 0, so that's why my ack. I think early return 0 on len == 0 should be fine.
On Tue, Feb 11, 2025 at 9:19 PM Shakeel Butt <shakeel.butt@linux.dev> wrote: > > On Tue, Feb 11, 2025 at 02:07:31PM -0800, Andrii Nakryiko wrote: > > On Mon, Feb 10, 2025 at 2:23 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 | 73 +++++++++++++++++++++++++++ > > > 3 files changed, 195 insertions(+) > > > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > > index 7b1068ddcbb7..aee23d84ce01 100644 > > > --- a/include/linux/mm.h > > > +++ b/include/linux/mm.h > > > @@ -2486,6 +2486,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 539c0f7c6d54..e9d8584a7f56 100644 > > > --- a/mm/memory.c > > > +++ b/mm/memory.c > > > @@ -6803,6 +6803,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'; > > > > LGTM overall: > > > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > > > > But note that all this unconditional buf access will be incorrect if > > len == 0. So either all of that has to be guarded with `if (len)`, > > just dropped, or declared unsupported, depending on what mm folks > > think. BPF helper won't ever call with len == 0, so that's why my ack. > > I think early return 0 on len == 0 should be fine. Ack.
On Mon, Feb 10, 2025 at 02:16:24PM -0800, Jordan Rome 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> If you decide to send a newer version, my request would be to add some more text in the commit message particularly on the motivation. Otherwise this looks good to me. Reviewed-by: Shakeel Butt <shakeel.butt@linux.dev>
diff --git a/include/linux/mm.h b/include/linux/mm.h index 7b1068ddcbb7..aee23d84ce01 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -2486,6 +2486,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 539c0f7c6d54..e9d8584a7f56 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -6803,6 +6803,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'; + + 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'; + 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) { + /* Found the end of the string */ + buf += retval; + unmap_and_put_page(page, maddr); + break; + } + + 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); + } + +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'; + 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 baa79abdaf03..ec3bfcb35215 100644 --- a/mm/nommu.c +++ b/mm/nommu.c @@ -1708,6 +1708,79 @@ 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) +{ + unsigned long addr_end; + struct vm_area_struct *vma; + int ret = -EFAULT; + + *(char *)buf = '\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, &addr_end)) + goto out; + /* don't overrun this mapping */ + if (addr_end > 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'; + 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
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 | 73 +++++++++++++++++++++++++++ 3 files changed, 195 insertions(+) -- 2.43.5