Re: [bpf-next v3 1/3] mm: add copy_remote_vm_str

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Fri, Jan 24, 2025 at 10:22 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        | 119 +++++++++++++++++++++++++++++++++++++++++++++
>  mm/nommu.c         |  68 ++++++++++++++++++++++++++
>  3 files changed, 190 insertions(+)
>

[...]

> +               maddr = kmap_local_page(page);
> +               retval = strscpy(buf, maddr + offset, bytes);
> +               unmap_and_put_page(page, maddr);
> +
> +               if (retval > -1 && retval < bytes) {
> +                       /* found the end of the string */
> +                       buf += retval;
> +                       goto out;
> +               }
> +
> +               if (retval == -E2BIG) {

nit: strscpy() can't return any other error, so I'd structure result
handling as:

if (retval < 0) {
  /* that annoying last byte copy */
  retval = bytes;
}
if (retval < bytes) {
    /* "we are done" handling */
}

/* common len, buf, addr adjustment logic stays here */


but also here's the question. If we get E2BIG, while bytes is exactly
how many bytes we have left in the buffer, the last byte should be
zero, no? So this should be cleanly handled, right? Or do we have a
test for that and it works already?

> +                       retval = bytes;
> +                       /*
> +                        * Because strscpy always null terminates we need to
> +                        * copy the last byte in the page if we are going to
> +                        * load more pages
> +                        */
> +                       if (bytes < len) {
> +                               end = bytes - 1;
> +                               copy_from_user_page(vma,
> +                                               page,
> +                                               addr + end,
> +                                               buf + end,

you don't need the `end` variable, just use `bytes - 1` twice?

> +                                               maddr + (PAGE_SIZE - 1),
> +                                               1);
> +                       }
> +               }
> +
> +               len -= retval;
> +               buf += retval;
> +               addr += retval;
> +       }
> +
> +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 transfer
> + * @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).
> + * If the source string is shorter than @len then return the length of the
> + * source string. If the source string is longer than @len, return @len.
> + * On any error, return -EFAULT.

strncpy_from_user_nofault() doc says:

  On success, returns the length of the string INCLUDING the trailing NUL

Is this the case with copy_remote_vm_str() as well? I.e., if the
source string is 5 bytes + NUL, dst buf is 10. Will we get 5 or 6
returned? We should be very careful with all this +/- 1 business in
corner cases, too easy to mess this up.

> + */
> +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);
> +

[...]