linux-kernel - Re: [PATCH RFC V2 04/17] x86/pks: Preserve the PKRS MSR on context switch

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:   Fri, 17 Jul 2020 14:39:30 -0700
From:   Ira Weiny <ira.weiny@...el.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Andy Lutomirski <luto@...nel.org>,
        Fenghua Yu <fenghua.yu@...el.com>, x86@...nel.org,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Vishal Verma <vishal.l.verma@...el.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-nvdimm@...ts.01.org, linux-fsdevel@...r.kernel.org,
        linux-mm@...ck.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH RFC V2 04/17] x86/pks: Preserve the PKRS MSR on context
 switch

On Fri, Jul 17, 2020 at 10:31:40AM +0200, Peter Zijlstra wrote:
> On Fri, Jul 17, 2020 at 12:20:43AM -0700, ira.weiny@...el.com wrote:
> 
> > diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> > index f362ce0d5ac0..d69250a7c1bf 100644
> > --- a/arch/x86/kernel/process.c
> > +++ b/arch/x86/kernel/process.c
> > @@ -42,6 +42,7 @@
> >  #include <asm/spec-ctrl.h>
> >  #include <asm/io_bitmap.h>
> >  #include <asm/proto.h>
> > +#include <asm/pkeys_internal.h>
> >  
> >  #include "process.h"
> >  
> > @@ -184,6 +185,36 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long sp,
> >  	return ret;
> >  }
> >  
> > +/*
> > + * NOTE: We wrap pks_init_task() and pks_sched_in() with
> > + * CONFIG_ARCH_HAS_SUPERVISOR_PKEYS because using IS_ENABLED() fails
> > + * due to the lack of task_struct->saved_pkrs in this configuration.
> > + * Furthermore, we place them here because of the complexity introduced by
> > + * header conflicts introduced to get the task_struct definition in the pkeys
> > + * headers.
> > + */
> 
> I don't see anything much useful in that comment.

I'm happy to delete.  Internal reviews questioned the motive here so I added
the comment to inform why this style was chosen rather than the preferred
IS_ENABLED().

I've deleted it now.

> 
> > +#ifdef CONFIG_ARCH_HAS_SUPERVISOR_PKEYS
> > +DECLARE_PER_CPU(u32, pkrs_cache);
> > +static inline void pks_init_task(struct task_struct *tsk)
> > +{
> > +	/* New tasks get the most restrictive PKRS value */
> > +	tsk->thread.saved_pkrs = INIT_PKRS_VALUE;
> > +}
> > +static inline void pks_sched_in(void)
> > +{
> > +	u64 current_pkrs = current->thread.saved_pkrs;
> > +
> > +	/* Only update the MSR when current's pkrs is different from the MSR. */
> > +	if (this_cpu_read(pkrs_cache) == current_pkrs)
> > +		return;
> > +
> > +	write_pkrs(current_pkrs);
> 
> Should we write that like:
> 
> 	/*
> 	 * PKRS is only temporarily changed during specific code paths.
> 	 * Only a preemption during these windows away from the default
> 	 * value would require updating the MSR.
> 	 */
> 	if (unlikely(this_cpu_read(pkrs_cache) != current_pkrs))
> 		write_pkrs(current_pkrs);
> 
> ?

Yes I think the unlikely is better.

Thanks,
Ira

> 
> > +}
> > +#else
> > +static inline void pks_init_task(struct task_struct *tsk) { }
> > +static inline void pks_sched_in(void) { }
> > +#endif