linux-kernel - Re: [PATCH 0/9] [v3] x86, pkeys: two protection keys bug fixes

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] [day] [month] [year] [list]

Date:   Tue, 8 May 2018 15:49:39 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     Ingo Molnar <mingo@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>
Cc:     linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        linuxram@...ibm.com, tglx@...utronix.de, mpe@...erman.id.au,
        akpm@...ux-foundation.org, shuah@...nel.org, shakeelb@...gle.com
Subject: Re: [PATCH 0/9] [v3] x86, pkeys: two protection keys bug fixes

> 1)
> 
> Minor patch series organization requests:
> 
>  - please include the shortlog and diffstat in the cover letter in the future, as 
>    it makes it easier to see the overall structure and makes it easier to reply to 
>    certain commits as a group.

Will do.

>  - please capitalize commit titles as is usually done in arch/x86/ and change the 
>    change the subsystem tags to the usual ones:
> 
> d76eeb1914c8: x86/pkeys: Override pkey when moving away from PROT_EXEC
> f30f10248200: x86/pkeys/selftests: Add PROT_EXEC test
> 0530ebfefcdc: x86/pkeys/selftests: Add allow faults on unknown keys
> e81c40e33818: x86/pkeys/selftests: Factor out "instruction page"
> 57042882631c: x86/pkeys/selftests: Fix pkey exhaustion test off-by-one
> 6b833e9d3171: x86/pkeys/selftests: Fix pointer math
> d16f12e3c4ca: x86/pkeys: Do not special case protection key 0
> 1cb7691d0ee4: x86/pkeys/selftests: Add a test for pkey 0
> 273ae5cde423: x86/pkeys/selftests: Save off 'prot' for allocations
>
>  - please re-order the series to first introduce a unit test which specifically 
>    tests for the failure, ascertain that it indeed fails, and then apply the 
>    kernel fix. I.e. please use the order I used above for future versions of this 
>    patch-set.

I can't _quite_ use this order, but I get your point and I'll do as you
suggest, conceptually.


> 2)
> 
> The new self-test you added does not fail overly nicely, it does the following on 
> older kernels:
...
> I.e. x86 unit tests should never 'crash' in a way that suggests that the testing 
> itself might be buggy - the crashes/failures should always be well controlled.

I've tried to make this nicer.  I never abort() any more, for instance.

> 3)
> 
> When the first kernel bug fix is applied but not the second, then I don't see the 
> new PROT_EXEC test catching the bug:

Thanks for catching this.  I forgot to add the test function to the
pkey_tests[] array.  It's fixed up now.

> 4)
> 
> In the above kernel that was missing the PROT_EXEC fix I was repeatedly running 
> the 64-bit and 32-bit testcases as non-root and as root as well, until I got a 
> hang in the middle of a 32-bit test running as root:

I believe this is all my stupidity from not being careful about using
signal-safe functions in the signal handlers.  There's no pretty
solution for this, but I've at least made it stop hanging.  The fixes
for that will be in the beginning of the next series.