On Tue, Feb 11, 2025 at 9:19 PM Shakeel Butt <shakeel.butt@xxxxxxxxx> 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@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.
Ack.
