linux-hardening - Re: [PATCH 3/6] PKEY: Apply PKEY_ENFORCE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 16 May 2023 21:50:31 -0700
From:   Jeff Xu <jeffxu@...omium.org>
To:     Jeff Xu <jeffxu@...gle.com>
Cc:     Dave Hansen <dave.hansen@...el.com>, luto@...nel.org,
        jorgelo@...omium.org, keescook@...omium.org, groeck@...omium.org,
        jannh@...gle.com, sroettger@...gle.com, akpm@...ux-foundation.org,
        linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
        linux-mm@...ck.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH 3/6] PKEY: Apply PKEY_ENFORCE_API to mprotect

On Tue, May 16, 2023 at 4:37 PM Jeff Xu <jeffxu@...gle.com> wrote:
>
> On Tue, May 16, 2023 at 4:19 PM Dave Hansen <dave.hansen@...el.com> wrote:
> >
> > On 5/15/23 06:05, jeffxu@...omium.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.