Re: [PATCH 3/6] PKEY: Apply PKEY_ENFORCE_API to mprotect

[Jeff Xu <jeffxu@chromium.org>]

On Tue, May 16, 2023 at 4:37 PM Jeff Xu <jeffxu@google.com> wrote:
>
> On Tue, May 16, 2023 at 4:19 PM Dave Hansen <dave.hansen@intel.com> wrote:
> >
> > On 5/15/23 06:05, jeffxu@chromium.org wrote:
> > >  /*
> > >   * pkey==-1 when doing a legacy mprotect()
> > > + * syscall==true if this is called by syscall from userspace.
> > > + * Note: this is always true for now, added as a reminder in case that
> > > + * do_mprotect_pkey is called directly by kernel in the future.
> > > + * Also it is consistent with __do_munmap().
> > >   */
> > >  static int do_mprotect_pkey(unsigned long start, size_t len,
> > > -             unsigned long prot, int pkey)
> > > +             unsigned long prot, int pkey, bool syscall)
> > >  {
> >
> > The 'syscall' seems kinda silly (and a bit confusing).  It's easy to
> > check if the caller is a kthread or has a current->mm==NULL.  If you
> > *really* want a warning, I'd check for those rather than plumb a
> > apparently unused argument in here.
> >
> > BTW, this warning is one of those things that will probably cause some
> > amount of angst.  I'd move it to the end of the series or just axe it
> > completely.
>
Okay, I will move the logging part to the end of the series.


> Agreed. syscall is not a good name here.
> The intention is to check this at the system call entry point
> For example, munmap can get called inside mremap(), but by that time
> mremap() should already check that all the memory is writeable.
>
> I will remove "syscall" from do_mprotect_pkey signature, it seems it caused
> more confusion than helpful.  I will keep the comments/note in place to remind
> future developer.