Re: [PATCH] sparc64: Add support for Application Data Integrity (ADI)

[Julian Calaby]

Hi Khalid,

On Fri, Mar 4, 2016 at 4:42 AM, Khalid Aziz <khalid.aziz@xxxxxxxxxx> wrote:
> On 03/02/2016 06:33 PM, Julian Calaby wrote:
>>
>> Hi Khalid,
>>
>> A couple of other comments:
>>
>> On Thu, Mar 3, 2016 at 5:54 AM, Khalid Aziz <khalid.aziz@xxxxxxxxxx>
>> wrote:
>>>
>>>
>>> Enable Application Data Integrity (ADI) support in the sparc
>>> kernel for applications to use ADI in userspace. ADI is a new
>>> feature supported on sparc M7 and newer processors. ADI is supported
>>> for data fetches only and not instruction fetches. This patch adds
>>> prctl commands to enable and disable ADI (TSTATE.mcde), return ADI
>>> parameters to userspace, enable/disable MCD (Memory Corruption
>>> Detection) on selected memory ranges and enable TTE.mcd in PTEs. It
>>> also adds handlers for all traps related to MCD. ADI is not enabled
>>> by default for any task and a task must explicitly enable ADI
>>> (TSTATE.mcde), turn MCD on on a memory range and set version tag
>>> for ADI to be effective for the task. This patch adds support for
>>> ADI for hugepages only. Addresses passed into system calls must be
>>> non-ADI tagged addresses.
>>>
>>> Signed-off-by: Khalid Aziz <khalid.aziz@xxxxxxxxxx>
>>> ---
>>> NOTES: ADI is a new feature added to M7 processor to allow hardware
>>>          to catch rogue accesses to memory. An app can enable ADI on
>>>          its data pages, set version tags on them and use versioned
>>>          addresses (bits 63-60 of the address contain a version tag)
>>>          to access the data pages. If a rogue app attempts to access
>>>          ADI enabled data pages, its access is blocked and processor
>>>          generates an exception. Enabling this functionality for all
>>>          data pages of an app requires adding infrastructure to save
>>>          version tags for any data pages that get swapped out and
>>>          restoring those tags when pages are swapped back in. In this
>>>          first implementation I am enabling ADI for hugepages only
>>>          since these pages are locked in memory and hence avoid the
>>>          issue of saving and restoring tags. Once this core functionality
>>>          is stable, ADI for other memory pages can be enabled more
>>>          easily.
>>>
>>>   Documentation/prctl/sparc_adi.txt     |  62 ++++++++++
>>>   Documentation/sparc/adi.txt           | 206
>>> +++++++++++++++++++++++++++++++
>>>   arch/sparc/Kconfig                    |  12 ++
>>>   arch/sparc/include/asm/hugetlb.h      |  14 +++
>>>   arch/sparc/include/asm/hypervisor.h   |   2 +
>>>   arch/sparc/include/asm/mmu_64.h       |   1 +
>>>   arch/sparc/include/asm/pgtable_64.h   |  15 +++
>>>   arch/sparc/include/asm/processor_64.h |  19 +++
>>>   arch/sparc/include/asm/ttable.h       |  10 ++
>>>   arch/sparc/include/uapi/asm/asi.h     |   3 +
>>>   arch/sparc/include/uapi/asm/pstate.h  |  10 ++
>>>   arch/sparc/kernel/entry.h             |   3 +
>>>   arch/sparc/kernel/head_64.S           |   1 +
>>>   arch/sparc/kernel/mdesc.c             |  81 +++++++++++++
>>>   arch/sparc/kernel/process_64.c        | 221
>>> ++++++++++++++++++++++++++++++++++
>>>   arch/sparc/kernel/sun4v_mcd.S         |  16 +++
>>>   arch/sparc/kernel/traps_64.c          |  96 ++++++++++++++-
>>>   arch/sparc/kernel/ttable_64.S         |   6 +-
>>>   include/linux/mm.h                    |   2 +
>>>   include/uapi/asm-generic/siginfo.h    |   5 +-
>>>   include/uapi/linux/prctl.h            |  16 +++
>>>   kernel/sys.c                          |  30 +++++
>>>   22 files changed, 825 insertions(+), 6 deletions(-)
>>>   create mode 100644 Documentation/prctl/sparc_adi.txt
>>>   create mode 100644 Documentation/sparc/adi.txt
>>>   create mode 100644 arch/sparc/kernel/sun4v_mcd.S
>>>
>>> diff --git a/arch/sparc/include/asm/pgtable_64.h
>>> b/arch/sparc/include/asm/pgtable_64.h
>>> index 131d36f..cddea30 100644
>>> --- a/arch/sparc/include/asm/pgtable_64.h
>>> +++ b/arch/sparc/include/asm/pgtable_64.h
>>> @@ -162,6 +162,9 @@ bool kern_addr_valid(unsigned long addr);
>>>   #define _PAGE_E_4V       _AC(0x0000000000000800,UL) /* side-Effect
>>> */
>>>   #define _PAGE_CP_4V      _AC(0x0000000000000400,UL) /* Cacheable in
>>> P-Cache */
>>>   #define _PAGE_CV_4V      _AC(0x0000000000000200,UL) /* Cacheable in
>>> V-Cache */
>>> +/* Bit 9 is used to enable MCD corruption detection instead on M7
>>> + */
>>> +#define _PAGE_MCD_4V     _AC(0x0000000000000200,UL) /* Memory Corruption
>>> */
>>
>>
>> I'm not sure that everywhere _PAGE_CV_4V is used is guarded against
>> setting it on M7, could someone who knows the code better than I do
>> please check that? It looks like the tests around it's use are
>> essentially "is it sun4v".
>>
>> I'm probably being paranoid, but reused values like this make me worry.
>>
>
> I took care of this issue in an earlier patch (commit
> 494e5b6faeda1d1e830a13e10b3c7bc323f35d97 - "sparc: Resolve conflict between
> sparc v9 and M7 on usage of bit 9 of TTE"), so I think we are ok here.

Ah, I remember those changes, however I didn't recall which processor
they were for, so that's awesome.

>>>   #define _PAGE_P_4V       _AC(0x0000000000000100,UL) /* Privileged Page
>>> */
>>>   #define _PAGE_EXEC_4V    _AC(0x0000000000000080,UL) /* Executable Page
>>> */
>>>   #define _PAGE_W_4V       _AC(0x0000000000000040,UL) /* Writable
>>> */
>>> diff --git a/arch/sparc/include/uapi/asm/pstate.h
>>> b/arch/sparc/include/uapi/asm/pstate.h
>>> index cf832e1..d0521db 100644
>>> --- a/arch/sparc/include/uapi/asm/pstate.h
>>> +++ b/arch/sparc/include/uapi/asm/pstate.h
>>> @@ -10,7 +10,12 @@
>>>    *
>>> -----------------------------------------------------------------------
>>>    *  63  12  11   10    9     8    7   6   5     4     3     2     1
>>> 0
>>>    */
>>> +/* IG on V9 conflicts with MCDE on M7. PSTATE_MCDE will only be used on
>>> + * processors that support ADI which do not use IG, hence there is no
>>> + * functional conflict
>>> + */
>>>   #define PSTATE_IG   _AC(0x0000000000000800,UL) /* Interrupt Globals.
>>> */
>>> +#define PSTATE_MCDE _AC(0x0000000000000800,UL) /* MCD Enable
>>> */
>>
>>
>> Again, I can't tell if the code that uses PSTATE_IG is guarded against
>> use on M7. Could someone else please check? It's used in cherrs.S
>> which appears to be Cheetah specific, so that's not a problem, however
>> it's also used in ultra.S in xcall_sync_tick which might get patched
>> out however I don't know the code well enough to be certain. I'm also
>> guessing that as this file is in include/uapi, userspace could use it
>> for something.
>
>
> My understanding of the code in ultra.S is xcall_sync_tick doe snot get
> called on sun4v, so PSTATE_IG will not get set unintentionally on M7.
> include/uapi is an interesting thought. PSTATE is a privileged register, so
> userspace can not write to it directly without using a system call. I don't
> think that is an issue here.

Is it something userspace would interpret then? I guess any software
making use of the PSTATE or TSTATE registers would have to handle any
processor differences itself.

Thanks for explaining this!

-- 
Julian Calaby

Email: julian.calaby@xxxxxxxxx
Profile: http://www.google.com/profiles/julian.calaby/