Linux-Security-Module Archive on lore.kernel.org
 help / color / Atom feed
From: "Sakkinen, Jarkko" <jarkko.sakkinen@intel.com>
To: "tglx@linutronix.de" <tglx@linutronix.de>,
	"Schofield, Alison" <alison.schofield@intel.com>,
	"dhowells@redhat.com" <dhowells@redhat.com>
Cc: "kirill.shutemov@linux.intel.com"
	<kirill.shutemov@linux.intel.com>,
	"peterz@infradead.org" <peterz@infradead.org>,
	"jmorris@namei.org" <jmorris@namei.org>,
	"Huang, Kai" <kai.huang@intel.com>,
	"keyrings@vger.kernel.org" <keyrings@vger.kernel.org>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	"linux-security-module@vger.kernel.org" 
	<linux-security-module@vger.kernel.org>,
	"Williams, Dan J" <dan.j.williams@intel.com>,
	"x86@kernel.org" <x86@kernel.org>,
	"hpa@zytor.com" <hpa@zytor.com>,
	"mingo@redhat.com" <mingo@redhat.com>,
	"luto@kernel.org" <luto@kernel.org>,
	"bp@alien8.de" <bp@alien8.de>,
	"Hansen, Dave" <dave.hansen@intel.com>,
	"Nakajima, Jun" <jun.nakajima@intel.com>
Subject: Re: [RFC v2 01/13] x86/mktme: Document the MKTME APIs
Date: Thu, 6 Dec 2018 08:04:09 +0000
Message-ID: <10b900d7461d81433120e614287d6f837cea83ef.camel@intel.com> (raw)
In-Reply-To: <c2276bbbb19f3a28bd37c3dd6b1021e2d9a10916.1543903910.git.alison.schofield@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

  parent reply index

Thread overview: 91+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-04  7:39 [RFC v2 00/13] Multi-Key Total Memory Encryption API (MKTME) Alison Schofield
2018-12-04  7:39 ` [RFC v2 01/13] x86/mktme: Document the MKTME APIs Alison Schofield
2018-12-05 18:11   ` Andy Lutomirski
2018-12-05 19:22     ` Alison Schofield
2018-12-05 23:35       ` Andy Lutomirski
2018-12-06  8:04   ` Sakkinen, Jarkko [this message]
2018-12-04  7:39 ` [RFC v2 02/13] mm: Generalize the mprotect implementation to support extensions Alison Schofield
2018-12-06  8:08   ` Sakkinen, Jarkko
2018-12-04  7:39 ` [RFC v2 03/13] syscall/x86: Wire up a new system call for memory encryption keys Alison Schofield
2018-12-04  7:39 ` [RFC v2 04/13] x86/mm: Add helper functions for MKTME " Alison Schofield
2018-12-04  9:14   ` Peter Zijlstra
2018-12-05  5:49     ` Alison Schofield
2018-12-04 15:35   ` Andy Lutomirski
2018-12-05  5:52     ` Alison Schofield
2018-12-06  8:31   ` Sakkinen, Jarkko
2018-12-04  7:39 ` [RFC v2 05/13] x86/mm: Set KeyIDs in encrypted VMAs Alison Schofield
2018-12-06  8:37   ` Sakkinen, Jarkko
2018-12-04  7:39 ` [RFC v2 06/13] mm: Add the encrypt_mprotect() system call Alison Schofield
2018-12-06  8:38   ` Sakkinen, Jarkko
2018-12-04  7:39 ` [RFC v2 07/13] x86/mm: Add helpers for reference counting encrypted VMAs Alison Schofield
2018-12-04  8:58   ` Peter Zijlstra
2018-12-05  5:28     ` Alison Schofield
2018-12-04  7:39 ` [RFC v2 08/13] mm: Use reference counting for " Alison Schofield
2018-12-04  7:39 ` [RFC v2 09/13] mm: Restrict memory encryption to anonymous VMA's Alison Schofield
2018-12-04  9:10   ` Peter Zijlstra
2018-12-05  5:30     ` Alison Schofield
2018-12-05  9:07       ` Peter Zijlstra
2018-12-04  7:39 ` [RFC v2 10/13] keys/mktme: Add the MKTME Key Service type for memory encryption Alison Schofield
2018-12-06  8:51   ` Sakkinen, Jarkko
2018-12-06  8:54     ` Sakkinen, Jarkko
2018-12-06 15:11     ` Dave Hansen
2018-12-06 22:56       ` Sakkinen, Jarkko
2018-12-04  7:39 ` [RFC v2 11/13] keys/mktme: Program memory encryption keys on a system wide basis Alison Schofield
2018-12-04  9:21   ` Peter Zijlstra
2018-12-04  9:50     ` Kirill A. Shutemov
2018-12-05  5:44       ` Alison Schofield
2018-12-05  5:43     ` Alison Schofield
2018-12-05  9:10       ` Peter Zijlstra
2018-12-05 17:26         ` Alison Schofield
2018-12-04  7:39 ` [RFC v2 12/13] keys/mktme: Save MKTME data if kernel cmdline parameter allows Alison Schofield
2018-12-04  9:22   ` Peter Zijlstra
2018-12-07  2:14   ` Huang, Kai
2018-12-07  3:42     ` Alison Schofield
2018-12-07  6:39     ` Jarkko Sakkinen
2018-12-07  6:45       ` Jarkko Sakkinen
2018-12-07 11:47     ` Kirill A. Shutemov
2018-12-04  7:40 ` [RFC v2 13/13] keys/mktme: Support CPU Hotplug for MKTME keys Alison Schofield
2018-12-04  9:28   ` Peter Zijlstra
2018-12-05  5:32     ` Alison Schofield
2018-12-04  9:31   ` Peter Zijlstra
2018-12-05  5:36     ` Alison Schofield
2018-12-04  9:25 ` [RFC v2 00/13] Multi-Key Total Memory Encryption API (MKTME) Peter Zijlstra
2018-12-04  9:46   ` Kirill A. Shutemov
2018-12-05 20:32     ` Sakkinen, Jarkko
2018-12-06 11:22       ` Kirill A. Shutemov
2018-12-06 14:59         ` Dave Hansen
2018-12-07 10:12           ` Huang, Kai
2018-12-06 21:23         ` Sakkinen, Jarkko
2018-12-07 11:54           ` Kirill A. Shutemov
2018-12-04 19:19 ` Andy Lutomirski
2018-12-04 20:00   ` Andy Lutomirski
2018-12-04 20:32     ` Dave Hansen
2018-12-05 22:19   ` Sakkinen, Jarkko
2018-12-07  2:05     ` Huang, Kai
2018-12-07  6:48       ` Jarkko Sakkinen
2018-12-07 11:57     ` Kirill A. Shutemov
2018-12-07 21:59       ` Sakkinen, Jarkko
2018-12-07 23:45         ` Sakkinen, Jarkko
2018-12-07 23:48           ` Andy Lutomirski
2018-12-08  1:33           ` Huang, Kai
2018-12-08  3:53             ` Sakkinen, Jarkko
2018-12-12 15:31           ` Sakkinen, Jarkko
2018-12-12 16:29             ` Andy Lutomirski
2018-12-12 16:43               ` Sakkinen, Jarkko
2018-12-12 23:27                 ` Huang, Kai
2018-12-13  5:49                   ` Sakkinen, Jarkko
2018-12-13  5:52                     ` Sakkinen, Jarkko
2018-12-12 23:24               ` Huang, Kai
2018-12-07 23:35       ` Eric Rannaud
2018-12-05 23:49   ` Dave Hansen
2018-12-06  1:09     ` Andy Lutomirski
2018-12-06  1:25       ` Dan Williams
2018-12-06 15:39       ` Dave Hansen
2018-12-06 19:10         ` Andy Lutomirski
2018-12-06 19:31           ` Dave Hansen
2018-12-07  1:55       ` Huang, Kai
2018-12-07  4:23         ` Dave Hansen
2018-12-07 23:53         ` Andy Lutomirski
2018-12-08  1:11           ` Dave Hansen
2018-12-08  2:07           ` Huang, Kai
2018-12-05 20:30 ` Sakkinen, Jarkko

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=10b900d7461d81433120e614287d6f837cea83ef.camel@intel.com \
    --to=jarkko.sakkinen@intel.com \
    --cc=alison.schofield@intel.com \
    --cc=bp@alien8.de \
    --cc=dan.j.williams@intel.com \
    --cc=dave.hansen@intel.com \
    --cc=dhowells@redhat.com \
    --cc=hpa@zytor.com \
    --cc=jmorris@namei.org \
    --cc=jun.nakajima@intel.com \
    --cc=kai.huang@intel.com \
    --cc=keyrings@vger.kernel.org \
    --cc=kirill.shutemov@linux.intel.com \
    --cc=linux-mm@kvack.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --cc=tglx@linutronix.de \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Security-Module Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-security-module/0 linux-security-module/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-security-module linux-security-module/ https://lore.kernel.org/linux-security-module \
		linux-security-module@vger.kernel.org linux-security-module@archiver.kernel.org
	public-inbox-index linux-security-module

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-security-module


AGPL code for this site: git clone https://public-inbox.org/ public-inbox