linux-kernel - Re: [PATCH V8 19/44] mm/pkeys: PKS Testing, add pks_mk

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]

Message-ID: <Yg/Wd4We9uinmGYk@iweiny-desk3>
Date:   Fri, 18 Feb 2022 09:25:11 -0800
From:   Ira Weiny <ira.weiny@...el.com>
To:     Dave Hansen <dave.hansen@...el.com>
Cc:     Dave Hansen <dave.hansen@...ux.intel.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        Rick Edgecombe <rick.p.edgecombe@...el.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH V8 19/44] mm/pkeys: PKS Testing, add pks_mk_*() tests

On Fri, Feb 18, 2022 at 07:28:04AM -0800, Dave Hansen wrote:
> On 2/17/22 21:34, Ira Weiny wrote:
> > On Tue, Feb 01, 2022 at 09:45:03AM -0800, Dave Hansen wrote:
> >> On 1/27/22 09:54, ira.weiny@...el.com wrote:
> >>>  bool pks_test_callback(void)
> >>>  {
> >>> -	return false;
> >>> +	bool armed = (test_armed_key != 0);
> >>> +
> >>> +	if (armed) {
> >>> +		pks_mk_readwrite(test_armed_key);
> >>> +		fault_cnt++;
> >>> +	}
> >>> +
> >>> +	return armed;
> >>> +}
> >>
> >> Where's the locking for all this?  I don't think we need anything fancy,
> >> but is there anything preventing the test from being started from
> >> multiple threads at the same time?  I think a simple global test mutex
> >> would probably suffice.
> > 
> > Good idea.  Generally I don't see that happening but it is good to be safe.
> 
> I'm not sure what you mean.
> 
> In the kernel, we always program as if userspace is out to get us.  If
> userspace can possibly do something to confuse the kernel, it will.  It
> might be malicious or incompetent, but it will happen.
> 
> This isn't really a "good to be safe" kind of thing.  Kernel code must
> *be* safe.

Yes

> 
> >> Also, pks_test_callback() needs at least a comment or two about what
> >> it's doing.
> > 
> > The previous patch which adds this call in the fault handler contains the
> > following comment which is in the final code:
> > 
> > /*
> >  * pks_test_callback() is called by the fault handler to indicate it saw a pkey
> >  * fault.
> >  *
> >  * NOTE: The callback is responsible for clearing any condition which would
> >  * cause the fault to re-trigger.
> >  */
> > 
> > Would you like more comments within the function?
> 
> Ahh, it just wasn't in the context.
> 
> Looking at this again, I don't really like the name "callback" is almost
> always a waste of bytes.  Imagine this was named something like:
> 
> 	pks_test_induced_fault();
> 
> ... and had a comment like:
> 
> /*
>  * Ensure that the fault handler does not treat
>  * test-induced faults as actual errors.
>  */

Ok.  At this point this may go away depending on how I resolve the ability to
test all the keys.  pks_test_callback() was critical for that feature without
introducing a bunch of ugly test code in pks-keys.h and pkeys.c.

> 
> >> Does this work if you have a test armed and then you get an unrelated
> >> PKS fault on another CPU?  I think this will disarm the test from the
> >> unrelated thread.
> > 
> > This code will detect a false fault.  
> 
> That's a bug that's going to get fixed, right? ;)

Yep.  Not sure how at the moment.

Ira