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: <311ef2df-dd49-0711-aa8b-fa88a87ff196@intel.com>
Date:   Fri, 18 Feb 2022 07:28:04 -0800
From:   Dave Hansen <dave.hansen@...el.com>
To:     Ira Weiny <ira.weiny@...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 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.

>> 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.
 */

>> 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? ;)