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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
>
> 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.
