[PATCH 00/16] [RFC PATCH] Signed kexec support

[PATCH 00/16] [RFC PATCH] Signed kexec support

[Vivek Goyal]

Hi,

Matthew has been posting patches to lock down kernel either due to
secureboot requirements or because of signed modules with signing
enforced. In kernel lock down mode, kexec will be disabled and that
means kdump will not work either.

These patches sign /sbin/kexec and kernel verifies the signature
and allows loading a kernel if signature verification is successful.
IOW, trust is extended to validly signed user space.

Currently it works only for statically linked applications.

I have generated these patches on top of keys-devel branch of david howell's
linux-fs tree (as I required his system_kerying and trusted keyring patches
to build upon).

git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git keys-devel

These patches assume that matthew's patches to lockdown kernel will go
in some form (secure modules or secure levels or something else). Right
now I have hardcoded few things and will remove those once matthew's
patches are in. 

Some more details about design I have written here.

http://people.redhat.com/vgoyal/kdump-secureboot/kdump-secureboot-summary.txt

Any comments/feedback is appreciated.

Thanks
Vivek

Vivek Goyal (16):
  mm: vm_brk(), align the length to page boundary
  integrity: Add a function to determine digital signature length
  ima: Allow adding more memory locking metadata after digital signature
    v2
  integrity: Allow digital signature verification with a given keyring
    ptr
  integrity: Export a function to retrieve hash algo used in digital
    signature
  ima: export new IMA functions for signature verification
  mm: Define a task flag MMF_VM_LOCKED for memlocked tasks and don't
    allow munlock
  binfmt_elf: Elf executable signature verification
  ima: define functions to appraise memory buffer contents
  keyctl: Introduce a new operation KEYCTL_VERIFY_SIGNATURE
  ptrace: Do not allow ptrace() from unsigned process to signed one
  binfmt_elf: Do not mark process signed if binary has elf interpreter
  kexec: Allow only signed processes to call sys_kexec() in secureboot
    mode
  kexec: Export sysfs attributes for secureboot and secure modules to
    user space
  bootparam: Pass acpi_rsdp pointer in bootparam
  mount: Add a flag to not follow symlink at the end of mount point

 arch/x86/include/uapi/asm/bootparam.h  |   3 +-
 arch/x86/kernel/acpi/boot.c            |   5 +
 drivers/acpi/osl.c                     |  10 ++
 fs/Kconfig.binfmt                      |  10 ++
 fs/binfmt_elf.c                        | 116 ++++++++++++++++++++-
 fs/namespace.c                         |   6 +-
 include/linux/acpi.h                   |   1 +
 include/linux/cred.h                   |   2 +
 include/linux/ima.h                    |  27 +++++
 include/linux/integrity.h              |  25 ++++-
 include/linux/sched.h                  |   2 +
 include/uapi/linux/fs.h                |   1 +
 include/uapi/linux/keyctl.h            |  16 +++
 kernel/cred.c                          |   2 +
 kernel/kexec.c                         |  29 ++++++
 kernel/ksysfs.c                        |  25 +++++
 mm/mlock.c                             |   6 ++
 mm/mmap.c                              |   8 +-
 security/commoncap.c                   |  11 ++
 security/integrity/digsig.c            | 180 +++++++++++++++++++++++++++++++--
 security/integrity/digsig_asymmetric.c |  11 --
 security/integrity/ima/ima_api.c       |  51 ++++++++++
 security/integrity/ima/ima_appraise.c  | 132 +++++++++++++++++++++++-
 security/integrity/integrity.h         |  36 +++++--
 security/keys/compat.c                 |  28 +++++
 security/keys/internal.h               |   2 +
 security/keys/keyctl.c                 |  79 +++++++++++++++
 27 files changed, 788 insertions(+), 36 deletions(-)

-- 
1.8.3.1

[PATCH 01/16] mm: vm_brk(), align the length to page boundary

[Vivek Goyal]

I was writing some code where I was locking all pages of a process
during exec() time by setting VM_LOCKED flag in mm->def_flags. But
that lead to errors because length of mapping is not page aligned.

login: [  174.669002] INFO: rcu_sched self-detected stall on CPU { 2}  (t=60000
jiffies g=2580 c=2579 q=1085)
[  174.669002] Pid: 4894, comm: kexec Not tainted 3.9.0-rc6+ #243
[  174.669002] Call Trace:
[  174.669002]  <IRQ>  [<ffffffff810c413a>] rcu_check_callbacks+0x21a/0x760
[  174.669002]  [<ffffffff810c7c0c>] ? acct_account_cputime+0x1c/0x20
[  174.669002]  [<ffffffff8104fd08>] update_process_times+0x48/0x80
[  174.669002]  [<ffffffff810913dd>] tick_sched_handle+0x3d/0x50
[  174.669002]  [<ffffffff810915e5>] tick_sched_timer+0x45/0x70
[  174.669002]  [<ffffffff81066951>] __run_hrtimer+0x81/0x220
[  174.669002]  [<ffffffff810915a0>] ? tick_nohz_handler+0xa0/0xa0
[  174.669002]  [<ffffffff8108ae0c>] ? ktime_get_update_offsets+0x4c/0xd0
[  174.669002]  [<ffffffff81067297>] hrtimer_interrupt+0xf7/0x250
[  174.669002]  [<ffffffff81886739>] smp_apic_timer_interrupt+0x69/0x99
[  174.669002]  [<ffffffff818859ca>] apic_timer_interrupt+0x6a/0x70
[  174.669002]  <EOI>  [<ffffffff8111e557>] ?  __mlock_vma_pages_range+0x57/0x70
[  174.669002]  [<ffffffff8111e568>] ? __mlock_vma_pages_range+0x68/0x70
[  174.669002]  [<ffffffff8111ea01>] __mm_populate+0x71/0x140
[  174.669002]  [<ffffffff81121b5f>] vm_brk+0x7f/0xa0
[  174.669002]  [<ffffffff81199633>] load_elf_binary+0x1a73/0x1b10
[  174.669002]  [<ffffffff812d25a5>] ? ima_bprm_check+0x55/0x70
[  174.669002]  [<ffffffff8114890a>] search_binary_handler+0x12a/0x3b0
[  174.669002]  [<ffffffff81197bc0>] ? load_elf_library+0x210/0x210
[  174.669002]  [<ffffffff8114aa00>] do_execve_common+0x500/0x5c0
[  174.669002]  [<ffffffff8114aaf7>] do_execve+0x37/0x40
[  174.669002]  [<ffffffff8114ad9d>] sys_execve+0x3d/0x60
[  174.669002]  [<ffffffff81885379>] stub_execve+0x69/0xa0

Thanks to Michel and Hugh Dickens that they identified that __mm_populate()
will loop forever if passed in length is not page aligned. Similar
issues related to mmap() have already been fixed. This patch fixes
vm_brk().

sys_brk() seems to be only other caller of do_brk() and sys_brk()
already aligns lenth to page boundary. So looks like page alignment
logic can be removed from do_brk().

Signed-off-by: Michel Lespinasse <walken@google.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 mm/mmap.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index fbad7b0..3d806be 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2586,10 +2586,6 @@ static unsigned long do_brk(unsigned long addr, unsigned long len)
 	pgoff_t pgoff = addr >> PAGE_SHIFT;
 	int error;
 
-	len = PAGE_ALIGN(len);
-	if (!len)
-		return addr;
-
 	flags = VM_DATA_DEFAULT_FLAGS | VM_ACCOUNT | mm->def_flags;
 
 	error = get_unmapped_area(NULL, addr, len, 0, MAP_FIXED);
@@ -2672,6 +2668,10 @@ unsigned long vm_brk(unsigned long addr, unsigned long len)
 	unsigned long ret;
 	bool populate;
 
+	len = PAGE_ALIGN(len);
+	if (!len)
+		return addr;
+
 	down_write(&mm->mmap_sem);
 	ret = do_brk(addr, len);
 	populate = ((mm->def_flags & VM_LOCKED) != 0);
-- 
1.8.3.1

[PATCH 02/16] integrity: Add a function to determine digital signature length

[Vivek Goyal]

Currently if security.ima has digital signature, there is only one
structure in there. Either version 1 or version 2 of digital signature.
So all the functions assume that whole of the security.ima xattr contains
digital signautre and uses the length accordingly.

But, now I am planning to add more metadata in security.ima xattr. Apart
from signature, I am also planning to add additional info which tells
whether to memlock an executable file during execution or not. In that
case all functions need to know the size of digital signature as rest
of the data might be something else.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 security/integrity/digsig.c            | 17 +++++++++++++++++
 security/integrity/digsig_asymmetric.c | 11 -----------
 security/integrity/ima/ima_appraise.c  |  6 +++---
 security/integrity/integrity.h         | 14 ++++++++++++++
 4 files changed, 34 insertions(+), 14 deletions(-)

diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 0b759e1..160fec7 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -27,6 +27,23 @@ static const char *keyring_name[INTEGRITY_KEYRING_MAX] = {
 	"_ima",
 };
 
+/* Get size of digital signature */
+int integrity_get_digsig_size(char *sig)
+{
+	uint16_t sz;
+
+	if (sig[0] == 1) {
+		sz = *((uint16_t *)(sig + sizeof(struct signature_hdr)));
+		sz = __be16_to_cpu(sz);
+		return sizeof(struct signature_hdr) + 2 + (sz >> 3);
+	} else if (sig[0] == 2 ) {
+		sz = ((struct signature_v2_hdr *)sig)->sig_size;
+		return sizeof(struct signature_v2_hdr) + __be16_to_cpu(sz);
+	}
+
+	return -EBADMSG;
+}
+
 int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
 					const char *digest, int digestlen)
 {
diff --git a/security/integrity/digsig_asymmetric.c b/security/integrity/digsig_asymmetric.c
index b475466..9eae480 100644
--- a/security/integrity/digsig_asymmetric.c
+++ b/security/integrity/digsig_asymmetric.c
@@ -20,17 +20,6 @@
 #include "integrity.h"
 
 /*
- * signature format v2 - for using with asymmetric keys
- */
-struct signature_v2_hdr {
-	uint8_t version;	/* signature format version */
-	uint8_t	hash_algo;	/* Digest algorithm [enum pkey_hash_algo] */
-	uint32_t keyid;		/* IMA key identifier - not X509/PGP specific*/
-	uint16_t sig_size;	/* signature size */
-	uint8_t sig[0];		/* signature payload */
-} __packed;
-
-/*
  * Request an asymmetric key.
  */
 static struct key *request_asymmetric_key(struct key *keyring, uint32_t keyid)
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 2d4beca..a9206d1 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -171,9 +171,9 @@ int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
 	case EVM_IMA_XATTR_DIGSIG:
 		iint->flags |= IMA_DIGSIG;
 		rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
-					     xattr_value->digest, rc - 1,
-					     iint->ima_xattr.digest,
-					     IMA_DIGEST_SIZE);
+				xattr_value->digest,
+				integrity_get_digsig_size(xattr_value->digest),
+				iint->ima_xattr.digest, IMA_DIGEST_SIZE);
 		if (rc == -EOPNOTSUPP) {
 			status = INTEGRITY_UNKNOWN;
 		} else if (rc) {
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index c42fb7a..4246417 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -75,6 +75,17 @@ struct integrity_iint_cache {
 	enum integrity_status evm_status:4;
 };
 
+/*
+ * signature format v2 - for using with asymmetric keys
+ */
+struct signature_v2_hdr {
+	uint8_t version;	/* signature format version */
+	uint8_t	hash_algo;	/* Digest algorithm [enum pkey_hash_algo] */
+	uint32_t keyid;		/* IMA key identifier - not X509/PGP specific*/
+	uint16_t sig_size;	/* signature size */
+	uint8_t sig[0];		/* signature payload */
+} __packed;
+
 /* rbtree tree calls to lookup, insert, delete
  * integrity data associated with an inode.
  */
@@ -90,6 +101,7 @@ struct integrity_iint_cache *integrity_iint_find(struct inode *inode);
 
 int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
 					const char *digest, int digestlen);
+extern int integrity_get_digsig_size(char *sig);
 
 #else
 
@@ -100,6 +112,8 @@ static inline int integrity_digsig_verify(const unsigned int id,
 	return -EOPNOTSUPP;
 }
 
+static inline int integrity_get_digsig_size(char *sig) { return -EOPNOTSUPP; }
+
 #endif /* CONFIG_INTEGRITY_SIGNATURE */
 
 #ifdef CONFIG_INTEGRITY_ASYMMETRIC_KEYS
-- 
1.8.3.1

[PATCH 03/16] ima: Allow adding more memory locking metadata after digital signature v2

[Vivek Goyal]

Now user space tools should be able to append more metadata after digital
signature in security.ima attribute. I intend to add an structure which
tells whether to memory lock a file or not during execution.

This will allow only selected files to be memory locked while signing
all user space. This will make sure that current IMA installations are
not broken as we don't want to lock down every executable in memory.

I intend to add following structure after digital signature.

struct memlock_hdr {
	uint8_t magic_str[8];   /* magic to detect memlock hdr presence */
	uint8_t version;        /* memlock info hdr version */
	uint8_t memlock_file;   /* If set, run executable locked in memory */
} __attribute__ ((packed));

Will use magic string "MEMLOCK" to identify memlock_hdr. This will allow
to append more metadata in future.

version will allow adding more fields to to this structure.

This patch exports a function which tells whether IMA signature tells
to memlock a file or not. This can be used by executable loader to
lock a file.

Unfortunately, adding more metadata is not forward compatible. That
is if we sign a file with new ima/evm tools with memlock_hdr attached,
old kernel version will not recognize that and will consider whole thing
as digital signature and signature verification will fail. So one will
need to operate with new kernel if signing happens with new tools and
some file is signed for memory locking. Not sure how can I add more metadata
in fully forward compatible manner.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 include/linux/ima.h              |  6 ++++++
 security/integrity/ima/ima_api.c | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index 1b7f268..3c40b5e 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -19,6 +19,7 @@ extern int ima_file_check(struct file *file, int mask);
 extern void ima_file_free(struct file *file);
 extern int ima_file_mmap(struct file *file, unsigned long prot);
 extern int ima_module_check(struct file *file);
+extern bool ima_memlock_file(char *sig, unsigned int siglen);
 
 #else
 static inline int ima_bprm_check(struct linux_binprm *bprm)
@@ -46,6 +47,11 @@ static inline int ima_module_check(struct file *file)
 	return 0;
 }
 
+static inline bool ima_memlock_file(char *sig, unsigned int siglen)
+{
+	return false;
+}
+
 #endif /* CONFIG_IMA */
 
 #ifdef CONFIG_IMA_APPRAISE
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 1c03e8f1..0f30cf1 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -254,3 +254,39 @@ const char *ima_d_path(struct path *path, char **pathbuf)
 	}
 	return pathname;
 }
+
+/* Given the signature check whether file should be memlocked or not */
+bool ima_memlock_file(char *sig, unsigned int siglen)
+{
+	struct evm_ima_xattr_data *ima_xattr = (struct evm_ima_xattr_data *)sig;
+	char *sptr;
+	unsigned int dsiglen;
+	uint8_t version;
+
+	dsiglen = integrity_get_digsig_size((char *)ima_xattr->digest);
+
+	if (siglen <= dsiglen)
+		return false;
+
+	/*
+	 * Make sure atleast 9 more bytes are there to scan for magic string
+	 * and version info
+	 */
+	if (siglen <= dsiglen + 9)
+		return false;
+
+	sptr = (char *)ima_xattr->digest + dsiglen;
+
+	if (strncmp(sptr, "MEMLOCK", 7))
+		return false;
+
+	sptr += 8;
+	version = sptr[0];
+	if (version != 1)
+		return false;
+	sptr++;
+	if (sptr[0] != 1)
+		return false;
+
+	return true;
+}
-- 
1.8.3.1

[PATCH 04/16] integrity: Allow digital signature verification with a given keyring ptr

[Vivek Goyal]

Currently digital signature verification code assumes that it can be
used only with 3 keyrings. IMA, EVM and MODULE keyring. Provide another
variant where one can pass in a pointer to keyring (struct key *), and
integrity code can try to find key in that keyring and verify signature.

This will be useful at two places.

- elf binary loader can use system keyring and call into integrity
  subsystem for signature verification.
- In later patches I am extending keyctl() to allow signature of
  a user buffer against specified keyring. That logic can make use
  of this code too.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 security/integrity/digsig.c    | 26 ++++++++++++++++----------
 security/integrity/integrity.h |  9 +++++++++
 2 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 160fec7..f1259bd 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -44,6 +44,20 @@ int integrity_get_digsig_size(char *sig)
 	return -EBADMSG;
 }
 
+int integrity_digsig_verify_keyring(struct key *keyring, const char *sig,
+			int siglen, const char *digest, int digestlen)
+{
+	switch (sig[0]) {
+	case 1:
+		return digsig_verify(keyring, sig, siglen,
+				     digest, digestlen);
+	case 2:
+		return asymmetric_verify(keyring, sig, siglen,
+					 digest, digestlen);
+	}
+	return -EOPNOTSUPP;
+}
+
 int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
 					const char *digest, int digestlen)
 {
@@ -61,14 +75,6 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
 		}
 	}
 
-	switch (sig[0]) {
-	case 1:
-		return digsig_verify(keyring[id], sig, siglen,
-				     digest, digestlen);
-	case 2:
-		return asymmetric_verify(keyring[id], sig, siglen,
-					 digest, digestlen);
-	}
-
-	return -EOPNOTSUPP;
+	return integrity_digsig_verify_keyring(keyring[id], sig, siglen,
+						digest, digestlen);
 }
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 4246417..130eb3b 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -101,6 +101,8 @@ struct integrity_iint_cache *integrity_iint_find(struct inode *inode);
 
 int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
 					const char *digest, int digestlen);
+int integrity_digsig_verify_keyring(struct key *keyring, const char *sig,
+			int siglen, const char *digest, int digestlen);
 extern int integrity_get_digsig_size(char *sig);
 
 #else
@@ -112,6 +114,13 @@ static inline int integrity_digsig_verify(const unsigned int id,
 	return -EOPNOTSUPP;
 }
 
+static inline int integrity_digsig_verify_keyring(struct key *keyring,
+			const char *sig, int siglen, const char *digest,
+			int digestlen)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline int integrity_get_digsig_size(char *sig) { return -EOPNOTSUPP; }
 
 #endif /* CONFIG_INTEGRITY_SIGNATURE */
-- 
1.8.3.1

Re: [PATCH 04/16] integrity: Allow digital signature verification with a given keyring ptr

[Mimi Zohar]

On Tue, 2013-09-10 at 17:44 -0400, Vivek Goyal wrote:
> Currently digital signature verification code assumes that it can be
> used only with 3 keyrings. IMA, EVM and MODULE keyring. Provide another
> variant where one can pass in a pointer to keyring (struct key *), and
> integrity code can try to find key in that keyring and verify signature.
> 
> This will be useful at two places.
> 
> - elf binary loader can use system keyring and call into integrity
>   subsystem for signature verification.
> - In later patches I am extending keyctl() to allow signature of
>   a user buffer against specified keyring. That logic can make use
>   of this code too.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

My original thought was to use the system keyring, in lieu of having the
multiple keyrings.  Unfortunately, the scope of a key's usage needs to
be limited, which can not be done safely with a single keyring.

Mimi

[PATCH 05/16] integrity: Export a function to retrieve hash algo used in digital signature

[Vivek Goyal]

A function to retrieve hash algo used in digital signature.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 security/integrity/digsig.c    | 26 ++++++++++++++++++++++++++
 security/integrity/integrity.h |  7 +++++++
 2 files changed, 33 insertions(+)

diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index f1259bd..153cff4 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -16,6 +16,8 @@
 #include <linux/rbtree.h>
 #include <linux/key-type.h>
 #include <linux/digsig.h>
+#include <crypto/hash.h>
+#include <crypto/public_key.h>
 
 #include "integrity.h"
 
@@ -27,6 +29,30 @@ static const char *keyring_name[INTEGRITY_KEYRING_MAX] = {
 	"_ima",
 };
 
+int integrity_digsig_get_hash_algo(char *sig)
+{
+	uint8_t hash_algo;
+
+	if (sig[0] == 1) {
+		hash_algo = ((struct signature_hdr *)sig)->hash;
+		switch (hash_algo) {
+		case 0:
+			return PKEY_HASH_SHA1;
+		case 1:
+			return PKEY_HASH_SHA256;
+		default:
+			return -ENOPKG;
+		}
+	} else if (sig[0] == 2 ) {
+		hash_algo = ((struct signature_v2_hdr *)sig)->hash_algo;
+		if (hash_algo >= PKEY_HASH__LAST)
+			return -ENOPKG;
+		return hash_algo;
+	}
+
+	return -EBADMSG;
+}
+
 /* Get size of digital signature */
 int integrity_get_digsig_size(char *sig)
 {
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 130eb3b..284bb8d 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -15,6 +15,7 @@
 #include <linux/integrity.h>
 #include <crypto/sha.h>
 #include <linux/key.h>
+#include <crypto/public_key.h>
 
 /* iint action cache flags */
 #define IMA_MEASURE		0x00000001
@@ -105,8 +106,14 @@ int integrity_digsig_verify_keyring(struct key *keyring, const char *sig,
 			int siglen, const char *digest, int digestlen);
 extern int integrity_get_digsig_size(char *sig);
 
+extern int integrity_digsig_get_hash_algo(char *sig);
 #else
 
+static inline int integrity_digsig_get_hash_algo(char *sig)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline int integrity_digsig_verify(const unsigned int id,
 					  const char *sig, int siglen,
 					  const char *digest, int digestlen)
-- 
1.8.3.1

[PATCH 06/16] ima: export new IMA functions for signature verification

[Vivek Goyal]

Export IMA functions so that other subsystems can use IMA for file
signature verification.

Why introduce these functions and not use security hooks? Now callers
have new requirements which are not satisfied by the hooks.

a. Caller need to know whether signature verification actually took
   place or not. Based on that caller will set some flags in task.
   security hooks don't give this info. If IMA is disabled or right
   policy is not configured, security hook will still return success.

b. Caller needs to know the signature type. They might just consider
   digital signatures and not rely on hash or hmac type of signatures.
   security hooks don't have any provision where they can enforce what
   kind of signatures need to be present on the file in question.

c. IMA does the caching of appraisal results and does not detect direct
   writes to disk. So it can happen that after initial bprm check one
   can directly write to file on disk and call IMA hook for signature
   verification again after loading executable in memory and verification
   passes as bprm check had passed. But file contents are different as
   a direct write to disk was done and IMA did not detect this file change.

In some previous discussions it was suggested that define a new hook
and always force appraise the file on that hook to avoid caching
related issues (c). Export the type of signature used for appraisal and
allow caller to query it to take care of issue a and b.

I have not taken that approach yet for following reasons.

- Forcing re-appraisal on a particular hook is very odd. What's special
  about that hook that we hard code it.

- Querying type of signature used for appraisal after hook execution
  if racy as same file might have been re-appraised by the time we
  queried the type of signature used for appraisal. I guess this should
  be fixable if done carefully.

- I anyway need to export more functions later to appraise a memory
  buffer (and not file) so for the time being I continued with the approach
  of exporting functions for file appraisal.

I this approach is not acceptable, I can try implementing the other
one. Personally I find this one a cleaner approach.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 include/linux/ima.h                   |  21 ++++++
 include/linux/integrity.h             |   6 ++
 security/integrity/ima/ima_api.c      |  15 ++++
 security/integrity/ima/ima_appraise.c | 126 ++++++++++++++++++++++++++++++++++
 security/integrity/integrity.h        |   6 --
 5 files changed, 168 insertions(+), 6 deletions(-)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index 3c40b5e..60c75bf 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -11,6 +11,8 @@
 #define _LINUX_IMA_H
 
 #include <linux/fs.h>
+#include <linux/integrity.h>
+#include <linux/key.h>
 struct linux_binprm;
 
 #ifdef CONFIG_IMA
@@ -20,6 +22,8 @@ extern void ima_file_free(struct file *file);
 extern int ima_file_mmap(struct file *file, unsigned long prot);
 extern int ima_module_check(struct file *file);
 extern bool ima_memlock_file(char *sig, unsigned int siglen);
+extern int ima_file_signature_alloc(struct file *file, char **sig);
+extern int ima_signature_type(char *sig);
 
 #else
 static inline int ima_bprm_check(struct linux_binprm *bprm)
@@ -52,6 +56,16 @@ static inline bool ima_memlock_file(char *sig, unsigned int siglen)
 	return false;
 }
 
+static inline int ima_file_signature_alloc(struct file *file, char **sig)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline int ima_signature_type(char *sig)
+{
+	return -EOPNOTSUPP;
+}
+
 #endif /* CONFIG_IMA */
 
 #ifdef CONFIG_IMA_APPRAISE
@@ -59,6 +73,7 @@ extern void ima_inode_post_setattr(struct dentry *dentry);
 extern int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
 		       const void *xattr_value, size_t xattr_value_len);
 extern int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name);
+extern int ima_appraise_file_digsig(struct key *keyring, struct file *file, char *sig, unsigned int siglen);
 #else
 static inline void ima_inode_post_setattr(struct dentry *dentry)
 {
@@ -78,5 +93,11 @@ static inline int ima_inode_removexattr(struct dentry *dentry,
 {
 	return 0;
 }
+
+static inline int ima_appraise_file_digsig(struct key *keyring, struct file *file, char *sig, unsigned int siglen)
+{
+	return -EOPNOTSUPP;
+}
+
 #endif /* CONFIG_IMA_APPRAISE */
 #endif /* _LINUX_IMA_H */
diff --git a/include/linux/integrity.h b/include/linux/integrity.h
index 83222ce..b26bd7d 100644
--- a/include/linux/integrity.h
+++ b/include/linux/integrity.h
@@ -20,6 +20,12 @@ enum integrity_status {
 	INTEGRITY_UNKNOWN,
 };
 
+enum evm_ima_xattr_type {
+	IMA_XATTR_DIGEST = 0x01,
+	EVM_XATTR_HMAC,
+	EVM_IMA_XATTR_DIGSIG,
+};
+
 /* List of EVM protected security xattrs */
 #ifdef CONFIG_INTEGRITY
 extern struct integrity_iint_cache *integrity_inode_get(struct inode *inode);
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 0f30cf1..95c9412 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -290,3 +290,18 @@ bool ima_memlock_file(char *sig, unsigned int siglen)
 
 	return true;
 }
+
+/*
+ * Get ima signature.
+ */
+int ima_file_signature_alloc(struct file *file, char **sig)
+{
+	struct dentry *dentry = file->f_dentry;
+
+	return vfs_getxattr_alloc(dentry, XATTR_NAME_IMA, sig, 0, GFP_NOFS);
+}
+
+int ima_signature_type(char *sig)
+{
+	return ((struct evm_ima_xattr_data *)sig)->type;
+}
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index a9206d1..f8d147a 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -15,6 +15,8 @@
 #include <linux/magic.h>
 #include <linux/ima.h>
 #include <linux/evm.h>
+#include <crypto/public_key.h>
+#include <crypto/hash.h>
 
 #include "ima.h"
 
@@ -107,6 +109,130 @@ static void ima_cache_flags(struct integrity_iint_cache *iint, int func)
 	}
 }
 
+static int ima_get_file_hash(struct file *file, char **_digest,
+				unsigned int *digest_len,
+				enum pkey_hash_algo hash_algo)
+{
+	loff_t i_size, offset = 0;
+	char *rbuf;
+	int ret, read = 0;
+	struct crypto_shash *tfm;
+	size_t desc_size, digest_size;
+	struct shash_desc *desc;
+	char *digest = NULL;
+
+	rbuf = kzalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!rbuf)
+		return -ENOMEM;
+
+	tfm = crypto_alloc_shash(pkey_hash_algo_name[hash_algo], 0, 0);
+	if (IS_ERR(tfm)) {
+		ret = PTR_ERR(tfm);
+		goto out;
+	}
+
+	desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
+	desc = kzalloc(desc_size, GFP_KERNEL);
+	if (!desc) {
+		ret = -ENOMEM;
+		goto out_free_tfm;
+	}
+
+	desc->tfm   = tfm;
+	desc->flags = 0;
+
+	ret = crypto_shash_init(desc);
+	if (ret < 0)
+		goto out_free_desc;
+
+	digest_size = crypto_shash_digestsize(tfm);
+	digest = kzalloc(digest_size, GFP_KERNEL);
+	if (!digest) {
+		ret = -ENOMEM;
+		goto out_free_desc;
+	}
+
+	if (!(file->f_mode & FMODE_READ)) {
+		file->f_mode |= FMODE_READ;
+		read = 1;
+	}
+	i_size = i_size_read(file_inode(file));
+	while (offset < i_size) {
+		int rbuf_len;
+
+		rbuf_len = kernel_read(file, offset, rbuf, PAGE_SIZE);
+		if (rbuf_len < 0) {
+			ret = rbuf_len;
+			break;
+		}
+		if (rbuf_len == 0)
+			break;
+		offset += rbuf_len;
+
+		ret = crypto_shash_update(desc, rbuf, rbuf_len);
+		if (ret)
+			break;
+	}
+
+	if (!ret) {
+		ret = crypto_shash_final(desc, digest);
+		*_digest = digest;
+		*digest_len = digest_size;
+		digest = NULL;
+	}
+
+	if (read)
+		file->f_mode &= ~FMODE_READ;
+
+out_free_desc:
+	kfree(desc);
+out_free_tfm:
+	kfree(tfm);
+out:
+	if (digest)
+		kfree(digest);
+	kfree(rbuf);
+	return ret;
+}
+
+/*
+ * Appraise a file with a given digital signature
+ * keyring: keyring to use for appraisal
+ * sig: signature
+ * siglen: length of signature
+ *
+ * Returns 0 on successful appraisal, error otherwise.
+ */
+int ima_appraise_file_digsig(struct key *keyring, struct file *file, char *sig,
+				unsigned int siglen)
+{
+	struct evm_ima_xattr_data *xattr_value;
+	int ret = 0;
+	char *digest = NULL;
+	enum pkey_hash_algo hash_algo;
+	unsigned int digest_len = 0;
+
+	xattr_value = (struct evm_ima_xattr_data*) sig;
+
+	if (xattr_value->type != EVM_IMA_XATTR_DIGSIG)
+		return -EBADMSG;
+
+	ret = integrity_digsig_get_hash_algo(xattr_value->digest);
+	if (ret < 0)
+		return ret;
+
+	hash_algo = (enum pkey_hash_algo)ret;
+	ret = ima_get_file_hash(file, &digest, &digest_len, hash_algo);
+	if (ret)
+		return ret;
+
+	ret = integrity_digsig_verify_keyring(keyring, xattr_value->digest,
+				integrity_get_digsig_size(xattr_value->digest),
+				digest, digest_len);
+	kfree(digest);
+	return ret;
+}
+
 /*
  * ima_appraise_measurement - appraise file measurement
  *
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 284bb8d..f7db589 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -51,12 +51,6 @@
 #define IMA_APPRAISED_SUBMASK	(IMA_FILE_APPRAISED | IMA_MMAP_APPRAISED | \
 				 IMA_BPRM_APPRAISED | IMA_MODULE_APPRAISED)
 
-enum evm_ima_xattr_type {
-	IMA_XATTR_DIGEST = 0x01,
-	EVM_XATTR_HMAC,
-	EVM_IMA_XATTR_DIGSIG,
-};
-
 struct evm_ima_xattr_data {
 	u8 type;
 	u8 digest[SHA1_DIGEST_SIZE];
-- 
1.8.3.1

[PATCH 07/16] mm: Define a task flag MMF_VM_LOCKED for memlocked tasks and don't allow munlock

[Vivek Goyal]

Define a flag to mark tasks which are running locked in memory. And use it
to deny munlock/munlockall operation.

We want to lock down a task in memory so that it is not swapped out.
Otherwise it is susceptible to attack on swap disk and then modified
code/data will be swapped back.

I am not sure what exactly a memory locked task should mean. Should
we lock down selected vmas of a task or all the vmas of a task.

In this patch series, I am locking down everything by setting VM_LOCKED
bit in mm->def_flags at exec() time. A task who is running memlocked,
can not unlock any of the vmas. munlock() call will fail. So one need
to be careful while signing a task and designating it to run as memlocked.

If feedback is to lock down only selected vmas, then we can probably
define a flag VM_LOCKED_PERM per vma which signifies that a particuar
vma is permanently locked by kernel and can not be unlocked if user calls
munlock[all]. This will allow to not memlock whole of the address space
of process.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 include/linux/sched.h | 2 ++
 mm/mlock.c            | 6 ++++++
 2 files changed, 8 insertions(+)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 50d04b9..0f7e8c0 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -367,6 +367,8 @@ extern int get_dumpable(struct mm_struct *mm);
 
 #define MMF_HAS_UPROBES		19	/* has uprobes */
 #define MMF_RECALC_UPROBES	20	/* MMF_HAS_UPROBES can be wrong */
+#define MMF_VM_LOCKED		21	/* Task needs to run with some VMAs
+					   locked permanently */
 
 #define MMF_INIT_MASK		(MMF_DUMPABLE_MASK | MMF_DUMP_FILTER_MASK)
 
diff --git a/mm/mlock.c b/mm/mlock.c
index 79b7cf7..50ab11a 100644
--- a/mm/mlock.c
+++ b/mm/mlock.c
@@ -478,6 +478,9 @@ SYSCALL_DEFINE2(munlock, unsigned long, start, size_t, len)
 {
 	int ret;
 
+	if (test_bit(MMF_VM_LOCKED, &current->mm->flags))
+		return -EINVAL;
+
 	down_write(&current->mm->mmap_sem);
 	len = PAGE_ALIGN(len + (start & ~PAGE_MASK));
 	start &= PAGE_MASK;
@@ -546,6 +549,9 @@ SYSCALL_DEFINE0(munlockall)
 {
 	int ret;
 
+	if (test_bit(MMF_VM_LOCKED, &current->mm->flags))
+		return -EINVAL;
+
 	down_write(&current->mm->mmap_sem);
 	ret = do_mlockall(0);
 	up_write(&current->mm->mmap_sem);
-- 
1.8.3.1

[PATCH 08/16] binfmt_elf: Elf executable signature verification

[Vivek Goyal]

Do elf executable signature verification (if one is present). If signature
is present, it should be valid. Validly signed executables are locked in
memory and a flag cred->proc_signed gets set to signify this process
executable contents are signed.

If file is unsigned, it can execute but it does not have the cred->proc_signed
set.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/Kconfig.binfmt    | 10 +++++++++
 fs/binfmt_elf.c      | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/cred.h |  2 ++
 kernel/cred.c        |  2 ++
 4 files changed, 76 insertions(+), 1 deletion(-)

diff --git a/fs/Kconfig.binfmt b/fs/Kconfig.binfmt
index 370b24c..25ae6d3 100644
--- a/fs/Kconfig.binfmt
+++ b/fs/Kconfig.binfmt
@@ -23,6 +23,16 @@ config BINFMT_ELF
 	  ld.so (check the file <file:Documentation/Changes> for location and
 	  latest version).
 
+config BINFMT_ELF_SIG
+	bool "ELF binary signature verification"
+	depends on BINFMT_ELF
+	depends on INTEGRITY_ASYMMETRIC_KEYS
+	depends on IMA_APPRAISE
+	depends on SYSTEM_TRUSTED_KEYRING
+	default n
+	---help---
+	  Check ELF binary signature verfication.
+
 config COMPAT_BINFMT_ELF
 	bool
 	depends on COMPAT && BINFMT_ELF
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 100edcc..22a8272 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -34,6 +34,8 @@
 #include <linux/utsname.h>
 #include <linux/coredump.h>
 #include <linux/sched.h>
+#include <linux/ima.h>
+#include <keys/system_keyring.h>
 #include <asm/uaccess.h>
 #include <asm/param.h>
 #include <asm/page.h>
@@ -584,6 +586,11 @@ static int load_elf_binary(struct linux_binprm *bprm)
 	int executable_stack = EXSTACK_DEFAULT;
 	unsigned long def_flags = 0;
 	struct pt_regs *regs = current_pt_regs();
+	char *signature = NULL;
+#ifdef CONFIG_BINFMT_ELF_SIG
+	unsigned int siglen = 0;
+	bool mlock_mappings = false;
+#endif
 	struct {
 		struct elfhdr elf_ex;
 		struct elfhdr interp_elf_ex;
@@ -725,6 +732,43 @@ static int load_elf_binary(struct linux_binprm *bprm)
 	/* OK, This is the point of no return */
 	current->mm->def_flags = def_flags;
 
+#ifdef CONFIG_BINFMT_ELF_SIG
+	/*
+	 * If executable is digitally signed and ima memlock info present,
+	 * Lock down in memory
+	 */
+	retval = ima_file_signature_alloc(bprm->file, &signature);
+
+	/*
+	 * If there is an error getting signature, bail out. Having
+	 * no signature is fine though.
+	 */
+	if (retval < 0 && retval != -ENODATA && retval != -EOPNOTSUPP)
+		goto out_free_dentry;
+
+	if (signature != NULL) {
+		siglen = retval;
+		retval = ima_signature_type(signature);
+		if (retval == EVM_IMA_XATTR_DIGSIG &&
+		    ima_memlock_file(signature, siglen)) {
+			/*
+			 * Verify signature before locking down file. We don't
+			 * want to memlock executables with fake signatures
+			 */
+			retval = ima_appraise_file_digsig(
+					system_trusted_keyring,
+					bprm->file, signature, siglen);
+			if (retval) {
+				send_sig(SIGKILL, current, 0);
+				goto out_free_dentry;
+			}
+
+			mlock_mappings = true;
+			current->mm->def_flags |= VM_LOCKED;
+			set_bit(MMF_VM_LOCKED, &current->mm->flags);
+		}
+	}
+#endif
 	/* Do this immediately, since STACK_TOP as used in setup_arg_pages
 	   may depend on the personality.  */
 	SET_PERSONALITY(loc->elf_ex);
@@ -895,6 +939,23 @@ static int load_elf_binary(struct linux_binprm *bprm)
 		goto out_free_dentry;
 	}
 
+#ifdef CONFIG_BINFMT_ELF_SIG
+	if (mlock_mappings) {
+		/*
+		 * File locked down in memory. Now it is safe against any
+		 * modifications on disk by raw disk writes. Verify signature.
+		 */
+		retval = ima_appraise_file_digsig(system_trusted_keyring,
+					bprm->file, signature, siglen);
+		if (retval) {
+			send_sig(SIGKILL, current, 0);
+			goto out_free_dentry;
+		}
+		/* Signature verification successful */
+		bprm->cred->proc_signed = true;
+	}
+#endif
+
 	if (elf_interpreter) {
 		unsigned long interp_map_addr = 0;
 
@@ -988,11 +1049,11 @@ static int load_elf_binary(struct linux_binprm *bprm)
 	 */
 	ELF_PLAT_INIT(regs, reloc_func_desc);
 #endif
-
 	start_thread(regs, elf_entry, bprm->p);
 	retval = 0;
 out:
 	kfree(loc);
+	kfree(signature);
 out_ret:
 	return retval;
 
diff --git a/include/linux/cred.h b/include/linux/cred.h
index 04421e8..1f5f418 100644
--- a/include/linux/cred.h
+++ b/include/linux/cred.h
@@ -136,6 +136,8 @@ struct cred {
 	struct user_namespace *user_ns; /* user_ns the caps and keyrings are relative to. */
 	struct group_info *group_info;	/* supplementary groups for euid/fsgid */
 	struct rcu_head	rcu;		/* RCU deletion hook */
+	bool	proc_signed;		/* Executable signature have been
+					 * verified post load */
 };
 
 extern void __put_cred(struct cred *);
diff --git a/kernel/cred.c b/kernel/cred.c
index e0573a4..589f1fa 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -299,6 +299,8 @@ struct cred *prepare_exec_creds(void)
 	new->process_keyring = NULL;
 #endif
 
+	/* proc_signed status will be evaluated again from executable file */
+	new->proc_signed = false;
 	return new;
 }
 
-- 
1.8.3.1

[PATCH 09/16] ima: define functions to appraise memory buffer contents

[Vivek Goyal]

I need to provide user space with facility that it can call into kernel
for signature verification of a file. Trying to rely on file based appraisal
has the downside that somebody might write to file after appraisal and it
is racy.

Alternative is that one can copy file contents in memory buffer and pass
buffer and signature to kernel and ask whether signatures are valid.

Hence introduce an IMA function to be able verify signature of meory
buffer. This will in turn be called the keyctl() which will provide
this facility to user space.

This can be used by kexec to verify signature of bzImage being loaded.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 include/linux/integrity.h   |  19 +++++++-
 security/integrity/digsig.c | 115 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 133 insertions(+), 1 deletion(-)

diff --git a/include/linux/integrity.h b/include/linux/integrity.h
index b26bd7d..36c9a6d 100644
--- a/include/linux/integrity.h
+++ b/include/linux/integrity.h
@@ -11,6 +11,7 @@
 #define _LINUX_INTEGRITY_H
 
 #include <linux/fs.h>
+#include <linux/key.h>
 
 enum integrity_status {
 	INTEGRITY_PASS = 0,
@@ -30,7 +31,6 @@ enum evm_ima_xattr_type {
 #ifdef CONFIG_INTEGRITY
 extern struct integrity_iint_cache *integrity_inode_get(struct inode *inode);
 extern void integrity_inode_free(struct inode *inode);
-
 #else
 static inline struct integrity_iint_cache *
 				integrity_inode_get(struct inode *inode)
@@ -42,5 +42,22 @@ static inline void integrity_inode_free(struct inode *inode)
 {
 	return;
 }
+
 #endif /* CONFIG_INTEGRITY */
+
+#ifdef CONFIG_INTEGRITY_SIGNATURE
+extern int integrity_verify_user_buffer_digsig(struct key *keyring,
+				const char __user *data,
+				unsigned long data_len,
+				char *sig, unsigned int siglen);
+#else
+static inline int integrity_verify_user_buffer_digsig(struct key *keyring,
+				const char __user *data,
+				unsigned long data_len,
+				char *sig, unsigned int siglen)
+{
+	return -EOPNOTSUPP;
+}
+#endif /* CONFIG_INTEGRITY_SIGNATURE */
+
 #endif /* _LINUX_INTEGRITY_H */
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 153cff4..166a7dd 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -16,6 +16,7 @@
 #include <linux/rbtree.h>
 #include <linux/key-type.h>
 #include <linux/digsig.h>
+#include <linux/sched.h>
 #include <crypto/hash.h>
 #include <crypto/public_key.h>
 
@@ -104,3 +105,117 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
 	return integrity_digsig_verify_keyring(keyring[id], sig, siglen,
 						digest, digestlen);
 }
+
+static int integrity_calc_user_buffer_hash(enum pkey_hash_algo hash_algo,
+					const char __user *data,
+					unsigned long data_len, char **_digest,
+					unsigned int *digest_len)
+{
+	char *buffer, *digest;
+	unsigned long len;
+	struct crypto_shash *tfm;
+	size_t desc_size, digest_size;
+	struct shash_desc *desc;
+	int ret;
+
+	buffer = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;
+
+	/* TODO: allow different kind of hash */
+	tfm = crypto_alloc_shash(pkey_hash_algo_name[hash_algo], 0, 0);
+	if (IS_ERR(tfm)) {
+		ret = PTR_ERR(tfm);
+		goto out;
+	}
+	desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
+	desc = kzalloc(desc_size, GFP_KERNEL);
+	if (!desc) {
+		ret = -ENOMEM;
+		goto out_free_tfm;
+	}
+
+	desc->tfm   = tfm;
+	desc->flags = 0;
+
+	ret = crypto_shash_init(desc);
+	if (ret < 0)
+		goto out_free_desc;
+
+	digest_size = crypto_shash_digestsize(tfm);
+	digest = kzalloc(digest_size, GFP_KERNEL);
+	if (!digest) {
+		ret = -ENOMEM;
+		goto out_free_desc;
+	}
+
+	do {
+		len = min(data_len, PAGE_SIZE - ((size_t)data & ~PAGE_MASK));
+		ret = -EFAULT;
+		if (copy_from_user(buffer, data, len) != 0)
+			goto out_free_digest;
+
+		ret = crypto_shash_update(desc, buffer, len);
+                if (ret)
+                        break;
+
+		data_len -= len;
+		data += len;
+
+		if (fatal_signal_pending(current)) {
+			ret = -EINTR;
+			break;
+		}
+	} while (data_len > 0);
+
+	if (!ret) {
+		ret = crypto_shash_final(desc, digest);
+		*_digest = digest;
+		*digest_len = digest_size;
+		digest = NULL;
+	}
+
+out_free_digest:
+	if (digest)
+		kfree(digest);
+out_free_desc:
+	kfree(desc);
+out_free_tfm:
+	kfree(tfm);
+out:
+	kfree(buffer);
+	return ret;
+}
+
+/*
+ * Appraise a user buffer with a given digital signature
+ * keyring: keyring to use for appraisal
+ * sig: signature
+ * siglen: length of signature
+ *
+ * Returns 0 on successful appraisal, error otherwise.
+ */
+int integrity_verify_user_buffer_digsig(struct key *keyring,
+				const char __user *data,
+				unsigned long data_len,
+				char *sig, unsigned int siglen)
+{
+	int ret = 0;
+	enum pkey_hash_algo hash_algo;
+	char *digest = NULL;
+	unsigned int digest_len = 0;
+
+	hash_algo = integrity_digsig_get_hash_algo(sig);
+	if (hash_algo < 0)
+		return hash_algo;
+
+	ret = integrity_calc_user_buffer_hash(hash_algo, data, data_len,
+						&digest, &digest_len);
+	if (ret)
+		return ret;
+
+	ret = integrity_digsig_verify_keyring(keyring, sig, siglen, digest,
+					digest_len);
+	kfree(digest);
+	return ret;
+}
-- 
1.8.3.1

[PATCH 10/16] keyctl: Introduce a new operation KEYCTL_VERIFY_SIGNATURE

[Vivek Goyal]

This is based on a patch david howells sent me. I have modified that
patch to meet my needs.

Extend kecytl() to add an option to verify signature of a user buffer.
One needs to pass in the signature type also so that respective handler
can be called. Currently I have defined a new signature type
KEYCTL_SIG_TYPE_IMA_DIGSIG, which sinifies signatures generated by IMA
subsystem.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 include/uapi/linux/keyctl.h | 16 +++++++++
 security/keys/compat.c      | 28 ++++++++++++++++
 security/keys/internal.h    |  2 ++
 security/keys/keyctl.c      | 79 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 125 insertions(+)

diff --git a/include/uapi/linux/keyctl.h b/include/uapi/linux/keyctl.h
index 840cb99..d7c7471 100644
--- a/include/uapi/linux/keyctl.h
+++ b/include/uapi/linux/keyctl.h
@@ -12,6 +12,17 @@
 #ifndef _LINUX_KEYCTL_H
 #define _LINUX_KEYCTL_H
 
+/* Data required to verify signature of a user buffer */
+struct keyctl_sig_data {
+	void *data;
+	size_t datalen;
+	void *sig;
+	size_t siglen;
+	unsigned long sig_type;
+	unsigned long keyring_id;
+	unsigned long flags;
+};
+
 /* special process keyring shortcut IDs */
 #define KEY_SPEC_THREAD_KEYRING		-1	/* - key ID for thread-specific keyring */
 #define KEY_SPEC_PROCESS_KEYRING	-2	/* - key ID for process-specific keyring */
@@ -57,5 +68,10 @@
 #define KEYCTL_INSTANTIATE_IOV		20	/* instantiate a partially constructed key */
 #define KEYCTL_INVALIDATE		21	/* invalidate a key */
 #define KEYCTL_GET_PERSISTENT		22	/* get a user's persistent keyring */
+#define KEYCTL_VERIFY_SIGNATURE		23	/* use a key to verify a signature */
+
+/* Type of signatures */
+#define KEYCTL_SIG_TYPE_UNKNOWN			0
+#define KEYCTL_SIG_TYPE_INTEGRITY_DIGSIG	1 	/* Digital Signature generated by integrity subsystem utilities */
 
 #endif /*  _LINUX_KEYCTL_H */
diff --git a/security/keys/compat.c b/security/keys/compat.c
index bbd32c7..3af2cf2 100644
--- a/security/keys/compat.c
+++ b/security/keys/compat.c
@@ -15,6 +15,31 @@
 #include <linux/slab.h>
 #include "internal.h"
 
+struct compat_keyctl_sig_data {
+	compat_uptr_t data;
+	compat_size_t datalen;
+	compat_uptr_t sig;
+	compat_size_t siglen;
+	compat_ulong_t sig_type;
+	compat_ulong_t keyring_id;
+	compat_ulong_t flags;
+};
+
+static long compat_keyctl_verify_signature(const void __user *_sig_data)
+{
+	struct compat_keyctl_sig_data csig_data;
+	int result;
+
+	result = copy_from_user(&csig_data, _sig_data, sizeof(csig_data));
+	if (result)
+		return -EFAULT;
+
+	return __keyctl_verify_signature(csig_data.keyring_id,
+			compat_ptr(csig_data.data), csig_data.datalen,
+			compat_ptr(csig_data.sig), csig_data.siglen,
+			csig_data.sig_type, csig_data.flags);
+}
+
 /*
  * Instantiate a key with the specified compatibility multipart payload and
  * link the key into the destination keyring if one is given.
@@ -141,6 +166,9 @@ asmlinkage long compat_sys_keyctl(u32 option,
 	case KEYCTL_GET_PERSISTENT:
 		return keyctl_get_persistent(arg2, arg3);
 
+	case KEYCTL_VERIFY_SIGNATURE:
+		return compat_keyctl_verify_signature(compat_ptr(arg2));
+
 	default:
 		return -EOPNOTSUPP;
 	}
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 80b2aac..f15acee 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -255,6 +255,8 @@ extern long keyctl_invalidate_key(key_serial_t);
 extern long keyctl_instantiate_key_common(key_serial_t,
 					  const struct iovec *,
 					  unsigned, size_t, key_serial_t);
+extern long keyctl_verify_signature(const void __user *_sig_data);
+extern long __keyctl_verify_signature(key_serial_t keyring_id, void __user *_data, size_t dlen, void __user *_sig, size_t siglen, unsigned long sig_type, unsigned long flags);
 #ifdef CONFIG_PERSISTENT_KEYRINGS
 extern long keyctl_get_persistent(uid_t, key_serial_t);
 extern unsigned persistent_keyring_expiry;
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index cee72ce..84b7c3d 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -23,6 +23,8 @@
 #include <linux/vmalloc.h>
 #include <linux/security.h>
 #include <linux/uio.h>
+#include <linux/ima.h>
+#include <keys/system_keyring.h>
 #include <asm/uaccess.h>
 #include "internal.h"
 
@@ -1564,6 +1566,80 @@ error_keyring:
 	return ret;
 }
 
+long __keyctl_verify_signature(key_serial_t keyring_id, void __user *_data,
+				size_t dlen, void __user *_sig, size_t siglen,
+				unsigned long sig_type, unsigned long flags)
+{
+	void *sig;
+	long ret;
+	key_ref_t keyring_ref;
+
+	pr_devel("-->keyctl_verify_signature(,%zu,,%zu,%lu)\n",
+			dlen, siglen, sig_type);
+
+	if (!_data || !dlen || !_sig || !siglen || !keyring_id)
+		return -EINVAL;
+	/*
+	 * Possibly various signature handlers could scan signature and
+	 * claim it belongs to them and verify.
+	 */
+	if (sig_type == KEYCTL_SIG_TYPE_UNKNOWN)
+		return -EOPNOTSUPP;
+
+	/* Get the keyring which should be used */
+	keyring_ref = lookup_user_key(keyring_id, 0, KEY_SEARCH);
+	if (IS_ERR(keyring_ref))
+		return PTR_ERR(keyring_ref);
+
+
+	ret = -ENOMEM;
+	sig = kmalloc(siglen, GFP_KERNEL);
+	if (!sig)
+		goto error_keyref_put;
+
+	ret = -EFAULT;
+	if (copy_from_user(sig, _sig, siglen) != 0)
+		goto error_free_sig;
+
+	switch(sig_type) {
+	case KEYCTL_SIG_TYPE_INTEGRITY_DIGSIG:
+		ret = integrity_verify_user_buffer_digsig(
+					key_ref_to_ptr(keyring_ref),
+					_data, dlen, sig, siglen);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+error_free_sig:
+	kfree(sig);
+error_keyref_put:
+	key_ref_put(keyring_ref);
+	return ret;
+}
+
+/*
+ * Use a key to verify a signature.
+ *
+ * The key argument gives a key to use or a keyring in which a suitable key
+ * might be found.  The signature will be examined and an attempt will be made
+ * to determine the key to use from the information contained therein.
+ */
+long keyctl_verify_signature(const void __user *_sig_data)
+{
+	struct keyctl_sig_data sig_data;
+	int result;
+
+	result = copy_from_user(&sig_data, _sig_data, sizeof(sig_data));
+	if (result)
+		return -EFAULT;
+
+	return __keyctl_verify_signature(sig_data.keyring_id, sig_data.data,
+				sig_data.datalen, sig_data.sig, sig_data.siglen,
+				sig_data.sig_type, sig_data.flags);
+
+}
+
 /*
  * The key control system call
  */
@@ -1670,6 +1746,9 @@ SYSCALL_DEFINE5(keyctl, int, option, unsigned long, arg2, unsigned long, arg3,
 	case KEYCTL_GET_PERSISTENT:
 		return keyctl_get_persistent((uid_t)arg2, (key_serial_t)arg3);
 
+	case KEYCTL_VERIFY_SIGNATURE:
+		return keyctl_verify_signature((const void __user *)arg2);
+
 	default:
 		return -EOPNOTSUPP;
 	}
-- 
1.8.3.1

[PATCH 11/16] ptrace: Do not allow ptrace() from unsigned process to signed one

[Vivek Goyal]

Do not allow unsigned processes to ptrace() signed ones otherwise they can
modify the address space of signed processes and whole purpose of signature
verification is defeated.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/binfmt_elf.c      | 49 +++++++++++++++++++++++++++++++++++++++++++++++--
 security/commoncap.c | 11 +++++++++++
 2 files changed, 58 insertions(+), 2 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 22a8272..8f2286e 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -568,6 +568,43 @@ static unsigned long randomize_stack_top(unsigned long stack_top)
 #endif
 }
 
+#ifdef CONFIG_BINFMT_ELF_SIG
+/* check if current is being ptraced by tracer which is unsigned */
+static bool ptraced_by_unsafe_tracer(void)
+{
+	struct task_struct *child = current, *parent;
+	bool ret = false;
+	const struct cred *tcred;
+
+	/* Make sure parent does not change due to tracer ptrace detach */
+	read_lock(&tasklist_lock);
+
+	if (!child->ptrace) {
+		ret = false;
+		goto out;
+	}
+
+	parent = child->parent;
+	rcu_read_lock();
+	tcred = __task_cred(parent);
+	if (!tcred->proc_signed)
+		ret = true;
+	rcu_read_unlock();
+
+	/*
+	 * Make sure parent is memlocked too otherwise it might be signed
+	 * but still being swapped out and is open to address space
+	 * modifications.
+	 */
+	if (!test_bit(MMF_VM_LOCKED, &parent->mm->flags))
+		ret = true;
+
+out:
+	read_unlock(&tasklist_lock);
+	return ret;
+}
+#endif
+
 static int load_elf_binary(struct linux_binprm *bprm)
 {
 	struct file *interpreter = NULL; /* to shut gcc up */
@@ -951,8 +988,16 @@ static int load_elf_binary(struct linux_binprm *bprm)
 			send_sig(SIGKILL, current, 0);
 			goto out_free_dentry;
 		}
-		/* Signature verification successful */
-		bprm->cred->proc_signed = true;
+
+		/*
+		 * Signature verification successful. If this process is
+		 * is being ptraced at the time of exec() and tracer is
+		 * not signed, do not set proc_signed, otherwise unsigned
+		 * tracer could change signed tracee's address space,
+		 * effectively nullifying singature checking.
+		 */
+		if (!ptraced_by_unsafe_tracer())
+			bprm->cred->proc_signed = true;
 	}
 #endif
 
diff --git a/security/commoncap.c b/security/commoncap.c
index c44b6fe..4d7f90e 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -146,6 +146,12 @@ int cap_ptrace_access_check(struct task_struct *child, unsigned int mode)
 	rcu_read_lock();
 	cred = current_cred();
 	child_cred = __task_cred(child);
+
+	if (mode != PTRACE_MODE_READ && child_cred->proc_signed &&
+	    !cred->proc_signed) {
+		ret = -EPERM;
+		goto out;
+	}
 	if (cred->user_ns == child_cred->user_ns &&
 	    cap_issubset(child_cred->cap_permitted, cred->cap_permitted))
 		goto out;
@@ -178,6 +184,11 @@ int cap_ptrace_traceme(struct task_struct *parent)
 	rcu_read_lock();
 	cred = __task_cred(parent);
 	child_cred = current_cred();
+
+	if (child_cred->proc_signed && !cred->proc_signed) {
+		ret = -EPERM;
+		goto out;
+	}
 	if (cred->user_ns == child_cred->user_ns &&
 	    cap_issubset(child_cred->cap_permitted, cred->cap_permitted))
 		goto out;
-- 
1.8.3.1

[PATCH 12/16] binfmt_elf: Do not mark process signed if binary has elf interpreter

[Vivek Goyal]

Currently one can write to shared libraries while these are mapped.
That means shared library code can not be trusted as after signature
verification, one can overwrite the code.

Till we find a way to take care of that issue, do not mark a process
signed if it has interpreter which in turn will load shared librareis.

This does not take care of application doing dlopen(). Just that be
careful while signing applications and don't sign anything which does
dlopen().

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/binfmt_elf.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 8f2286e..52f8bd2 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -995,8 +995,16 @@ static int load_elf_binary(struct linux_binprm *bprm)
 		 * not signed, do not set proc_signed, otherwise unsigned
 		 * tracer could change signed tracee's address space,
 		 * effectively nullifying singature checking.
+		 *
+		 * Also set proc_signed only if there is no elf interpreter.
+		 * We don't have a way to avoid writes to shared libraries
+		 * after they have been mapped. That means anybody can
+		 * write to library after signature verification. So don't
+		 * trust executables which are dynamically linked. This
+		 * does not cover dlopen() and friends. So don't sign
+		 * applications using dlopen().
 		 */
-		if (!ptraced_by_unsafe_tracer())
+		if (!ptraced_by_unsafe_tracer() && !elf_interpreter)
 			bprm->cred->proc_signed = true;
 	}
 #endif
-- 
1.8.3.1

[PATCH 13/16] kexec: Allow only signed processes to call sys_kexec() in secureboot mode

[Vivek Goyal]

Modify sys_kexec() so that it allows only signed processes to execute
sys_kexec() when secureboot is enabled.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 kernel/kexec.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/kernel/kexec.c b/kernel/kexec.c
index 59f7b55..478566e 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -907,6 +907,31 @@ static int kimage_load_segment(struct kimage *image,
 	return result;
 }
 
+static int check_task_signature(void)
+{
+	int ret = 0;
+	const struct cred *cred;
+
+	/* If secureboot is enabled, There are extra checks required */
+	/* TODO: Change it once secure_level patches stablize */
+/*
+	if (!secure_modules())
+		return ret;
+*/
+	/*
+	 * Calling process should be signed, memlocked.
+	 */
+
+	if (!test_bit(MMF_VM_LOCKED, &current->mm->flags))
+		return -EPERM;
+
+	cred = current_cred();
+	if (!cred->proc_signed)
+		return -EPERM;
+
+	return ret;
+}
+
 /*
  * Exec Kernel system call: for obvious reasons only root may call it.
  *
@@ -942,6 +967,10 @@ SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments,
 	if (!capable(CAP_SYS_BOOT))
 		return -EPERM;
 
+	result = check_task_signature();
+	if (result)
+		return result;
+
 	/*
 	 * Verify we have a legal set of flags
 	 * This leaves us room for future extensions.
-- 
1.8.3.1

[PATCH 14/16] kexec: Export sysfs attributes for secureboot and secure modules to user space

[Vivek Goyal]

User space kexec-tools need to know whether to verify signature of kernel
image being loaded. This patch exports two knobs to user space. One is
for knowing if  secureboot is enabled, this knob will be set to 1 if secure
boot is enabled. Other knob is secure_module_enabled. This knob will be set
to 1 if secure modules is one.

kexec-tools will verify signature of kernel image if either secureboot is
enabled or secure modules is enabled. The only difference between two is
that kexec-tools will set secureboot on in bootparams being passed to
second kernel if secureboot is on in first kernel.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 kernel/ksysfs.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/kernel/ksysfs.c b/kernel/ksysfs.c
index 6ada93c..7262245 100644
--- a/kernel/ksysfs.c
+++ b/kernel/ksysfs.c
@@ -18,6 +18,8 @@
 #include <linux/stat.h>
 #include <linux/sched.h>
 #include <linux/capability.h>
+#include <linux/efi.h>
+#include <linux/module.h>
 
 #define KERNEL_ATTR_RO(_name) \
 static struct kobj_attribute _name##_attr = __ATTR_RO(_name)
@@ -101,6 +103,25 @@ static ssize_t kexec_crash_loaded_show(struct kobject *kobj,
 }
 KERNEL_ATTR_RO(kexec_crash_loaded);
 
+static ssize_t secureboot_enabled_show(struct kobject *kobj,
+				       struct kobj_attribute *attr, char *buf)
+{
+	/* TODO: Change it once secureboot patches are in */
+	return sprintf(buf, "%d\n", 1);
+}
+KERNEL_ATTR_RO(secureboot_enabled);
+
+static ssize_t secure_modules_enabled_show(struct kobject *kobj,
+				       struct kobj_attribute *attr, char *buf)
+{
+	/*
+	 * TODO: Change it once secure_modules() or secure_level() patches
+	 * are in
+	 */
+	return sprintf(buf, "%d\n", 1);
+}
+KERNEL_ATTR_RO(secure_modules_enabled);
+
 static ssize_t kexec_crash_size_show(struct kobject *kobj,
 				       struct kobj_attribute *attr, char *buf)
 {
@@ -196,6 +217,10 @@ static struct attribute * kernel_attrs[] = {
 	&kexec_crash_size_attr.attr,
 	&vmcoreinfo_attr.attr,
 #endif
+#ifdef CONFIG_EFI
+	&secureboot_enabled_attr.attr,
+#endif
+	&secure_modules_enabled_attr.attr,
 	&rcu_expedited_attr.attr,
 	NULL
 };
-- 
1.8.3.1

Re: [PATCH 14/16] kexec: Export sysfs attributes for secureboot and secure modules to user space

[Greg KH]

On Tue, Sep 10, 2013 at 05:44:29PM -0400, Vivek Goyal wrote:
> User space kexec-tools need to know whether to verify signature of kernel
> image being loaded. This patch exports two knobs to user space. One is
> for knowing if  secureboot is enabled, this knob will be set to 1 if secure
> boot is enabled. Other knob is secure_module_enabled. This knob will be set
> to 1 if secure modules is one.
> 
> kexec-tools will verify signature of kernel image if either secureboot is
> enabled or secure modules is enabled. The only difference between two is
> that kexec-tools will set secureboot on in bootparams being passed to
> second kernel if secureboot is on in first kernel.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  kernel/ksysfs.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)

Minor nit, if you add/modify/delete sysfs files, you also have to update
Documentation/ABI/ with the information about those files.

thanks,

greg k-h

Re: [PATCH 14/16] kexec: Export sysfs attributes for secureboot and secure modules to user space

[Vivek Goyal]

On Tue, Sep 10, 2013 at 03:40:09PM -0700, Greg KH wrote:
> On Tue, Sep 10, 2013 at 05:44:29PM -0400, Vivek Goyal wrote:
> > User space kexec-tools need to know whether to verify signature of kernel
> > image being loaded. This patch exports two knobs to user space. One is
> > for knowing if  secureboot is enabled, this knob will be set to 1 if secure
> > boot is enabled. Other knob is secure_module_enabled. This knob will be set
> > to 1 if secure modules is one.
> > 
> > kexec-tools will verify signature of kernel image if either secureboot is
> > enabled or secure modules is enabled. The only difference between two is
> > that kexec-tools will set secureboot on in bootparams being passed to
> > second kernel if secureboot is on in first kernel.
> > 
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  kernel/ksysfs.c | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> 
> Minor nit, if you add/modify/delete sysfs files, you also have to update
> Documentation/ABI/ with the information about those files.
> 

Sure. Will update Documentation/ABI/ in next version.

Thanks
Vivek

Re: [PATCH 14/16] kexec: Export sysfs attributes for secureboot and secure modules to user space

[Josh Boyer]

On Tue, Sep 10, 2013 at 5:44 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> User space kexec-tools need to know whether to verify signature of kernel
> image being loaded. This patch exports two knobs to user space. One is
> for knowing if  secureboot is enabled, this knob will be set to 1 if secure
> boot is enabled. Other knob is secure_module_enabled. This knob will be set
> to 1 if secure modules is one.
>
> kexec-tools will verify signature of kernel image if either secureboot is
> enabled or secure modules is enabled. The only difference between two is
> that kexec-tools will set secureboot on in bootparams being passed to
> second kernel if secureboot is on in first kernel.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  kernel/ksysfs.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/kernel/ksysfs.c b/kernel/ksysfs.c
> index 6ada93c..7262245 100644
> --- a/kernel/ksysfs.c
> +++ b/kernel/ksysfs.c
> @@ -18,6 +18,8 @@
>  #include <linux/stat.h>
>  #include <linux/sched.h>
>  #include <linux/capability.h>
> +#include <linux/efi.h>
> +#include <linux/module.h>
>
>  #define KERNEL_ATTR_RO(_name) \
>  static struct kobj_attribute _name##_attr = __ATTR_RO(_name)
> @@ -101,6 +103,25 @@ static ssize_t kexec_crash_loaded_show(struct kobject *kobj,
>  }
>  KERNEL_ATTR_RO(kexec_crash_loaded);
>
> +static ssize_t secureboot_enabled_show(struct kobject *kobj,
> +                                      struct kobj_attribute *attr, char *buf)
> +{
> +       /* TODO: Change it once secureboot patches are in */
> +       return sprintf(buf, "%d\n", 1);
> +}
> +KERNEL_ATTR_RO(secureboot_enabled);

You're defaulting this to enabled, even on machines where SB isn't
possible.  I realize there are TODOs there, but you might want to
default it to off if you really intend this on going upstream before
any of the other secure_* infrastructure does.

> +
> +static ssize_t secure_modules_enabled_show(struct kobject *kobj,
> +                                      struct kobj_attribute *attr, char *buf)
> +{
> +       /*
> +        * TODO: Change it once secure_modules() or secure_level() patches
> +        * are in
> +        */
> +       return sprintf(buf, "%d\n", 1);
> +}
> +KERNEL_ATTR_RO(secure_modules_enabled);
> +

Similarly, this should either default to off, or just return the value
of sig_enforce.  You can replace the open coded sig_enforce with
secure_modules if/when it goes upstream.

josh

Re: [PATCH 14/16] kexec: Export sysfs attributes for secureboot and secure modules to user space

[Vivek Goyal]

On Tue, Sep 10, 2013 at 06:57:55PM -0400, Josh Boyer wrote:

[..]
> > +static ssize_t secureboot_enabled_show(struct kobject *kobj,
> > +                                      struct kobj_attribute *attr, char *buf)
> > +{
> > +       /* TODO: Change it once secureboot patches are in */
> > +       return sprintf(buf, "%d\n", 1);
> > +}
> > +KERNEL_ATTR_RO(secureboot_enabled);
> 
> You're defaulting this to enabled, even on machines where SB isn't
> possible.  I realize there are TODOs there, but you might want to
> default it to off if you really intend this on going upstream before
> any of the other secure_* infrastructure does.

I defaulted it to 1 because I wanted to artificially simulate that
secureboot is enabled and make sure kexec-tools verifies signature
of bzImage.

This is just an RFC series to show what's cooking and get early feedback
about design. I am not expecting any of this to get in before secureboot/
secure_modules/securelevel patches get in.

First some set of patches need to block kexec loading, only then these
patches make sense.

So don't read too much into current default values. Idea is to show
that kernel will export 1-2 knobs to user space which will signal
kexec-tools to verify signature of bzImage and also set "secureboot"
in bootparams for next kernel as needed.

Thanks
Vivek

[PATCH 15/16] bootparam: Pass acpi_rsdp pointer in bootparam

[Vivek Goyal]

Currently kexec does not enables EFI and its tables in second kernel. Hence
acpi rsdp root pointer is passed on command line. But secureboot does not trust
acpi_rsdp on command line as kernel can execute some of the code as retrieved
by following acpi_rsdp and root can modify command line. So in secureboot
mode we ignore acpi_rsdp on command line.

Start passing it in bootparams for the time being. kexec-tools will prepare
the bootparams and put acpi_rsdp pointer there.

Peter Jones suggested that scan all ACPI memory for acpi_rsdp if EFI is
not enabled. This probably is a better fix and most likely this patch will
change and adopt that approach down the line.

In fact if we figure out how to make UEFI run time calls in second kernel,
we will not need acpi_rsdp at all.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 arch/x86/include/uapi/asm/bootparam.h |  3 ++-
 arch/x86/kernel/acpi/boot.c           |  5 +++++
 drivers/acpi/osl.c                    | 10 ++++++++++
 include/linux/acpi.h                  |  1 +
 4 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
index c15ddaf..8a5f7ae 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -122,7 +122,8 @@ struct boot_params {
 	__u32 ext_ramdisk_image;			/* 0x0c0 */
 	__u32 ext_ramdisk_size;				/* 0x0c4 */
 	__u32 ext_cmd_line_ptr;				/* 0x0c8 */
-	__u8  _pad4[116];				/* 0x0cc */
+	__u64 acpi_rsdp_addr;				/* 0x0cc */
+	__u8  _pad4[108];				/* 0x0d4 */
 	struct edid_info edid_info;			/* 0x140 */
 	struct efi_info efi_info;			/* 0x1c0 */
 	__u32 alt_mem_k;				/* 0x1e0 */
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 2627a81..b667d65 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -43,6 +43,8 @@
 #include <asm/io.h>
 #include <asm/mpspec.h>
 #include <asm/smp.h>
+#include <asm/bootparam.h>
+#include <asm/setup.h>
 
 #include "sleep.h" /* To include x86_acpi_suspend_lowlevel */
 static int __initdata acpi_force = 0;
@@ -1518,6 +1520,9 @@ static struct dmi_system_id __initdata acpi_dmi_table_late[] = {
 
 void __init acpi_boot_table_init(void)
 {
+	/* Read  acpi_rsdp_bootparams from bootparam */
+	acpi_rsdp_bootparam = boot_params.acpi_rsdp_addr;
+
 	dmi_check_system(acpi_dmi_table);
 
 	/*
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 6ab2c35..99e8ca9 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -242,9 +242,19 @@ static int __init setup_acpi_rsdp(char *arg)
 early_param("acpi_rsdp", setup_acpi_rsdp);
 #endif
 
+unsigned long long acpi_rsdp_bootparam;
 acpi_physical_address __init acpi_os_get_root_pointer(void)
 {
 #ifdef CONFIG_KEXEC
+	/*
+	 * If bootloader (kexec in this case), has passed, the acpi_rsdp
+	 * in boot params, use that. In case of secureboot /sbin/kexec
+	 * is signed and verified. That means we can trust acpi_rsdp
+	 * as passed in by kexec bootloader
+	 */
+	if (acpi_rsdp_bootparam)
+		return acpi_rsdp_bootparam;
+
 	if (acpi_rsdp)
 		return acpi_rsdp;
 #endif
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 6ad72f9..54cc8ab 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -128,6 +128,7 @@ extern u32 acpi_irq_not_handled;
 
 extern int sbf_port;
 extern unsigned long acpi_realmode_flags;
+extern unsigned long long acpi_rsdp_bootparam;
 
 int acpi_register_gsi (struct device *dev, u32 gsi, int triggering, int polarity);
 int acpi_gsi_to_irq (u32 gsi, unsigned int *irq);
-- 
1.8.3.1

Re: [PATCH 15/16] bootparam: Pass acpi_rsdp pointer in bootparam

[H. Peter Anvin]

On 09/10/2013 02:44 PM, Vivek Goyal wrote:
> Currently kexec does not enables EFI and its tables in second kernel. Hence
> acpi rsdp root pointer is passed on command line. But secureboot does not trust
> acpi_rsdp on command line as kernel can execute some of the code as retrieved
> by following acpi_rsdp and root can modify command line. So in secureboot
> mode we ignore acpi_rsdp on command line.
> 
> Start passing it in bootparams for the time being. kexec-tools will prepare
> the bootparams and put acpi_rsdp pointer there.
> 
> Peter Jones suggested that scan all ACPI memory for acpi_rsdp if EFI is
> not enabled. This probably is a better fix and most likely this patch will
> change and adopt that approach down the line.
> 
> In fact if we figure out how to make UEFI run time calls in second kernel,
> we will not need acpi_rsdp at all.
> 

Borislav Petkov has been working on a fixed mapping of UEFI memory,
which should allow UEFI runtime calls across kexec.

	-hpa

Re: [PATCH 15/16] bootparam: Pass acpi_rsdp pointer in bootparam

[Borislav Petkov]

On Tue, Sep 10, 2013 at 03:52:24PM -0700, H. Peter Anvin wrote:
> Borislav Petkov has been working on a fixed mapping of UEFI memory,...

... who will back from vacation on Monday and will be sending out a new
RFC version.

-- 
Regards/Gruss,
Boris.

Re: [PATCH 15/16] bootparam: Pass acpi_rsdp pointer in bootparam

[Vivek Goyal]

On Wed, Sep 11, 2013 at 01:44:19PM +0200, Borislav Petkov wrote:
> On Tue, Sep 10, 2013 at 03:52:24PM -0700, H. Peter Anvin wrote:
> > Borislav Petkov has been working on a fixed mapping of UEFI memory,...
> 
> ... who will back from vacation on Monday and will be sending out a new
> RFC version.

Hi Boris,

I am looking forward to that new version. CCing Dave Young. He is also 
looking into it and going through history of patches.

Thanks
Vivek

Re: [PATCH 15/16] bootparam: Pass acpi_rsdp pointer in bootparam

[Borislav Petkov]

On Wed, Sep 11, 2013 at 09:45:34AM -0400, Vivek Goyal wrote:
> I am looking forward to that new version. CCing Dave Young. He is also
> looking into it and going through history of patches.

Ok, I'll CC you guys on the submission - I'd need any and all feedback I
can get on that topic.

Thanks.

-- 
Regards/Gruss,
Boris.

Re: [PATCH 15/16] bootparam: Pass acpi_rsdp pointer in bootparam

[Dave Young]

On 09/11/13 at 04:32pm, Borislav Petkov wrote:
> On Wed, Sep 11, 2013 at 09:45:34AM -0400, Vivek Goyal wrote:
> > I am looking forward to that new version. CCing Dave Young. He is also
> > looking into it and going through history of patches.
> 
> Ok, I'll CC you guys on the submission - I'd need any and all feedback I
> can get on that topic.

I'm playing with skipping SetVirtualAddressMap in kexec kernel, for same kernel
the test result is ok for me both for kexec and kdump. Takao Indoh sent a patch
with this approatch, but his V2 switched to use physical mapping.
During my test, additional data of config table elems need to be saved besides of
fw_vendor, runtime and tables or dereference taglep->guid will panic.

Also kexec userspace need to fill efi_info in bootparams and pass the previous
saved efi data to 2nd kernel.

I'm worrying just skiping enter virt mode have risk though it's an easy solution.
Your 1:1 mapping approatch looks better. I look forward to test your new patchset.
Are you also working on kexec userspace part? Already have a patch?

> 
> Thanks.
> 
> -- 
> Regards/Gruss,
> Boris.

Re: [PATCH 15/16] bootparam: Pass acpi_rsdp pointer in bootparam

[Borislav Petkov]

On Thu, Sep 12, 2013 at 03:34:15PM +0800, Dave Young wrote:
> I'm playing with skipping SetVirtualAddressMap in kexec kernel, for
> same kernel the test result is ok for me both for kexec and kdump.
> Takao Indoh sent a patch with this approatch, but his V2 switched to
> use physical mapping. During my test, additional data of config table

Physical mapping won't work because of some very brilliant Apple UEFI
implementations, as I came to realize. My previous version did that :)

> elems need to be saved besides of fw_vendor, runtime and tables or
> dereference taglep->guid will panic.
>
> Also kexec userspace need to fill efi_info in bootparams and pass the
> previous saved efi data to 2nd kernel.

Hmm, yes, we need to tell the kexec kernel the EFI regions.

> I'm worrying just skiping enter virt mode have risk though it's an
> easy solution. Your 1:1 mapping approatch looks better. I look forward
> to test your new patchset.

Yeah, we had a discussion at the SUSE Labs conf about whether we could
really skip SetVirtualAddressMap in the first kernel and do it in the
kexec kernel but the first kernel might want to call runtime services
for whatever reason and for that we need the runtime services. So we
opted for the stable VA mappings.

> Are you also working on kexec userspace part? Already have a patch?

Why userspace part - I'm thinking the kexec'ed kernel would simply add
the mappings made by SetVirtualAddressMap without calling it. And it
will know which mappings go to which virtual addresses because we start
at the -4G virtual address and go downwards and the mappings will have
the same addresses per UEFI implementation.

It'll make more sense when you see the code, I hope :)

Thanks.

-- 
Regards/Gruss,
Boris.

Re: [PATCH 15/16] bootparam: Pass acpi_rsdp pointer in bootparam

[Borislav Petkov]

On Thu, Sep 12, 2013 at 09:19:30AM -0400, Vivek Goyal wrote:
> I did not understand this argument that we need to use high virtual
> addresses because windows is using it and now we end up creating
> fixed EFI addresses and that becomes an ABI. If EFI implementations

The only thing that becomes sort-of ABI is that we start the mappings at
-4G virtual.

> are dependent on high addresses being passed, shouldn't it be those
> implementations which need to be fixed instead of kernel fixing EFI

Right, this is the biggest issue with firmware - vendors like to declare
those as End-of-Life platforms and for them there are no fixes. This is
the reason why we don't do the 1:1 mappings.

> addresses in higher region.

The thing is, reportedly some Apple UEFI implementations cannot stomach
1:1 SetVirtualAddressMap mappings. Also, the high addresses mappings is
the only thing that vendors test on windoze so in that field we want
to do what windoze does as this is the only thing that gets reliable
testing.

But I get the feeling we're feeling up stuff in the dark as firmware is
closed crap which we cannot look at.

HTH.

-- 
Regards/Gruss,
Boris.

Re: [PATCH 15/16] bootparam: Pass acpi_rsdp pointer in bootparam

[Matthew Garrett]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 694 bytes --]

On Thu, 2013-09-12 at 14:53 +0200, Borislav Petkov wrote:

> Why userspace part - I'm thinking the kexec'ed kernel would simply add
> the mappings made by SetVirtualAddressMap without calling it. And it
> will know which mappings go to which virtual addresses because we start
> at the -4G virtual address and go downwards and the mappings will have
> the same addresses per UEFI implementation.

The second kernel still needs to be passed a pointer to the EFI tables
and memory map.

-- 
Matthew Garrett <matthew.garrett@nebula.com>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 15/16] bootparam: Pass acpi_rsdp pointer in bootparam

[Borislav Petkov]

On Thu, Sep 12, 2013 at 02:34:58PM +0000, Matthew Garrett wrote:
> The second kernel still needs to be passed a pointer to the EFI tables
> and memory map.

Date: Thu, 12 Sep 2013 14:53:50 +0200
From: Borislav Petkov <bp@alien8.de
Message-ID: <20130912125350.GB9280@x1.alien8.de>

On Thu, Sep 12, 2013 at 03:34:15PM +0800, Dave Young wrote:
> Also kexec userspace need to fill efi_info in bootparams and pass the
> previous saved efi data to 2nd kernel.

Hmm, yes, we need to tell the kexec kernel the EFI regions.

-- 
Regards/Gruss,
Boris.

Re: [PATCH 15/16] bootparam: Pass acpi_rsdp pointer in bootparam

[Dave Young]

On 09/12/13 at 02:53pm, Borislav Petkov wrote:
> On Thu, Sep 12, 2013 at 03:34:15PM +0800, Dave Young wrote:
> > I'm playing with skipping SetVirtualAddressMap in kexec kernel, for
> > same kernel the test result is ok for me both for kexec and kdump.
> > Takao Indoh sent a patch with this approatch, but his V2 switched to
> > use physical mapping. During my test, additional data of config table
> 
> Physical mapping won't work because of some very brilliant Apple UEFI
> implementations, as I came to realize. My previous version did that :)
> 
> > elems need to be saved besides of fw_vendor, runtime and tables or
> > dereference taglep->guid will panic.
> >
> > Also kexec userspace need to fill efi_info in bootparams and pass the
> > previous saved efi data to 2nd kernel.
> 
> Hmm, yes, we need to tell the kexec kernel the EFI regions.
> 
> > I'm worrying just skiping enter virt mode have risk though it's an
> > easy solution. Your 1:1 mapping approatch looks better. I look forward
> > to test your new patchset.
> 
> Yeah, we had a discussion at the SUSE Labs conf about whether we could
> really skip SetVirtualAddressMap in the first kernel and do it in the
> kexec kernel but the first kernel might want to call runtime services
> for whatever reason and for that we need the runtime services. So we
> opted for the stable VA mappings.

I means only skipping SetVirtualAddressMap in kexec kernel, ioremap result 
should be same with 1st kernel, but I'm not sure, it's what I worried about.
With 1:1 mapping, even we do not call SetVirtualAddressMap in kexec kernel
the mapping will be same in theory.

> 
> > Are you also working on kexec userspace part? Already have a patch?
> 
> Why userspace part - I'm thinking the kexec'ed kernel would simply add
> the mappings made by SetVirtualAddressMap without calling it. And it
> will know which mappings go to which virtual addresses because we start
> at the -4G virtual address and go downwards and the mappings will have
> the same addresses per UEFI implementation.
> 
> It'll make more sense when you see the code, I hope :)

For user space part, we need at least fill efi_info in kexec boot loader.
I have a test patch for this.

Also the fw_vendor, runtime, tables elements will be fixed up to use
virtual address after 1st kernel call SetVirtualAddress, so even with
1:1 mapping we still need save them and use in kexec kernel.
Such as below code assume fw_vendor is physical address:
c16 = tmp = early_ioremap(efi.systab->fw_vendor, 2);

So as what Takao has done we need to save them and use the saved value
in kexeced kernel. Previously he used is_kdump_kernel() to check if it's
a kdump kernel. But this is not enough because we'd better to also
consider kexec. I'm thinking below two approatches:

1. use bootloader_type as below
#define LOADER_TYPE_KEXEC	0x0D
int is_kexec_boot()
{
	if (bootloader_type == LOADER_TYPE_KEXEC)
		return 1;
	return 0;
}

2. 1) is specific to X86, if consider other futuer arch, maybe add a
kernel cmdline like kexecboot=X [X=0|1|2], 0:not kexec 1:kexec 2:kdump

> 
> Thanks.
> 
> -- 
> Regards/Gruss,
> Boris.

Re: [PATCH 15/16] bootparam: Pass acpi_rsdp pointer in bootparam

[Borislav Petkov]

On Fri, Sep 13, 2013 at 03:12:40PM +0800, Dave Young wrote:
> Also the fw_vendor, runtime, tables elements will be fixed up to use
> virtual address after 1st kernel call SetVirtualAddress, so even with
> 1:1 mapping we still need save them and use in kexec kernel.

As I said a couple of times already, 1:1 mapping is a no go.

Concerning the runtime services, we need to pass the UEFI memory map to
the kexec'ed kernel because it needs to know those to start mapping them
from -4G virtual downwards.

-- 
Regards/Gruss,
Boris.

[PATCH 16/16] mount: Add a flag to not follow symlink at the end of mount point

[Vivek Goyal]

I have a requirement where I want to make sure that mount() fails if
mount point is a symlink. Hence introducing a new mount flag MS_NOSYMLINK.

Following is little more info on what I am trying to do. I am trying
to write patches for signed /sbin/kexec. That is /sbin/kexec binary will
be signed and in secureboot environment kernel will verify signature
of /sbin/kexec and upon successful verfication, /sbin/kexec will be
trusted and allowed to load new kernel.

/sbin/kexec gathers bunch of data from /sys and /proc. Given the fact that
only /sbin/kexec is trusted and not other root processes, one need to make
sure that a root process can not alter /sys or /proc to fool /sbin/kexec.

So requirement is that /sbin/kexec needs to make sure that it is
looking at /proc and /sys as exported by kernel (and not an artificial
view possibly created by a root process).

Eric Biederman suggested that use per process mount name space functionality.
/sbin/kexec runs as root. So create separate mount namespace. Make it
recursively private to disable any event propogation. Unmount existing
/proc and /sys and remount them.

Actual code of what I am trying to do in kexec-tools is posted here.

https://lists.fedoraproject.org/pipermail/kernel/2013-September/004463.html

Al Viro mentioned that one needs to make sure /proc and /sys are not symlinks.
Otherwise after remounting, root could remove symlinks and create /proc and
/sys with its own files.

And there comes the need to make sure mount point is not a symlink
and hence this patch.

I did basic testing by doing following and it seems to work.

syscall(__NR_mount, "none", <mount-point>, "proc", 1<<25,"");

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/namespace.c          | 6 +++++-
 include/uapi/linux/fs.h | 1 +
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 7b1ca9b..d19627e 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2278,7 +2278,11 @@ long do_mount(const char *dev_name, const char *dir_name,
 		((char *)data_page)[PAGE_SIZE - 1] = 0;
 
 	/* ... and get the mountpoint */
-	retval = kern_path(dir_name, LOOKUP_FOLLOW, &path);
+	if (flags & MS_NOSYMLINK)
+		retval = kern_path(dir_name, 0, &path);
+	else
+		retval = kern_path(dir_name, LOOKUP_FOLLOW, &path);
+
 	if (retval)
 		return retval;
 
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index a4ed56c..584f083 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -86,6 +86,7 @@ struct inodes_stat_t {
 #define MS_KERNMOUNT	(1<<22) /* this is a kern_mount call */
 #define MS_I_VERSION	(1<<23) /* Update inode I_version field */
 #define MS_STRICTATIME	(1<<24) /* Always perform atime updates */
+#define MS_NOSYMLINK	(1<<25) /* Do not follow symlink at the end */
 
 /* These sb flags are internal to the kernel */
 #define MS_NOSEC	(1<<28)
-- 
1.8.3.1

Re: [PATCH 00/16] [RFC PATCH] Signed kexec support

[Greg KH]

On Tue, Sep 10, 2013 at 05:44:15PM -0400, Vivek Goyal wrote:
> Hi,
> 
> Matthew has been posting patches to lock down kernel either due to
> secureboot requirements or because of signed modules with signing
> enforced. In kernel lock down mode, kexec will be disabled and that
> means kdump will not work either.
> 
> These patches sign /sbin/kexec and kernel verifies the signature
> and allows loading a kernel if signature verification is successful.
> IOW, trust is extended to validly signed user space.
> 
> Currently it works only for statically linked applications.

That's a really big restriction, pretty much making it not workable for
distros at the moment to use.

Any chance to change this in the future?  Or just rely on the existing
"signed binaries" functionality we have in the kernel today for the
kexec binary as well?  Wouldn't that be simpler?

thanks,

greg k-h

Re: [PATCH 00/16] [RFC PATCH] Signed kexec support

[Vivek Goyal]

On Wed, Sep 11, 2013 at 08:40:23PM -0700, Greg KH wrote:
> On Tue, Sep 10, 2013 at 05:44:15PM -0400, Vivek Goyal wrote:
> > Hi,
> > 
> > Matthew has been posting patches to lock down kernel either due to
> > secureboot requirements or because of signed modules with signing
> > enforced. In kernel lock down mode, kexec will be disabled and that
> > means kdump will not work either.
> > 
> > These patches sign /sbin/kexec and kernel verifies the signature
> > and allows loading a kernel if signature verification is successful.
> > IOW, trust is extended to validly signed user space.
> > 
> > Currently it works only for statically linked applications.
> 
> That's a really big restriction, pretty much making it not workable for
> distros at the moment to use.

It is a big restriction for general use case of signed user space but in
this case I am planning to build /sbin/kexec statically and solve the
kexec/kdump issue.

I have posted kexec-tools patches here.

https://lists.fedoraproject.org/pipermail/kernel/2013-September/004469.html

Once kernel patches get in, I plan to upstream kexec-tools patches too
and then distros should be able to build /sbin/kexec statically and
this should work.

Do you forsee a problem with that?

> 
> Any chance to change this in the future?

It might but currently I don't have any plans. I see atleast two issues
with that.

- If we allow dynamic linking for signed binaries, then dynamic libraries
  will have to be signed too. I suspect in that case pretty much whole of
  the user space will have to be signed. I am not sure if distros are
  willing to do that.

- Currently a shared library can be written on disk (unlike executables)
  while it is mapped. That means after signature verification a root just
  has to open and write to shared library and modify code and defeat the
  purpose of signature verfication.

Probably these issues can be addressed if there is a need. Just that I
have not looked into it.

> Or just rely on the existing
> "signed binaries" functionality we have in the kernel today for the
> kexec binary as well?  Wouldn't that be simpler?

Which signed binary mechanism are you referring to? Are you referring to
using IMA for signature verification? If yes, there are some issues with
that.

- IMA does not lock down signed binaries in memory. That means after
  signature verification files can potentially be swapped out and be
  attacked there and modified code then can be swapped back in.

- IMA caches the signature appraisal resutls and reappraises the things
  based on if file has been modified or not. But this does not detect any
  raw writes to disk. So after signature verification root should be able
  to do some raw writes to disk and IMA will think file signature are
  just fine.

- IMA does not have mechanism to keep track of signed applications and
  a mechanism to disallow ptrace() by unsigned applications. That means
  after signature verification root can just ptrace() signed binary and
  modify its code/data.

- IMA provides mechanism for file based signature verificaiton.
  kexec-tools also needs to verify signature of new kernel being loaded.
  Using IMA on bzImage file has same pitfalls where a file can be modified
  after signature verification.

  That's why I have extended keyctl() so that signature verification can
  be done on user space buffer. An application can first read a file in
  buffer and then ask kernel to verify signature. And now root should not
  be able to attack it.

So existing IMA does not seem to have been written for an environment
where all the user space is not signed we don't trust root and root can
attack a signed binary. And my patches try to fill that gap. 

Thanks
Vivek

Re: [PATCH 00/16] [RFC PATCH] Signed kexec support

[Greg KH]

On Thu, Sep 12, 2013 at 07:43:36AM -0400, Vivek Goyal wrote:
> On Wed, Sep 11, 2013 at 08:40:23PM -0700, Greg KH wrote:
> > On Tue, Sep 10, 2013 at 05:44:15PM -0400, Vivek Goyal wrote:
> > > Hi,
> > > 
> > > Matthew has been posting patches to lock down kernel either due to
> > > secureboot requirements or because of signed modules with signing
> > > enforced. In kernel lock down mode, kexec will be disabled and that
> > > means kdump will not work either.
> > > 
> > > These patches sign /sbin/kexec and kernel verifies the signature
> > > and allows loading a kernel if signature verification is successful.
> > > IOW, trust is extended to validly signed user space.
> > > 
> > > Currently it works only for statically linked applications.
> > 
> > That's a really big restriction, pretty much making it not workable for
> > distros at the moment to use.
> 
> It is a big restriction for general use case of signed user space but in
> this case I am planning to build /sbin/kexec statically and solve the
> kexec/kdump issue.
> 
> I have posted kexec-tools patches here.
> 
> https://lists.fedoraproject.org/pipermail/kernel/2013-September/004469.html
> 
> Once kernel patches get in, I plan to upstream kexec-tools patches too
> and then distros should be able to build /sbin/kexec statically and
> this should work.
> 
> Do you forsee a problem with that?

Your paranoia is admirable in these patches.  If they are accepted, that
is a good first step, but what about the other kexec variants out there?

> > Any chance to change this in the future?
> 
> It might but currently I don't have any plans. I see atleast two issues
> with that.
> 
> - If we allow dynamic linking for signed binaries, then dynamic libraries
>   will have to be signed too. I suspect in that case pretty much whole of
>   the user space will have to be signed. I am not sure if distros are
>   willing to do that.
> 
> - Currently a shared library can be written on disk (unlike executables)
>   while it is mapped. That means after signature verification a root just
>   has to open and write to shared library and modify code and defeat the
>   purpose of signature verfication.

Then the existing signature verification logic is broken if this is
possible.

> Probably these issues can be addressed if there is a need. Just that I
> have not looked into it.
> 
> > Or just rely on the existing
> > "signed binaries" functionality we have in the kernel today for the
> > kexec binary as well?  Wouldn't that be simpler?
> 
> Which signed binary mechanism are you referring to? Are you referring to
> using IMA for signature verification? If yes, there are some issues with
> that.

Yes, IMA.

> - IMA does not lock down signed binaries in memory. That means after
>   signature verification files can potentially be swapped out and be
>   attacked there and modified code then can be swapped back in.

How can you do that?  If this is the case, then IMA is pointless and
should be fixed.

> - IMA caches the signature appraisal resutls and reappraises the things
>   based on if file has been modified or not. But this does not detect any
>   raw writes to disk. So after signature verification root should be able
>   to do some raw writes to disk and IMA will think file signature are
>   just fine.

IMA should be fixed for this problem.

> - IMA does not have mechanism to keep track of signed applications and
>   a mechanism to disallow ptrace() by unsigned applications. That means
>   after signature verification root can just ptrace() signed binary and
>   modify its code/data.

Then IMA should be fixed.

> - IMA provides mechanism for file based signature verificaiton.
>   kexec-tools also needs to verify signature of new kernel being loaded.
>   Using IMA on bzImage file has same pitfalls where a file can be modified
>   after signature verification.
> 
>   That's why I have extended keyctl() so that signature verification can
>   be done on user space buffer. An application can first read a file in
>   buffer and then ask kernel to verify signature. And now root should not
>   be able to attack it.
> 
> So existing IMA does not seem to have been written for an environment
> where all the user space is not signed we don't trust root and root can
> attack a signed binary. And my patches try to fill that gap. 

It sounds like your changes should go into the IMA core code to resolve
the issues there, as I'm sure they want to also protect from the issues
you have pointed out here.  Have you talked to those developers about
this?

thanks,

greg k-h

Re: [PATCH 00/16] [RFC PATCH] Signed kexec support

[Mimi Zohar]

On Thu, 2013-09-12 at 09:17 -0700, Greg KH wrote:
> On Thu, Sep 12, 2013 at 07:43:36AM -0400, Vivek Goyal wrote:
> > On Wed, Sep 11, 2013 at 08:40:23PM -0700, Greg KH wrote:
> > > On Tue, Sep 10, 2013 at 05:44:15PM -0400, Vivek Goyal wrote:
> > > > Hi,
> > > > 
> > > > Matthew has been posting patches to lock down kernel either due to
> > > > secureboot requirements or because of signed modules with signing
> > > > enforced. In kernel lock down mode, kexec will be disabled and that
> > > > means kdump will not work either.
> > > > 
> > > > These patches sign /sbin/kexec and kernel verifies the signature
> > > > and allows loading a kernel if signature verification is successful.
> > > > IOW, trust is extended to validly signed user space.
> > > > 
> > > > Currently it works only for statically linked applications.
> > > 
> > > That's a really big restriction, pretty much making it not workable for
> > > distros at the moment to use.

> > It is a big restriction for general use case of signed user space but in
> > this case I am planning to build /sbin/kexec statically and solve the
> > kexec/kdump issue.
> > 
> > I have posted kexec-tools patches here.
> > 
> > https://lists.fedoraproject.org/pipermail/kernel/2013-September/004469.html
> > 
> > Once kernel patches get in, I plan to upstream kexec-tools patches too
> > and then distros should be able to build /sbin/kexec statically and
> > this should work.
> > 
> > Do you forsee a problem with that?
> 
> Your paranoia is admirable in these patches.  If they are accepted, that
> is a good first step, but what about the other kexec variants out there?
> 
> > > Any chance to change this in the future?
> > 
> > It might but currently I don't have any plans. I see atleast two issues
> > with that.
> > 
> > - If we allow dynamic linking for signed binaries, then dynamic libraries
> >   will have to be signed too. I suspect in that case pretty much whole of
> >   the user space will have to be signed. I am not sure if distros are
> >   willing to do that.
> > 
> > - Currently a shared library can be written on disk (unlike executables)
> >   while it is mapped. That means after signature verification a root just
> >   has to open and write to shared library and modify code and defeat the
> >   purpose of signature verfication.
> 
> Then the existing signature verification logic is broken if this is
> possible.
> 
> > Probably these issues can be addressed if there is a need. Just that I
> > have not looked into it.
> > 
> > > Or just rely on the existing
> > > "signed binaries" functionality we have in the kernel today for the
> > > kexec binary as well?  Wouldn't that be simpler?
> > 
> > Which signed binary mechanism are you referring to? Are you referring to
> > using IMA for signature verification? If yes, there are some issues with
> > that.
> 
> Yes, IMA.
> 
> > - IMA does not lock down signed binaries in memory. That means after
> >   signature verification files can potentially be swapped out and be
> >   attacked there and modified code then can be swapped back in.
> 
> How can you do that?  If this is the case, then IMA is pointless and
> should be fixed.
> 
> > - IMA caches the signature appraisal resutls and reappraises the things
> >   based on if file has been modified or not. But this does not detect any
> >   raw writes to disk. So after signature verification root should be able
> >   to do some raw writes to disk and IMA will think file signature are
> >   just fine.
> 
> IMA should be fixed for this problem.
> 
> > - IMA does not have mechanism to keep track of signed applications and
> >   a mechanism to disallow ptrace() by unsigned applications. That means
> >   after signature verification root can just ptrace() signed binary and
> >   modify its code/data.
> 
> Then IMA should be fixed.
> 
> > - IMA provides mechanism for file based signature verificaiton.
> >   kexec-tools also needs to verify signature of new kernel being loaded.
> >   Using IMA on bzImage file has same pitfalls where a file can be modified
> >   after signature verification.
> > 
> >   That's why I have extended keyctl() so that signature verification can
> >   be done on user space buffer. An application can first read a file in
> >   buffer and then ask kernel to verify signature. And now root should not
> >   be able to attack it.
> > 
> > So existing IMA does not seem to have been written for an environment
> > where all the user space is not signed we don't trust root and root can
> > attack a signed binary. And my patches try to fill that gap. 
> 
> It sounds like your changes should go into the IMA core code to resolve
> the issues there, as I'm sure they want to also protect from the issues
> you have pointed out here.  Have you talked to those developers about
> this?

IMA assumes a different threat model and performance tradeoffs.  The
solutions suggested for the kexec, single userspace application threat
model, presumably wouldn't scale very well.
 
Unlike the syscalls to load kernel modules, which either pass a buffer,
containing the file data and appended signature, or a file descriptor,
kexec doesn't pass either.  To get around this problem, Vivek's patches
extend the keyctl syscall to verify the userspace buffer containing the
filedata and signature.

Separating the file data signature verification from the existing kexec
syscall, even if the signature verification is done by the kernel,
results in needing to trust the application actually verified the
signature.  For this reason, Vivek's patches need to verify the
integrity of a single userspace application.

Mimi

Re: [PATCH 00/16] [RFC PATCH] Signed kexec support

[Andrea Adami]

Hello,

as one of the developers of kexecboot, a kexec-based
linux-as-bootloader, I'm following with interest this thread.
FWIW since the beginning we are compiling kexec-tools statically
against klibc for size constraints.

We have 2.02 and 2.0.4 almost finished (some issue with purgatory in
this last version).

There are some hacks needed but it is surely possible.

Cheers

Andrea



On Mon, Sep 16, 2013 at 4:28 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Thu, Sep 12, 2013 at 02:24:10PM -0400, Mimi Zohar wrote:
>
> [..]
>> > > So existing IMA does not seem to have been written for an environment
>> > > where all the user space is not signed we don't trust root and root can
>> > > attack a signed binary. And my patches try to fill that gap.
>> >
>> > It sounds like your changes should go into the IMA core code to resolve
>> > the issues there, as I'm sure they want to also protect from the issues
>> > you have pointed out here.  Have you talked to those developers about
>> > this?
>>
>> IMA assumes a different threat model and performance tradeoffs.  The
>> solutions suggested for the kexec, single userspace application threat
>> model, presumably wouldn't scale very well.
>
> Hi Mimi,
>
> Does IMA trust root or not? I got a feeling that IMA is assuming that
> root is trusted. Otherwise root can do raw writes to disk and bypass
> all the logic related to appraisal result caching.
>
> In fact on my system root disk belongs to group "disk". So any user in
> "disk" group seems to be a trusted user for IMA to work.
>
> Thanks
> Vivek
>
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 00/16] [RFC PATCH] Signed kexec support

[Vivek Goyal]

On Wed, Sep 18, 2013 at 04:51:21PM +0200, Andrea Adami wrote:
> Hello,
> 
> as one of the developers of kexecboot, a kexec-based
> linux-as-bootloader, I'm following with interest this thread.
> FWIW since the beginning we are compiling kexec-tools statically
> against klibc for size constraints.

Hi Andrea,

Good to know about kexecboot.

FWIW, recently we discussed the kexec/kdump with secureboot issue
at Linux Plumbers and Greg KH suggested that it will be simpler if
we implement a new system call which does the job of loading new
kernel. Effectively that means reimplementing /sbin/kexec in kernel.
This will also mean maintaining two code bases for some time and
slowly deprecating one over a period of time.

I will probably start with something  small and post proof of concept
patches for review and see how does that go. This is just heads up 
on what I am planning to do.

Thanks
Vivek

Re: [PATCH 00/16] [RFC PATCH] Signed kexec support

[Vivek Goyal]

On Thu, Sep 12, 2013 at 09:17:05AM -0700, Greg KH wrote:

[..]
> Your paranoia is admirable in these patches.  If they are accepted, that
> is a good first step, but what about the other kexec variants out there?

Any other kexec variant out there which are not statically compiled
will not work on secureboot enabled machines. They will continue to
work fine on machines they have been working on in the past.

[..]
> > - Currently a shared library can be written on disk (unlike executables)
> >   while it is mapped. That means after signature verification a root just
> >   has to open and write to shared library and modify code and defeat the
> >   purpose of signature verfication.
> 
> Then the existing signature verification logic is broken if this is
> possible.

I found following thread regarding being able to overwrite shared
libraries.

http://lkml.indiana.edu/hypermail/linux/kernel/0110.0/0476.html

I have not tested, but I think it is still the case that one can overwrite
mapped libraries.


[..]
> > - IMA does not lock down signed binaries in memory. That means after
> >   signature verification files can potentially be swapped out and be
> >   attacked there and modified code then can be swapped back in.
> 
> How can you do that?  If this is the case, then IMA is pointless and
> should be fixed.

Once things are swapped out to a disk, technically now root can do raw
writes to disk and modify process address space. That's a different
thing that it might be littler harder to do.

I am not sure what threat model IMA is exactly addressing. I will let
IMA developers help us understand that use case better.

[..]
> > So existing IMA does not seem to have been written for an environment
> > where all the user space is not signed we don't trust root and root can
> > attack a signed binary. And my patches try to fill that gap. 
> 
> It sounds like your changes should go into the IMA core code to resolve
> the issues there, as I'm sure they want to also protect from the issues
> you have pointed out here.  Have you talked to those developers about
> this?

I have talked to IMA developers in the past. We are meeting at LPC also
this week and have more discussions about this. But looks like IMA is
serving some other thread model (I don't understand it though).

So key question is whether this is generic enough that IMA should
be fixed to take care of all the above issues, or this is niche enough
that elf loader can be modified to take care  of it.

Thanks
Vivek