Re: [RFC v2 01/13] x86/mktme: Document the MKTME APIs

["Sakkinen, Jarkko" <jarkko.sakkinen@intel.com>]

I'll focus my remarks now towards documentation as I have lots of
catching up to do with TME :-) I'll give more feedback of actual code
changes once v18 of the SGX patch set is out, pull request for TPM 4.21
changes is out and maybe a new version of this patch set has been
released.

Right now too much on my shoulders to go too deep with this patch set.

On Mon, 2018-12-03 at 23:39 -0800, Alison Schofield wrote:
> This includes an overview, a section on each API: MTKME Keys and
> system call encrypt_mprotect(), and a demonstration program.
> 
> (Some of this info is destined for man pages.)
> 
> Change-Id: I34dc9ff1a1308c057ec4bb3e652c4d7ce6995606
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>

Co-developed-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> ?

Not needed if this is for the most part written by you.

> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  Documentation/x86/mktme/index.rst          |  11 +++
>  Documentation/x86/mktme/mktme_demo.rst     |  53 ++++++++++++++
>  Documentation/x86/mktme/mktme_encrypt.rst  |  58 +++++++++++++++
>  Documentation/x86/mktme/mktme_keys.rst     | 109
> +++++++++++++++++++++++++++++
>  Documentation/x86/mktme/mktme_overview.rst |  60 ++++++++++++++++
>  5 files changed, 291 insertions(+)
>  create mode 100644 Documentation/x86/mktme/index.rst
>  create mode 100644 Documentation/x86/mktme/mktme_demo.rst
>  create mode 100644 Documentation/x86/mktme/mktme_encrypt.rst
>  create mode 100644 Documentation/x86/mktme/mktme_keys.rst
>  create mode 100644 Documentation/x86/mktme/mktme_overview.rst
> 
> diff --git a/Documentation/x86/mktme/index.rst
> b/Documentation/x86/mktme/index.rst
> new file mode 100644
> index 000000000000..8c556d04cbc4
> --- /dev/null
> +++ b/Documentation/x86/mktme/index.rst
> @@ -0,0 +1,11 @@
> +

SPDX?

> +=============================================
> +Multi-Key Total Memory Encryption (MKTME) API
> +=============================================
> +
> +.. toctree::
> +
> +   mktme_overview
> +   mktme_keys
> +   mktme_encrypt
> +   mktme_demo
> diff --git a/Documentation/x86/mktme/mktme_demo.rst
> b/Documentation/x86/mktme/mktme_demo.rst
> new file mode 100644
> index 000000000000..afd50772e65d
> --- /dev/null
> +++ b/Documentation/x86/mktme/mktme_demo.rst
> @@ -0,0 +1,53 @@
> +Demonstration Program using MKTME API's
> +=======================================

Probably would be better idea to put into tools/testings/selftest/x86
and not as part of the documentation.

> +
> +/* Compile with the keyutils library: cc -o mdemo mdemo.c -lkeyutils */
> +
> +#include <sys/mman.h>
> +#include <sys/syscall.h>
> +#include <sys/types.h>
> +#include <keyutils.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <unistd.h>
> +
> +#define PAGE_SIZE sysconf(_SC_PAGE_SIZE)
> +#define sys_encrypt_mprotect 335
> +
> +void main(void)
> +{
> +	char *options_CPU = "algorithm=aes-xts-128 type=cpu";
> +	long size = PAGE_SIZE;
> +        key_serial_t key;
> +	void *ptra;
> +	int ret;
> +
> +        /* Allocate an MKTME Key */
> +	key = add_key("mktme", "testkey", options_CPU, strlen(options_CPU),
> +                      KEY_SPEC_THREAD_KEYRING);
> +
> +	if (key == -1) {
> +		printf("addkey FAILED\n");
> +		return;
> +	}
> +        /* Map a page of ANONYMOUS memory */
> +	ptra = mmap(NULL, size, PROT_NONE, MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
> +	if (!ptra) {
> +		printf("failed to mmap");
> +		goto inval_key;
> +	}
> +        /* Encrypt that page of memory with the MKTME Key */
> +	ret = syscall(sys_encrypt_mprotect, ptra, size, PROT_NONE, key);
> +	if (ret)
> +		printf("mprotect error [%d]\n", ret);
> +
> +        /* Enjoy that page of encrypted memory */
> +
> +        /* Free the memory */
> +	ret = munmap(ptra, size);
> +
> +inval_key:
> +        /* Free the Key */
> +	if (keyctl(KEYCTL_INVALIDATE, key) == -1)
> +		printf("invalidate failed on key [%d]\n", key);

Would it make sense to print error messages to stderr?

> +}
> diff --git a/Documentation/x86/mktme/mktme_encrypt.rst
> b/Documentation/x86/mktme/mktme_encrypt.rst
> new file mode 100644
> index 000000000000..ede5237183fc
> --- /dev/null
> +++ b/Documentation/x86/mktme/mktme_encrypt.rst
> @@ -0,0 +1,58 @@
> +MKTME API: system call encrypt_mprotect()
> +=========================================
> +
> +Synopsis
> +--------
> +int encrypt_mprotect(void \*addr, size_t len, int prot, key_serial_t serial);
> +
> +Where *key_serial_t serial* is the serial number of a key allocated
> +using the MKTME Key Service.

There is only one key service i.e. the kernel keyring. Should be rephrased
somehow.

> +
> +Description
> +-----------
> +    encrypt_mprotect() encrypts the memory pages containing any part
> +    of the address range in the interval specified by addr and len.

What does it actually do? I don't think the syscall does any encryption,
does it? I'm not looking SDM level details but somehow better
description what does it do would be nice.

> +
> +    encrypt_mprotect() supports the legacy mprotect() behavior plus
> +    the enabling of memory encryption. That means that in addition
> +    to encrypting the memory, the protection flags will be updated
> +    as requested in the call.

Ditto.

> +
> +    The *addr* and *len* must be aligned to a page boundary.
> +
> +    The caller must have *KEY_NEED_VIEW* permission on the key.

Maybe more verbose description, especially when it is a must.

> +
> +    The range of memory that is to be protected must be mapped as
> +    *ANONYMOUS*.

Ditto.

> +
> +Errors
> +------
> +    In addition to the Errors returned from legacy mprotect()
> +    encrypt_mprotect will return:
> +
> +    ENOKEY *serial* parameter does not represent a valid key.
> +
> +    EINVAL *len* parameter is not page aligned.
> +
> +    EACCES Caller does not have *KEY_NEED_VIEW* permission on the key.
> +
> +EXAMPLE
> +--------
> +  Allocate an MKTME Key::
> +        serial = add_key("mktme", "name", "type=cpu algorithm=aes-xts-128" @u
> +
> +  Map ANONYMOUS memory::
> +        ptr = mmap(NULL, size, PROT_NONE, MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
> +
> +  Protect memory::
> +        ret = syscall(SYS_encrypt_mprotect, ptr, size, PROT_READ|PROT_WRITE,
> +                      serial);
> +
> +  Use the encrypted memory
> +
> +  Free memory::
> +        ret = munmap(ptr, size);
> +
> +  Free the key resource::
> +        ret = keyctl(KEYCTL_INVALIDATE, serial);
> +

Not really sure what this example serves as you already had an example
program...

Have you read pkeys man page? It has really good balance explaining how
it is implemented and used (man pkeys, not man pkey_mprotect()).

What if you suddenly change a key for VMA? I guess memory is then
corrupted? Not documented here. Should be.

I did not find the thing I was looking for most i.e. some high level
description of the threat model. Emphasis on high-level as kernel
documentation is not a CVE database.

> diff --git a/Documentation/x86/mktme/mktme_keys.rst
> b/Documentation/x86/mktme/mktme_keys.rst
> new file mode 100644
> index 000000000000..5837909b2c54
> --- /dev/null
> +++ b/Documentation/x86/mktme/mktme_keys.rst
> @@ -0,0 +1,109 @@
> +MKTME Key Service API
> +=====================
> +MKTME is a new key service type added to the Linux Kernel Key Service.
> +
> +The MKTME Key Service type is available when CONFIG_X86_INTEL_MKTME is
> +turned on in Intel platforms that support the MKTME feature.
> +
> +The MKTME Key Service type manages the allocation of hardware encryption
> +keys. Users can request an MKTME type key and then use that key to
> +encrypt memory with the encrypt_mprotect() system call.
> +
> +Usage
> +-----
> +    When using the Kernel Key Service to request an *mktme* key,
> +    specify the *payload* as follows:
> +
> +    type=
> +        *user*	User will supply the encryption key data. Use this
> +                type to directly program a hardware encryption key.
> +
> +        *cpu*	User requests a CPU generated encryption key.
> +                The CPU generates and assigns an ephemeral key.

How are these implemented? Is there an opcode to request CPU to generate
a key, or? What about the user key? Does cpu key ever leave out of the
CPU package?

The user key sounds like a really bad idea at the first sight and maybe
should be considered to be left out. What would be a legit use case for
it?

Are the keys per-process or is it a global resource?

> +        *clear* User requests that a hardware encryption key be
> +                cleared. This will clear the encryption key from
> +                the hardware. On execution this hardware key gets
> +                TME behavior.
> +
> +        *no-encrypt*
> +                 User requests that hardware does not encrypt
> +                 memory when this key is in use.

Not sure about these with my current knowledge.

> +
> +    algorithm=
> +        When type=user or type=cpu the algorithm field must be
> +        *aes-xts-128*
> +
> +        When type=clear or type=no-encrypt the algorithm field
> +        must not be present in the payload.

This parameter must be removed as it is a function of other paramaters
and nothing else i.e. complexity without gain.

> +	This document does not intend to document KKS, but only the
> +	MKTME type of the KKS. The options of the KKS can be grouped
> +	into 2 classes for purposes of understanding how MKTME operates
> +	within the broader KKS.

Maybe just delete this paragraph? I think it is just stating the
obvious.

I think you need this paragraph only because you have deployed this
document to wrong place. Better path would be

Documentation/security/keys/mktme.rst.

/Jarkko