Re: [bpf-next v2 1/2] bpf: Add bpf_copy_from_user_task_str kfunc

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

On Thu, Jan 16, 2025 at 6:22 PM Jordan Rome <linux@jordanrome.com> wrote:
>
> On Fri, Jan 10, 2025 at 4:50 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Mon, Jan 6, 2025 at 6:12 PM Jordan Rome <linux@jordanrome.com> wrote:
> > >
> > > This new kfunc will be able to copy a string
> > > from another process's/task's address space.
> >
> > nit: this is kernel code, task is unambiguous, so I'd drop the
> > "process" reference here
> >
> > > This is similar to `bpf_copy_from_user_str`
> > > but accepts a `struct task_struct*` argument.
> > >
> > > This required adding an additional function
> > > in memory.c, namely `copy_str_from_process_vm`,
> > > which works similar to `access_process_vm`
> > > but utilizes the `strncpy_from_user` helper
> > > and only supports reading/copying and not writing.
> > >
> > > Signed-off-by: Jordan Rome <linux@jordanrome.com>
> > > ---
> > >  include/linux/mm.h   |   3 ++
> > >  kernel/bpf/helpers.c |  46 ++++++++++++++++++++
> > >  mm/memory.c          | 101 +++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 150 insertions(+)
> > >
> >
> > please check kernel test bot's complains as well
>
> Maybe I need an entry in nommu.c as well for 'copy_remote_vm_str'?

yep, probably, I see that access_remote_vm has two implementations as well

>
> >
> > > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > > index c39c4945946c..52b304b20630 100644
> > > --- a/include/linux/mm.h
> > > +++ b/include/linux/mm.h

[...]

> > > +/*
> > > + * Copy a string from another process's address space as given in mm.
> > > + * Don't return partial results. If there is any error return -EFAULT.
> >
> > What does "don't return partial results" mean? What happens if we read
> > part of a string and then fail to read the rest?
>
> As per the last sentence, if we fail to read the rest of the string then
> an -EFAULT is returned.

This is confusing because the buffer will contain the partially read
string contents even with -EFAULT. And this "Don't return partial
results" can be interpreted as this API sanitizing the buffer (by
zeroing or something). So I'd drop the "Don't return partial results"
part, as it just creates confusion.

>
> >
> > > + */

[...]

> >
> > > +               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;
> > > +                       break;
> > > +               }
> > > +
> > > +               bytes = len;
> > > +               offset = addr & (PAGE_SIZE - 1);
> > > +               if (bytes > PAGE_SIZE - offset)
> > > +                       bytes = PAGE_SIZE - offset;
> > > +
> > > +               maddr = kmap_local_page(page);
> > > +               retval = strncpy_from_user(buf, (const char __user *)addr, bytes);
> >
> > you are not using maddr... that seems wrong (even if it works due to
> > how kmap_local_page is currently implemented)
>
> How do you think we should handle it then?

Use (maddr + offset_within_the_page) instead of addr itself?

>
> >
> > > +               unmap_and_put_page(page, maddr);
> > > +
> > > +               if (retval < 0) {
> > > +                       err = retval;
> > > +                       break;
> > > +               }
> > > +
> > > +               len -= retval;
> > > +               buf += retval;
> > > +               addr += retval;
> > > +
> > > +               /* Found the end of the string */
> > > +               if (retval < bytes)
> > > +                       break;
> > > +       }
> >

[...]