* [PATCH 5/7] keys: Make __key_link_begin() handle lockdep nesting
From: David Howells @ 2019-05-22 22:28 UTC (permalink / raw)
To: keyrings; +Cc: dhowells, linux-security-module, linux-kernel
In-Reply-To: <155856408314.10428.17035328117829912815.stgit@warthog.procyon.org.uk>
Make __key_link_begin() handle lockdep nesting for the implementation of
key_move() where we have to lock two keyrings.
Signed-off-by: David Howells <dhowells@redhat.com>
---
security/keys/internal.h | 2 +-
security/keys/key.c | 6 +++---
security/keys/keyring.c | 6 +++---
security/keys/request_key.c | 2 +-
4 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 8f533c81aa8d..93513667ff9a 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -93,7 +93,7 @@ extern wait_queue_head_t request_key_conswq;
extern struct key_type *key_type_lookup(const char *type);
extern void key_type_put(struct key_type *ktype);
-extern int __key_link_begin(struct key *keyring,
+extern int __key_link_begin(struct key *keyring, unsigned int lock_nesting,
const struct keyring_index_key *index_key,
struct assoc_array_edit **_edit);
extern int __key_link_check_live_key(struct key *keyring, struct key *key);
diff --git a/security/keys/key.c b/security/keys/key.c
index 696f1c092c50..e0750bc85b68 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -515,7 +515,7 @@ int key_instantiate_and_link(struct key *key,
}
if (keyring) {
- ret = __key_link_begin(keyring, &key->index_key, &edit);
+ ret = __key_link_begin(keyring, 0, &key->index_key, &edit);
if (ret < 0)
goto error;
@@ -583,7 +583,7 @@ int key_reject_and_link(struct key *key,
if (keyring->restrict_link)
return -EPERM;
- link_ret = __key_link_begin(keyring, &key->index_key, &edit);
+ link_ret = __key_link_begin(keyring, 0, &key->index_key, &edit);
}
mutex_lock(&key_construction_mutex);
@@ -860,7 +860,7 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
}
index_key.desc_len = strlen(index_key.description);
- ret = __key_link_begin(keyring, &index_key, &edit);
+ ret = __key_link_begin(keyring, 0, &index_key, &edit);
if (ret < 0) {
key_ref = ERR_PTR(ret);
goto error_free_prep;
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 2cfeeeaa1ffa..cd669f758632 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -1202,7 +1202,7 @@ static int keyring_detect_cycle(struct key *A, struct key *B)
/*
* Preallocate memory so that a key can be linked into to a keyring.
*/
-int __key_link_begin(struct key *keyring,
+int __key_link_begin(struct key *keyring, unsigned int lock_nesting,
const struct keyring_index_key *index_key,
struct assoc_array_edit **_edit)
__acquires(&keyring->sem)
@@ -1219,7 +1219,7 @@ int __key_link_begin(struct key *keyring,
if (keyring->type != &key_type_keyring)
return -ENOTDIR;
- down_write(&keyring->sem);
+ down_write_nested(&keyring->sem, lock_nesting);
ret = -EKEYREVOKED;
if (test_bit(KEY_FLAG_REVOKED, &keyring->flags))
@@ -1366,7 +1366,7 @@ int key_link(struct key *keyring, struct key *key)
key_check(keyring);
key_check(key);
- ret = __key_link_begin(keyring, &key->index_key, &edit);
+ ret = __key_link_begin(keyring, 0, &key->index_key, &edit);
if (ret == 0) {
kdebug("begun {%d,%d}", keyring->serial, refcount_read(&keyring->usage));
ret = __key_link_check_restriction(keyring, key);
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 1f234b019437..e3653c6f85c6 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -372,7 +372,7 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
set_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags);
if (dest_keyring) {
- ret = __key_link_begin(dest_keyring, &ctx->index_key, &edit);
+ ret = __key_link_begin(dest_keyring, 0, &ctx->index_key, &edit);
if (ret < 0)
goto link_prealloc_failed;
}
^ permalink raw reply related
* [PATCH 6/7] keys: Add a keyctl to move a key between keyrings
From: David Howells @ 2019-05-22 22:28 UTC (permalink / raw)
To: keyrings; +Cc: dhowells, linux-security-module, linux-kernel
In-Reply-To: <155856408314.10428.17035328117829912815.stgit@warthog.procyon.org.uk>
Add a keyctl to atomically move a link to a key from one keyring to
another. The key must exist in "from" keyring and a flag can be given to
cause the operation to fail if there's a matching key already in the "to"
keyring.
This can be done with:
keyctl(KEYCTL_MOVE,
key_serial_t key,
key_serial_t from_keyring,
key_serial_t to_keyring,
unsigned int flags);
The key being moved must grant Link permission and both keyrings must grant
Write permission.
flags should be 0 or KEYCTL_MOVE_EXCL, with the latter preventing
displacement of a matching key from the "to" keyring.
Signed-off-by: David Howells <dhowells@redhat.com>
---
include/linux/key.h | 5 ++
include/uapi/linux/keyctl.h | 3 +
security/keys/compat.c | 3 +
security/keys/internal.h | 1
security/keys/keyctl.c | 55 +++++++++++++++++++++++++++
security/keys/keyring.c | 88 +++++++++++++++++++++++++++++++++++++++++++
6 files changed, 155 insertions(+)
diff --git a/include/linux/key.h b/include/linux/key.h
index 1f09aad1c98c..612e1cf84049 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -310,6 +310,11 @@ extern int key_update(key_ref_t key,
extern int key_link(struct key *keyring,
struct key *key);
+extern int key_move(struct key *key,
+ struct key *from_keyring,
+ struct key *to_keyring,
+ unsigned int flags);
+
extern int key_unlink(struct key *keyring,
struct key *key);
diff --git a/include/uapi/linux/keyctl.h b/include/uapi/linux/keyctl.h
index f45ee0f69c0c..fd9fb11b312b 100644
--- a/include/uapi/linux/keyctl.h
+++ b/include/uapi/linux/keyctl.h
@@ -67,6 +67,7 @@
#define KEYCTL_PKEY_SIGN 27 /* Create a public key signature */
#define KEYCTL_PKEY_VERIFY 28 /* Verify a public key signature */
#define KEYCTL_RESTRICT_KEYRING 29 /* Restrict keys allowed to link to a keyring */
+#define KEYCTL_MOVE 30 /* Move keys between keyrings */
/* keyctl structures */
struct keyctl_dh_params {
@@ -112,4 +113,6 @@ struct keyctl_pkey_params {
__u32 __spare[7];
};
+#define KEYCTL_MOVE_EXCL 0x00000001 /* Do not displace from the to-keyring */
+
#endif /* _LINUX_KEYCTL_H */
diff --git a/security/keys/compat.c b/security/keys/compat.c
index 9482df601dc3..b326bc4f84d7 100644
--- a/security/keys/compat.c
+++ b/security/keys/compat.c
@@ -159,6 +159,9 @@ COMPAT_SYSCALL_DEFINE5(keyctl, u32, option,
return keyctl_pkey_verify(compat_ptr(arg2), compat_ptr(arg3),
compat_ptr(arg4), compat_ptr(arg5));
+ case KEYCTL_MOVE:
+ return keyctl_keyring_move(arg2, arg3, arg4, arg5);
+
default:
return -EOPNOTSUPP;
}
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 93513667ff9a..821819b4ee13 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -215,6 +215,7 @@ extern long keyctl_update_key(key_serial_t, const void __user *, size_t);
extern long keyctl_revoke_key(key_serial_t);
extern long keyctl_keyring_clear(key_serial_t);
extern long keyctl_keyring_link(key_serial_t, key_serial_t);
+extern long keyctl_keyring_move(key_serial_t, key_serial_t, key_serial_t, unsigned int);
extern long keyctl_keyring_unlink(key_serial_t, key_serial_t);
extern long keyctl_describe_key(key_serial_t, char __user *, size_t);
extern long keyctl_keyring_search(key_serial_t, const char __user *,
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 0f947bcbad46..46188cda177e 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -572,6 +572,55 @@ long keyctl_keyring_unlink(key_serial_t id, key_serial_t ringid)
return ret;
}
+/*
+ * Move a link to a key from one keyring to another, displacing any matching
+ * key from the destination keyring.
+ *
+ * The key must grant the caller Link permission and both keyrings must grant
+ * the caller Write permission. There must also be a link in the from keyring
+ * to the key. If both keyrings are the same, nothing is done.
+ *
+ * If successful, 0 will be returned.
+ */
+long keyctl_keyring_move(key_serial_t id, key_serial_t from_ringid,
+ key_serial_t to_ringid, unsigned int flags)
+{
+ key_ref_t key_ref, from_ref, to_ref;
+ long ret;
+
+ if (flags & ~KEYCTL_MOVE_EXCL)
+ return -EINVAL;
+
+ key_ref = lookup_user_key(id, KEY_LOOKUP_CREATE, KEY_NEED_LINK);
+ if (IS_ERR(key_ref)) {
+ ret = PTR_ERR(key_ref);
+ goto error;
+ }
+
+ from_ref = lookup_user_key(from_ringid, 0, KEY_NEED_WRITE);
+ if (IS_ERR(from_ref)) {
+ ret = PTR_ERR(from_ref);
+ goto error2;
+ }
+
+ to_ref = lookup_user_key(to_ringid, KEY_LOOKUP_CREATE, KEY_NEED_WRITE);
+ if (IS_ERR(to_ref)) {
+ ret = PTR_ERR(to_ref);
+ goto error3;
+ }
+
+ ret = key_move(key_ref_to_ptr(key_ref), key_ref_to_ptr(from_ref),
+ key_ref_to_ptr(to_ref), flags);
+
+ key_ref_put(to_ref);
+error3:
+ key_ref_put(from_ref);
+error2:
+ key_ref_put(key_ref);
+error:
+ return ret;
+}
+
/*
* Return a description of a key to userspace.
*
@@ -1772,6 +1821,12 @@ SYSCALL_DEFINE5(keyctl, int, option, unsigned long, arg2, unsigned long, arg3,
(const void __user *)arg4,
(const void __user *)arg5);
+ case KEYCTL_MOVE:
+ return keyctl_keyring_move((key_serial_t)arg2,
+ (key_serial_t)arg3,
+ (key_serial_t)arg4,
+ (unsigned int)arg5);
+
default:
return -EOPNOTSUPP;
}
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index cd669f758632..df3144f9c1aa 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -1476,6 +1476,94 @@ int key_unlink(struct key *keyring, struct key *key)
}
EXPORT_SYMBOL(key_unlink);
+/**
+ * key_move - Move a key from one keyring to another
+ * @key: The key to move
+ * @from_keyring: The keyring to remove the link from.
+ * @to_keyring: The keyring to make the link in.
+ * @flags: Qualifying flags, such as KEYCTL_MOVE_EXCL.
+ *
+ * Make a link in @to_keyring to a key, such that the keyring holds a reference
+ * on that key and the key can potentially be found by searching that keyring
+ * whilst simultaneously removing a link to the key from @from_keyring.
+ *
+ * This function will write-lock both keyring's semaphores and will consume
+ * some of the user's key data quota to hold the link on @to_keyring.
+ *
+ * Returns 0 if successful, -ENOTDIR if either keyring isn't a keyring,
+ * -EKEYREVOKED if either keyring has been revoked, -ENFILE if the second
+ * keyring is full, -EDQUOT if there is insufficient key data quota remaining
+ * to add another link or -ENOMEM if there's insufficient memory. If
+ * KEYCTL_MOVE_EXCL is set, then -EEXIST will be returned if there's already a
+ * matching key in @to_keyring.
+ *
+ * It is assumed that the caller has checked that it is permitted for a link to
+ * be made (the keyring should have Write permission and the key Link
+ * permission).
+ */
+int key_move(struct key *key,
+ struct key *from_keyring,
+ struct key *to_keyring,
+ unsigned int flags)
+{
+ struct assoc_array_edit *from_edit, *to_edit;
+ int ret;
+
+ kenter("%d,%d,%d", key->serial, from_keyring->serial, to_keyring->serial);
+
+ if (from_keyring == to_keyring)
+ return 0;
+
+ key_check(key);
+ key_check(from_keyring);
+ key_check(to_keyring);
+
+ /* We have to be very careful here to take the keyring locks in the
+ * right order, lest we open ourselves to deadlocking against another
+ * move operation.
+ */
+ if (from_keyring < to_keyring) {
+ ret = __key_unlink_begin(from_keyring, 0, key, &from_edit);
+ if (ret < 0)
+ goto out;
+ ret = __key_link_begin(to_keyring, 1, &key->index_key, &to_edit);
+ if (ret < 0) {
+ assoc_array_cancel_edit(from_edit);
+ goto out;
+ }
+ } else {
+ ret = __key_link_begin(to_keyring, 0, &key->index_key, &to_edit);
+ if (ret < 0)
+ goto out;
+ ret = __key_unlink_begin(from_keyring, 1, key, &from_edit);
+ if (ret < 0) {
+ __key_link_end(to_keyring, &key->index_key, to_edit);
+ goto out;
+ }
+ }
+
+ ret = -EEXIST;
+ if (to_edit->dead_leaf && (flags & KEYCTL_MOVE_EXCL))
+ goto error;
+
+ ret = __key_link_check_restriction(to_keyring, key);
+ if (ret < 0)
+ goto error;
+ ret = __key_link_check_live_key(to_keyring, key);
+ if (ret < 0)
+ goto error;
+
+ __key_unlink(from_keyring, key, &from_edit);
+ __key_link(key, &to_edit);
+error:
+ __key_unlink_end(from_keyring, key, from_edit);
+ __key_link_end(to_keyring, &key->index_key, to_edit);
+out:
+ kleave(" = %d", ret);
+ return ret;
+}
+EXPORT_SYMBOL(key_move);
+
/**
* keyring_clear - Clear a keyring
* @keyring: The keyring to clear.
^ permalink raw reply related
* [PATCH 7/7] keys: Grant Link permission to possessers of request_key auth keys
From: David Howells @ 2019-05-22 22:28 UTC (permalink / raw)
To: keyrings; +Cc: dhowells, linux-security-module, linux-kernel
In-Reply-To: <155856408314.10428.17035328117829912815.stgit@warthog.procyon.org.uk>
Grant Link permission to the possessers of request_key authentication keys,
thereby allowing a daemon that is servicing upcalls to arrange things such
that only the necessary auth key is passed to the actual service program
and not all the daemon's pending auth keys.
Signed-off-by: David Howells <dhowells@redhat.com>
---
security/keys/request_key_auth.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/security/keys/request_key_auth.c b/security/keys/request_key_auth.c
index 572c7a60473a..ec5226557023 100644
--- a/security/keys/request_key_auth.c
+++ b/security/keys/request_key_auth.c
@@ -204,7 +204,7 @@ struct key *request_key_auth_new(struct key *target, const char *op,
authkey = key_alloc(&key_type_request_key_auth, desc,
cred->fsuid, cred->fsgid, cred,
- KEY_POS_VIEW | KEY_POS_READ | KEY_POS_SEARCH |
+ KEY_POS_VIEW | KEY_POS_READ | KEY_POS_SEARCH | KEY_POS_LINK |
KEY_USR_VIEW, KEY_ALLOC_NOT_IN_QUOTA, NULL);
if (IS_ERR(authkey)) {
ret = PTR_ERR(authkey);
^ permalink raw reply related
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Andy Lutomirski @ 2019-05-22 22:42 UTC (permalink / raw)
To: Sean Christopherson
Cc: Stephen Smalley, Jarkko Sakkinen, Andy Lutomirski, James Morris,
Serge E. Hallyn, LSM List, Paul Moore, Eric Paris, selinux,
Jethro Beekman, Xing, Cedric, Hansen, Dave, Thomas Gleixner,
Dr. Greg, Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes
In-Reply-To: <20190522153836.GA24833@linux.intel.com>
On Wed, May 22, 2019 at 8:38 AM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Wed, May 22, 2019 at 09:56:30AM -0400, Stephen Smalley wrote:
> > On 5/22/19 9:22 AM, Jarkko Sakkinen wrote:
> > >On Wed, May 22, 2019 at 04:20:22PM +0300, Jarkko Sakkinen wrote:
> > >>On Tue, May 21, 2019 at 08:51:40AM -0700, Sean Christopherson wrote:
> > >>>Except that mmap() is more or less required to guarantee that ELRANGE
> > >>>established by ECREATE is available. And we want to disallow mmap() as
> > >>>soon as the first EADD is done so that userspace can't remap the enclave's
> > >>>VMAs via munmap()->mmap() and gain execute permissions to pages that were
> > >>>EADD'd as NX.
> > >>
> > >>We don't want to guarantee such thing and it is not guaranteed. It does
> > >>not fit at all to the multi process work done. Enclaves are detached
> > >>from any particular process addresse spaces. It is responsibility of
> > >>process to open windows to them.
> > >>
> > >>That would be completely against work that we've done lately.
> > >
> > >Example use case: you have a process that just constructs an enclave
> > >and sends it to another process or processes for use. The constructor
> > >process could have basically anything on that range. This was the key
> > >goal of the fd based enclave work.
> >
> > What exactly happens in the constructor versus the recipient processes?
> > Which process performs each of the necessary open(), mmap(), and ioctl()
> > calls for setting up the enclave? Can you provide a high level overview of
> > the sequence of userspace calls by the constructor and by the recipient
> > similar to what Sean showed earlier for just a single process?
>
> Hmm, what we had talked about was allowing the SGX ioctls to work without
> an associated VMA, with the end goal of letting userspace restrict access
> to /dev/sgx/enclave. Very roughly...
>
> Enclave Owner:
>
> connect(builder, ...);
> send(builder, "/home/sean/path/to/my/enclave");
>
> recv(builder, &enclave_fd);
>
> for_each_chunk {
> mmap(enclave_addr + offset, size, ..., MAP_SHARED, enclave_fd, 0);
> }
>
>
> Enclave Builder:
>
> recv(sock, &enclave_path);
>
> source_fd = open(enclave_path, O_RDONLY);
> for_each_chunk {
> <hand waving - mmap()/mprotect() the enclave file into regular memory>
> }
>
> enclave_fd = open("/dev/sgx/enclave", O_RDWR);
>
> ioctl(enclave_fd, ENCLAVE_CREATE, ...);
> for_each_chunk {
> struct sgx_enclave_add ioctlargs = {
> .offset = chunk.offset,
> .source = chunk.addr,
> .size = chunk.size,
> .type = chunk.type, /* SGX specific metadata */
> }
> ioctl(fd, ENCLAVE_ADD, &ioctlargs); /* modifies enclave's VMAs */
> }
> ioctl(enclave_fd, ENCLAVE_INIT, ...);
>
> write(sock, enclave_fd);
>
>
> But the above flow is flawed because there'a catch-22: ENCLAVE_ECREATE
> takes the virtual address of the enclave, but in the above flow that's
> not established until "mmap(..., enclave_fd)". And because an enclave's
> virtual range needs to be naturally aligned (hardware requirements), the
> enclave owner would need to do something like:
>
> source_fd = open("/home/sean/path/to/my/enclave", O_RDONLY);
> size = <parse size from source_fd>
>
> enclave_range = mmap(NULL, size*2, PROT_READ, ???, NULL, 0);
> enclave_addr = (enclave_range + (size - 1)) & ~(size - 1);
>
> connect(builder, ...);
> send(builder, {"/home/sean/path/to/my/enclave", enclave_addr});
>
> recv(builder, &enclave_fd);
>
> munmap(enclave_range);
>
> for_each_chunk {
> addr = mmap(enclave_addr + c.offset, c.size, ..., MAP_SHARED, enclave_fd, 0);
> if (addr != enclave_addr + c.offset)
> exit(1);
> }
>
> And that straight up doesn't work with the v20 driver because mmap() with
> the enclave_fd will run through sgx_get_unmapped_area(), which also does
> the natural alignment adjustments (the idea being that mmap() is mapping
> the entire enclave). E.g. mmap() will map the wrong address if the offset
> of a chunk is less than its size due to the driver adjusting the address.
That presumably needs to change.
Are we entirely missing an API to allocate a naturally aligned VA
range? That's kind of annoying.
>
> Eliminating sgx_get_unmapped_area() means userspace is once again on the
> hook for naturally aligning the enclave, which is less than desirable.
>
> Looking back at the original API discussions around a builder process[1],
> we never fleshed out the end-to-end flow. While having a builder process
> *sounds* reasonable, in practice it adds a lot of complexity without
> providing much in the way of added security. E.g. in addition to the
> above mmap() issues, since the order of EADDs affects the enclave
> measurement, the enclave owner would need to communicate the exact steps
> to build the enclave, or the builder would need a priori knowledge of the
> enclave format.
>
> Userspace can still restrict access to /dev/sgx/enclave, e.g. by having a
> daemon that requires additional credentials to obtain a new enclave_fd.
> So AFAICT, the only benefit to having a dedicated builder is that it can
> do its own whitelisting of enclaves, but since we're trending towards
> supporting whitelisting enclaves in the kernel, e.g. via sigstruct,
> whitelisting in userspace purely in userspace also provides marginal value.
>
> TL;DR: Requiring VMA backing to build an enclave seems reasonable and sane.
This isn't necessarily a problem, but we pretty much have to use
mprotect() then.
Maybe the semantics could just be that mmap() on the SGX device gives
natural alignment, but that there is no actual constraint enforced by
the driver as to whether mmap() happens before or after ECREATE.
After all, it's *ugly* for user code to reserve its address range with
an awkward giant mmap(), there's nothing fundamentally wrong with it.
As far as I know from this whole discussion, we still haven't come up
with any credible way to avoid tracking, per enclave page, whether
that page came from unmodified PROT_EXEC memory.
^ permalink raw reply
* [PATCH 0/6] keys: request_key() improvements(vspace)s
From: David Howells @ 2019-05-22 22:46 UTC (permalink / raw)
To: keyrings; +Cc: dhowells, linux-afs, linux-security-module, linux-kernel
Here's a fix and some improvements for request_key() intended for the next
merge window:
(1) Fix the lack of a Link permission check on a key found by request_key(),
thereby enabling request_key() to link keys that don't grant this
permission to the target keyring (which must still grant Write
permission).
Note that the key must be in the caller's keyrings already to be found.
(2) Invalidate used request_key authentication keys rather than revoking
them, so that they get cleaned up immediately rather than hanging around
till the expiry time is passed.
(3) Move the RCU locks outwards from the keyring search functions so that a
request_key_rcu() can be provided. This can be called in RCU mode, so it
can't sleep and can't upcall - but it can be called from LOOKUP_RCU
pathwalk mode.
(4) Cache the latest positive result of request_key*() temporarily in
task_struct so that filesystems that make a lot of request_key() calls
during pathwalk can take advantage of it to avoid having to redo the
searching.
It is assumed that the key just found is unlikely to be superseded
between steps in an RCU pathwalk.
Note that the cleanup of the cache is done on TIF_NOTIFY_RESUME, just
before userspace resumes, and on exit.
I've included, for illustration, two patches to the in-kernel AFS filesystem
to make them use this.
The patches can be found on the following branch:
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-request
and this depends on keys-misc. Note that the AFS patches aren't on this branch.
David
---
David Howells (6):
keys: Fix request_key() lack of Link perm check on found key
keys: Invalidate used request_key authentication keys
keys: Move the RCU locks outwards from the keyring search functions
keys: Cache result of request_key*() temporarily in task_struct
afs: Provide an RCU-capable key lookup
afs: Support RCU pathwalk
Documentation/security/keys/core.rst | 8 ++
Documentation/security/keys/request-key.rst | 11 +++
fs/afs/dir.c | 54 ++++++++++++++
fs/afs/internal.h | 1
fs/afs/security.c | 102 +++++++++++++++++++++++----
include/keys/request_key_auth-type.h | 1
include/linux/key.h | 3 +
include/linux/sched.h | 5 +
include/linux/tracehook.h | 5 +
kernel/cred.c | 9 ++
security/keys/internal.h | 6 +-
security/keys/key.c | 4 +
security/keys/keyring.c | 16 ++--
security/keys/proc.c | 4 +
security/keys/process_keys.c | 41 +++++------
security/keys/request_key.c | 97 +++++++++++++++++++++++++-
security/keys/request_key_auth.c | 60 ++++++++++------
17 files changed, 346 insertions(+), 81 deletions(-)
^ permalink raw reply
* [PATCH 1/6] keys: Fix request_key() lack of Link perm check on found key
From: David Howells @ 2019-05-22 22:46 UTC (permalink / raw)
To: keyrings; +Cc: dhowells, linux-afs, linux-security-module, linux-kernel
In-Reply-To: <155856516286.11737.11196637682919902718.stgit@warthog.procyon.org.uk>
The request_key() syscall allows a process to gain access to the 'possessor'
permits of any key that grants it Search permission by virtue of request_key()
not checking whether a key it finds grants Link permission to the caller.
Signed-off-by: David Howells <dhowells@redhat.com>
---
security/keys/request_key.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index e3653c6f85c6..d204d7c0152e 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -559,6 +559,16 @@ struct key *request_key_and_link(struct key_type *type,
key_ref = search_process_keyrings(&ctx);
if (!IS_ERR(key_ref)) {
+ if (dest_keyring) {
+ ret = key_task_permission(key_ref, current_cred(),
+ KEY_NEED_LINK);
+ if (ret < 0) {
+ key_ref_put(key_ref);
+ key = ERR_PTR(ret);
+ goto error_free;
+ }
+ }
+
key = key_ref_to_ptr(key_ref);
if (dest_keyring) {
ret = key_link(dest_keyring, key);
^ permalink raw reply related
* [PATCH 2/6] keys: Invalidate used request_key authentication keys
From: David Howells @ 2019-05-22 22:46 UTC (permalink / raw)
To: keyrings; +Cc: dhowells, linux-afs, linux-security-module, linux-kernel
In-Reply-To: <155856516286.11737.11196637682919902718.stgit@warthog.procyon.org.uk>
Invalidate used request_key authentication keys rather than revoking them
so that they get cleaned up immediately rather than potentially hanging
around. There doesn't seem any need to keep the revoked keys around.
Signed-off-by: David Howells <dhowells@redhat.com>
---
security/keys/key.c | 4 ++--
security/keys/request_key.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/security/keys/key.c b/security/keys/key.c
index e0750bc85b68..00c7dbfc6a77 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -459,7 +459,7 @@ static int __key_instantiate_and_link(struct key *key,
/* disable the authorisation key */
if (authkey)
- key_revoke(authkey);
+ key_invalidate(authkey);
if (prep->expiry != TIME64_MAX) {
key->expiry = prep->expiry;
@@ -607,7 +607,7 @@ int key_reject_and_link(struct key *key,
/* disable the authorisation key */
if (authkey)
- key_revoke(authkey);
+ key_invalidate(authkey);
}
mutex_unlock(&key_construction_mutex);
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index d204d7c0152e..807c32b2c736 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -222,7 +222,7 @@ static int construct_key(struct key *key, const void *callout_info,
/* check that the actor called complete_request_key() prior to
* returning an error */
WARN_ON(ret < 0 &&
- !test_bit(KEY_FLAG_REVOKED, &authkey->flags));
+ !test_bit(KEY_FLAG_INVALIDATED, &authkey->flags));
key_put(authkey);
kleave(" = %d", ret);
^ permalink raw reply related
* [PATCH 3/6] keys: Move the RCU locks outwards from the keyring search functions
From: David Howells @ 2019-05-22 22:46 UTC (permalink / raw)
To: keyrings; +Cc: dhowells, linux-afs, linux-security-module, linux-kernel
In-Reply-To: <155856516286.11737.11196637682919902718.stgit@warthog.procyon.org.uk>
Move the RCU locks outwards from the keyring search functions so that a
request_key_rcu() function can be provided that searches for keys without
sleeping and without attempting to construct new keys. This new function
must be invoked with the RCU read lock held.
Signed-off-by: David Howells <dhowells@redhat.com>
---
Documentation/security/keys/core.rst | 8 ++++
Documentation/security/keys/request-key.rst | 11 +++++
include/keys/request_key_auth-type.h | 1
include/linux/key.h | 3 +
security/keys/internal.h | 6 +--
security/keys/keyring.c | 16 ++++---
security/keys/proc.c | 4 +-
security/keys/process_keys.c | 41 ++++++++----------
security/keys/request_key.c | 52 +++++++++++++++++++++++
security/keys/request_key_auth.c | 60 ++++++++++++++++-----------
10 files changed, 141 insertions(+), 61 deletions(-)
diff --git a/Documentation/security/keys/core.rst b/Documentation/security/keys/core.rst
index 9521c4207f01..3b812be5ea1e 100644
--- a/Documentation/security/keys/core.rst
+++ b/Documentation/security/keys/core.rst
@@ -1121,6 +1121,14 @@ payload contents" for more information.
If intr is true, then the wait can be interrupted by a signal, in which
case error ERESTARTSYS will be returned.
+ * To search for a key under RCU conditions, call::
+
+ struct key *request_key_rcu(const struct key_type *type,
+ const char *description);
+
+ which is similar to request_key() except that it does not check for keys
+ that are under construction and it will not call out to userspace to
+ construct a key if it can't find a match.
* When it is no longer required, the key should be released using::
diff --git a/Documentation/security/keys/request-key.rst b/Documentation/security/keys/request-key.rst
index 600ad67d1707..2147f5ffb40f 100644
--- a/Documentation/security/keys/request-key.rst
+++ b/Documentation/security/keys/request-key.rst
@@ -36,6 +36,11 @@ or::
size_t callout_len,
void *aux);
+or::
+
+ struct key *request_key_rcu(const struct key_type *type,
+ const char *description);
+
Or by userspace invoking the request_key system call::
key_serial_t request_key(const char *type,
@@ -57,6 +62,10 @@ The two async in-kernel calls may return keys that are still in the process of
being constructed. The two non-async ones will wait for construction to
complete first.
+The *_rcu() call is like the in-kernel request_key() call, except that it
+doesn't check for keys that are under construction and doesn't attempt to
+construct missing keys.
+
The userspace interface links the key to a keyring associated with the process
to prevent the key from going away, and returns the serial number of the key to
the caller.
@@ -148,7 +157,7 @@ The Search Algorithm
A search of any particular keyring proceeds in the following fashion:
- 1) When the key management code searches for a key (keyring_search_aux) it
+ 1) When the key management code searches for a key (keyring_search_rcu) it
firstly calls key_permission(SEARCH) on the keyring it's starting with,
if this denies permission, it doesn't search further.
diff --git a/include/keys/request_key_auth-type.h b/include/keys/request_key_auth-type.h
index a726dd3f1dc6..2a046062bb42 100644
--- a/include/keys/request_key_auth-type.h
+++ b/include/keys/request_key_auth-type.h
@@ -18,6 +18,7 @@
* Authorisation record for request_key().
*/
struct request_key_auth {
+ struct rcu_head rcu;
struct key *target_key;
struct key *dest_keyring;
const struct cred *cred;
diff --git a/include/linux/key.h b/include/linux/key.h
index 612e1cf84049..3604a554df99 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -274,6 +274,9 @@ extern struct key *request_key(struct key_type *type,
const char *description,
const char *callout_info);
+extern struct key *request_key_rcu(struct key_type *type,
+ const char *description);
+
extern struct key *request_key_with_auxdata(struct key_type *type,
const char *description,
const void *callout_info,
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 821819b4ee13..fd75b051d02a 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -135,11 +135,11 @@ struct keyring_search_context {
extern bool key_default_cmp(const struct key *key,
const struct key_match_data *match_data);
-extern key_ref_t keyring_search_aux(key_ref_t keyring_ref,
+extern key_ref_t keyring_search_rcu(key_ref_t keyring_ref,
struct keyring_search_context *ctx);
-extern key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx);
-extern key_ref_t search_process_keyrings(struct keyring_search_context *ctx);
+extern key_ref_t search_cred_keyrings_rcu(struct keyring_search_context *ctx);
+extern key_ref_t search_process_keyrings_rcu(struct keyring_search_context *ctx);
extern struct key *find_keyring_by_name(const char *name, bool uid_keyring);
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index df3144f9c1aa..a086abba47a6 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -835,7 +835,7 @@ static bool search_nested_keyrings(struct key *keyring,
}
/**
- * keyring_search_aux - Search a keyring tree for a key matching some criteria
+ * keyring_search_rcu - Search a keyring tree for a matching key under RCU
* @keyring_ref: A pointer to the keyring with possession indicator.
* @ctx: The keyring search context.
*
@@ -847,7 +847,9 @@ static bool search_nested_keyrings(struct key *keyring,
* addition, the LSM gets to forbid keyring searches and key matches.
*
* The search is performed as a breadth-then-depth search up to the prescribed
- * limit (KEYRING_SEARCH_MAX_DEPTH).
+ * limit (KEYRING_SEARCH_MAX_DEPTH). The caller must hold the RCU read lock to
+ * prevent keyrings from being destroyed or rearranged whilst they are being
+ * searched.
*
* Keys are matched to the type provided and are then filtered by the match
* function, which is given the description to use in any way it sees fit. The
@@ -866,7 +868,7 @@ static bool search_nested_keyrings(struct key *keyring,
* In the case of a successful return, the possession attribute from
* @keyring_ref is propagated to the returned key reference.
*/
-key_ref_t keyring_search_aux(key_ref_t keyring_ref,
+key_ref_t keyring_search_rcu(key_ref_t keyring_ref,
struct keyring_search_context *ctx)
{
struct key *keyring;
@@ -888,11 +890,9 @@ key_ref_t keyring_search_aux(key_ref_t keyring_ref,
return ERR_PTR(err);
}
- rcu_read_lock();
ctx->now = ktime_get_real_seconds();
if (search_nested_keyrings(keyring, ctx))
__key_get(key_ref_to_ptr(ctx->result));
- rcu_read_unlock();
return ctx->result;
}
@@ -902,7 +902,7 @@ key_ref_t keyring_search_aux(key_ref_t keyring_ref,
* @type: The type of keyring we want to find.
* @description: The name of the keyring we want to find.
*
- * As keyring_search_aux() above, but using the current task's credentials and
+ * As keyring_search_rcu() above, but using the current task's credentials and
* type's default matching function and preferred search method.
*/
key_ref_t keyring_search(key_ref_t keyring,
@@ -928,7 +928,9 @@ key_ref_t keyring_search(key_ref_t keyring,
return ERR_PTR(ret);
}
- key = keyring_search_aux(keyring, &ctx);
+ rcu_read_lock();
+ key = keyring_search_rcu(keyring, &ctx);
+ rcu_read_unlock();
if (type->match_free)
type->match_free(&ctx.match_data);
diff --git a/security/keys/proc.c b/security/keys/proc.c
index 78ac305d715e..f081dceae3b9 100644
--- a/security/keys/proc.c
+++ b/security/keys/proc.c
@@ -179,7 +179,9 @@ static int proc_keys_show(struct seq_file *m, void *v)
* skip if the key does not indicate the possessor can view it
*/
if (key->perm & KEY_POS_VIEW) {
- skey_ref = search_my_process_keyrings(&ctx);
+ rcu_read_lock();
+ skey_ref = search_cred_keyrings_rcu(&ctx);
+ rcu_read_unlock();
if (!IS_ERR(skey_ref)) {
key_ref_put(skey_ref);
key_ref = make_key_ref(key, 1);
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index ba5d3172cafe..fb31b408e294 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -318,7 +318,8 @@ void key_fsgid_changed(struct cred *new_cred)
/*
* Search the process keyrings attached to the supplied cred for the first
- * matching key.
+ * matching key under RCU conditions (the caller must be holding the RCU read
+ * lock).
*
* The search criteria are the type and the match function. The description is
* given to the match function as a parameter, but doesn't otherwise influence
@@ -337,7 +338,7 @@ void key_fsgid_changed(struct cred *new_cred)
* In the case of a successful return, the possession attribute is set on the
* returned key reference.
*/
-key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx)
+key_ref_t search_cred_keyrings_rcu(struct keyring_search_context *ctx)
{
key_ref_t key_ref, ret, err;
const struct cred *cred = ctx->cred;
@@ -355,7 +356,7 @@ key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx)
/* search the thread keyring first */
if (cred->thread_keyring) {
- key_ref = keyring_search_aux(
+ key_ref = keyring_search_rcu(
make_key_ref(cred->thread_keyring, 1), ctx);
if (!IS_ERR(key_ref))
goto found;
@@ -373,7 +374,7 @@ key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx)
/* search the process keyring second */
if (cred->process_keyring) {
- key_ref = keyring_search_aux(
+ key_ref = keyring_search_rcu(
make_key_ref(cred->process_keyring, 1), ctx);
if (!IS_ERR(key_ref))
goto found;
@@ -394,7 +395,7 @@ key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx)
/* search the session keyring */
if (cred->session_keyring) {
- key_ref = keyring_search_aux(
+ key_ref = keyring_search_rcu(
make_key_ref(cred->session_keyring, 1), ctx);
if (!IS_ERR(key_ref))
@@ -415,7 +416,7 @@ key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx)
}
/* or search the user-session keyring */
else if (READ_ONCE(cred->user->session_keyring)) {
- key_ref = keyring_search_aux(
+ key_ref = keyring_search_rcu(
make_key_ref(READ_ONCE(cred->user->session_keyring), 1),
ctx);
if (!IS_ERR(key_ref))
@@ -448,16 +449,16 @@ key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx)
* the keys attached to the assumed authorisation key using its credentials if
* one is available.
*
- * Return same as search_my_process_keyrings().
+ * The caller must be holding the RCU read lock.
+ *
+ * Return same as search_cred_keyrings_rcu().
*/
-key_ref_t search_process_keyrings(struct keyring_search_context *ctx)
+key_ref_t search_process_keyrings_rcu(struct keyring_search_context *ctx)
{
struct request_key_auth *rka;
key_ref_t key_ref, ret = ERR_PTR(-EACCES), err;
- might_sleep();
-
- key_ref = search_my_process_keyrings(ctx);
+ key_ref = search_cred_keyrings_rcu(ctx);
if (!IS_ERR(key_ref))
goto found;
err = key_ref;
@@ -472,24 +473,17 @@ key_ref_t search_process_keyrings(struct keyring_search_context *ctx)
) {
const struct cred *cred = ctx->cred;
- /* defend against the auth key being revoked */
- down_read(&cred->request_key_auth->sem);
-
- if (key_validate(ctx->cred->request_key_auth) == 0) {
+ if (key_validate(cred->request_key_auth) == 0) {
rka = ctx->cred->request_key_auth->payload.data[0];
+ //// was search_process_keyrings() [ie. recursive]
ctx->cred = rka->cred;
- key_ref = search_process_keyrings(ctx);
+ key_ref = search_cred_keyrings_rcu(ctx);
ctx->cred = cred;
- up_read(&cred->request_key_auth->sem);
-
if (!IS_ERR(key_ref))
goto found;
-
ret = key_ref;
- } else {
- up_read(&cred->request_key_auth->sem);
}
}
@@ -504,7 +498,6 @@ key_ref_t search_process_keyrings(struct keyring_search_context *ctx)
found:
return key_ref;
}
-
/*
* See if the key we're looking at is the target key.
*/
@@ -693,7 +686,9 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
ctx.index_key.desc_len = strlen(key->description);
ctx.match_data.raw_data = key;
kdebug("check possessed");
- skey_ref = search_process_keyrings(&ctx);
+ rcu_read_lock();
+ skey_ref = search_process_keyrings_rcu(&ctx);
+ rcu_read_unlock();
kdebug("possessed=%p", skey_ref);
if (!IS_ERR(skey_ref)) {
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 807c32b2c736..59a4e533e76a 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -382,7 +382,9 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
* waited for locks */
mutex_lock(&key_construction_mutex);
- key_ref = search_process_keyrings(ctx);
+ rcu_read_lock();
+ key_ref = search_process_keyrings_rcu(ctx);
+ rcu_read_unlock();
if (!IS_ERR(key_ref))
goto key_already_present;
@@ -556,7 +558,9 @@ struct key *request_key_and_link(struct key_type *type,
}
/* search all the process keyrings for a key */
- key_ref = search_process_keyrings(&ctx);
+ rcu_read_lock();
+ key_ref = search_process_keyrings_rcu(&ctx);
+ rcu_read_unlock();
if (!IS_ERR(key_ref)) {
if (dest_keyring) {
@@ -747,3 +751,47 @@ struct key *request_key_async_with_auxdata(struct key_type *type,
callout_len, aux, NULL, KEY_ALLOC_IN_QUOTA);
}
EXPORT_SYMBOL(request_key_async_with_auxdata);
+
+/**
+ * request_key_rcu - Request key from RCU-read-locked context
+ * @type: The type of key we want.
+ * @description: The name of the key we want.
+ *
+ * Request a key from a context that we may not sleep in (such as RCU-mode
+ * pathwalk). Keys under construction are ignored.
+ *
+ * Return a pointer to the found key if successful, -ENOKEY if we couldn't find
+ * a key or some other error if the key found was unsuitable or inaccessible.
+ */
+struct key *request_key_rcu(struct key_type *type, const char *description)
+{
+ struct keyring_search_context ctx = {
+ .index_key.type = type,
+ .index_key.description = description,
+ .index_key.desc_len = strlen(description),
+ .cred = current_cred(),
+ .match_data.cmp = key_default_cmp,
+ .match_data.raw_data = description,
+ .match_data.lookup_type = KEYRING_SEARCH_LOOKUP_DIRECT,
+ .flags = (KEYRING_SEARCH_DO_STATE_CHECK |
+ KEYRING_SEARCH_SKIP_EXPIRED),
+ };
+ struct key *key;
+ key_ref_t key_ref;
+
+ kenter("%s,%s", type->name, description);
+
+ /* search all the process keyrings for a key */
+ key_ref = search_process_keyrings_rcu(&ctx);
+ if (IS_ERR(key_ref)) {
+ key = ERR_CAST(key_ref);
+ if (PTR_ERR(key_ref) == -EAGAIN)
+ key = ERR_PTR(-ENOKEY);
+ } else {
+ key = key_ref_to_ptr(key_ref);
+ }
+
+ kleave(" = %p", key);
+ return key;
+}
+EXPORT_SYMBOL(request_key_rcu);
diff --git a/security/keys/request_key_auth.c b/security/keys/request_key_auth.c
index ec5226557023..99ed7a8a273d 100644
--- a/security/keys/request_key_auth.c
+++ b/security/keys/request_key_auth.c
@@ -58,7 +58,7 @@ static void request_key_auth_free_preparse(struct key_preparsed_payload *prep)
static int request_key_auth_instantiate(struct key *key,
struct key_preparsed_payload *prep)
{
- key->payload.data[0] = (struct request_key_auth *)prep->data;
+ rcu_assign_keypointer(key, (struct request_key_auth *)prep->data);
return 0;
}
@@ -68,7 +68,7 @@ static int request_key_auth_instantiate(struct key *key,
static void request_key_auth_describe(const struct key *key,
struct seq_file *m)
{
- struct request_key_auth *rka = get_request_key_auth(key);
+ struct request_key_auth *rka = dereference_key_rcu(key);
seq_puts(m, "key:");
seq_puts(m, key->description);
@@ -83,7 +83,7 @@ static void request_key_auth_describe(const struct key *key,
static long request_key_auth_read(const struct key *key,
char __user *buffer, size_t buflen)
{
- struct request_key_auth *rka = get_request_key_auth(key);
+ struct request_key_auth *rka = dereference_key_locked(key);
size_t datalen;
long ret;
@@ -102,23 +102,6 @@ static long request_key_auth_read(const struct key *key,
return ret;
}
-/*
- * Handle revocation of an authorisation token key.
- *
- * Called with the key sem write-locked.
- */
-static void request_key_auth_revoke(struct key *key)
-{
- struct request_key_auth *rka = get_request_key_auth(key);
-
- kenter("{%d}", key->serial);
-
- if (rka->cred) {
- put_cred(rka->cred);
- rka->cred = NULL;
- }
-}
-
static void free_request_key_auth(struct request_key_auth *rka)
{
if (!rka)
@@ -131,16 +114,43 @@ static void free_request_key_auth(struct request_key_auth *rka)
kfree(rka);
}
+/*
+ * Dispose of the request_key_auth record under RCU conditions
+ */
+static void request_key_auth_rcu_disposal(struct rcu_head *rcu)
+{
+ struct request_key_auth *rka =
+ container_of(rcu, struct request_key_auth, rcu);
+
+ free_request_key_auth(rka);
+}
+
+/*
+ * Handle revocation of an authorisation token key.
+ *
+ * Called with the key sem write-locked.
+ */
+static void request_key_auth_revoke(struct key *key)
+{
+ struct request_key_auth *rka = dereference_key_locked(key);
+
+ kenter("{%d}", key->serial);
+ rcu_assign_keypointer(key, NULL);
+ call_rcu(&rka->rcu, request_key_auth_rcu_disposal);
+}
+
/*
* Destroy an instantiation authorisation token key.
*/
static void request_key_auth_destroy(struct key *key)
{
- struct request_key_auth *rka = get_request_key_auth(key);
+ struct request_key_auth *rka = rcu_access_pointer(key->payload.rcu_data0);
kenter("{%d}", key->serial);
-
- free_request_key_auth(rka);
+ if (rka) {
+ rcu_assign_keypointer(key, NULL);
+ call_rcu(&rka->rcu, request_key_auth_rcu_disposal);
+ }
}
/*
@@ -249,7 +259,9 @@ struct key *key_get_instantiation_authkey(key_serial_t target_id)
ctx.index_key.desc_len = sprintf(description, "%x", target_id);
- authkey_ref = search_process_keyrings(&ctx);
+ rcu_read_lock();
+ authkey_ref = search_process_keyrings_rcu(&ctx);
+ rcu_read_unlock();
if (IS_ERR(authkey_ref)) {
authkey = ERR_CAST(authkey_ref);
^ permalink raw reply related
* [PATCH 4/6] keys: Cache result of request_key*() temporarily in task_struct
From: David Howells @ 2019-05-22 22:46 UTC (permalink / raw)
To: keyrings; +Cc: dhowells, linux-afs, linux-security-module, linux-kernel
In-Reply-To: <155856516286.11737.11196637682919902718.stgit@warthog.procyon.org.uk>
If a filesystem uses keys to hold authentication tokens, then it needs a
token for each VFS operation that might perform an authentication check -
either by passing it to the server, or using to perform a check based on
authentication data cached locally.
For open files this isn't a problem, since the key should be cached in the
file struct since it represents the subject performing operations on that
file descriptor.
During pathwalk, however, there isn't anywhere to cache the key, except
perhaps in the nameidata struct - but that isn't exposed to the
filesystems. Further, a pathwalk can incur a lot of operations, calling
one or more of the following, for instance:
->lookup()
->permission()
->d_revalidate()
->d_automount()
->get_acl()
->getxattr()
on each dentry/inode it encounters - and each one may need to call
request_key(). And then, at the end of pathwalk, it will call the actual
operation:
->mkdir()
->mknod()
->getattr()
->open()
...
which may need to go and get the token again.
However, it is very likely that all of the operations on a single
dentry/inode - and quite possibly a sequence of them - will all want to use
the same authentication token, which suggests that caching it would be a
good idea.
To this end:
(1) Make it so that a positive result of request_key() and co. that didn't
require upcalling to userspace is cached temporarily in task_struct.
(2) The cache is 1 deep, so a new result displaces the old one.
(3) The key is released by exit and by notify-resume.
(4) The cache is cleared in a newly forked process.
Signed-off-by: David Howells <dhowells@redhat.com>
---
include/linux/sched.h | 5 +++++
include/linux/tracehook.h | 5 +++++
kernel/cred.c | 9 +++++++++
security/keys/request_key.c | 33 +++++++++++++++++++++++++++++++++
4 files changed, 52 insertions(+)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 11837410690f..e5f18857dd53 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -831,6 +831,11 @@ struct task_struct {
/* Effective (overridable) subjective task credentials (COW): */
const struct cred __rcu *cred;
+#ifdef CONFIG_KEYS
+ /* Cached requested key. */
+ struct key *cached_requested_key;
+#endif
+
/*
* executable name, excluding path.
*
diff --git a/include/linux/tracehook.h b/include/linux/tracehook.h
index df20f8bdbfa3..ef497a94086c 100644
--- a/include/linux/tracehook.h
+++ b/include/linux/tracehook.h
@@ -187,6 +187,11 @@ static inline void tracehook_notify_resume(struct pt_regs *regs)
if (unlikely(current->task_works))
task_work_run();
+ if (unlikely(current->cached_requested_key)) {
+ key_put(current->cached_requested_key);
+ current->cached_requested_key = NULL;
+ }
+
mem_cgroup_handle_over_high();
blkcg_maybe_throttle_current();
}
diff --git a/kernel/cred.c b/kernel/cred.c
index 3bd40de9e192..20e0cd54aad2 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -174,6 +174,11 @@ void exit_creds(struct task_struct *tsk)
validate_creds(cred);
alter_cred_subscribers(cred, -1);
put_cred(cred);
+
+#ifdef CONFIG_KEYS
+ key_put(current->cached_requested_key);
+ current->cached_requested_key = NULL;
+#endif
}
/**
@@ -327,6 +332,10 @@ int copy_creds(struct task_struct *p, unsigned long clone_flags)
struct cred *new;
int ret;
+#ifdef CONFIG_KEYS
+ p->cached_requested_key = NULL;
+#endif
+
if (
#ifdef CONFIG_KEYS
!p->cred->thread_keyring &&
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 59a4e533e76a..855de14974c3 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -22,6 +22,27 @@
#define key_negative_timeout 60 /* default timeout on a negative key's existence */
+static struct key *check_cached_key(struct keyring_search_context *ctx)
+{
+ struct key *key = current->cached_requested_key;
+
+ if (key &&
+ ctx->match_data.cmp(key, &ctx->match_data) &&
+ !(key->flags & ((1 << KEY_FLAG_INVALIDATED) |
+ (1 << KEY_FLAG_REVOKED))))
+ return key_get(key);
+ return NULL;
+}
+
+static void cache_requested_key(struct key *key)
+{
+ struct task_struct *t = current;
+
+ key_put(t->cached_requested_key);
+ t->cached_requested_key = key_get(key);
+ set_tsk_thread_flag(t, TIF_NOTIFY_RESUME);
+}
+
/**
* complete_request_key - Complete the construction of a key.
* @authkey: The authorisation key.
@@ -557,6 +578,10 @@ struct key *request_key_and_link(struct key_type *type,
}
}
+ key = check_cached_key(&ctx);
+ if (key)
+ return key;
+
/* search all the process keyrings for a key */
rcu_read_lock();
key_ref = search_process_keyrings_rcu(&ctx);
@@ -582,6 +607,9 @@ struct key *request_key_and_link(struct key_type *type,
goto error_free;
}
}
+
+ /* Only cache the key on immediate success */
+ cache_requested_key(key);
} else if (PTR_ERR(key_ref) != -EAGAIN) {
key = ERR_CAST(key_ref);
} else {
@@ -781,6 +809,10 @@ struct key *request_key_rcu(struct key_type *type, const char *description)
kenter("%s,%s", type->name, description);
+ key = check_cached_key(&ctx);
+ if (key)
+ return key;
+
/* search all the process keyrings for a key */
key_ref = search_process_keyrings_rcu(&ctx);
if (IS_ERR(key_ref)) {
@@ -789,6 +821,7 @@ struct key *request_key_rcu(struct key_type *type, const char *description)
key = ERR_PTR(-ENOKEY);
} else {
key = key_ref_to_ptr(key_ref);
+ cache_requested_key(key);
}
kleave(" = %p", key);
^ permalink raw reply related
* [PATCH 5/6] afs: Provide an RCU-capable key lookup
From: David Howells @ 2019-05-22 22:46 UTC (permalink / raw)
To: keyrings; +Cc: dhowells, linux-afs, linux-security-module, linux-kernel
In-Reply-To: <155856516286.11737.11196637682919902718.stgit@warthog.procyon.org.uk>
Provide an RCU-capable key lookup function. We don't want to call
afs_request_key() in RCU-mode pathwalk as request_key() might sleep, even if
we don't ask it to construct anything as it might find a key that is currently
undergoing construction.
Signed-off-by: David Howells <dhowells@redhat.com>
---
fs/afs/internal.h | 1 +
fs/afs/security.c | 27 +++++++++++++++++++++++++++
2 files changed, 28 insertions(+)
diff --git a/fs/afs/internal.h b/fs/afs/internal.h
index 2073c1a3ab4b..3090efcc823f 100644
--- a/fs/afs/internal.h
+++ b/fs/afs/internal.h
@@ -1237,6 +1237,7 @@ extern void afs_cache_permit(struct afs_vnode *, struct key *, unsigned int,
struct afs_status_cb *);
extern void afs_zap_permits(struct rcu_head *);
extern struct key *afs_request_key(struct afs_cell *);
+extern struct key *afs_request_key_rcu(struct afs_cell *);
extern int afs_check_permit(struct afs_vnode *, struct key *, afs_access_t *);
extern int afs_permission(struct inode *, int);
extern void __exit afs_clean_up_permit_cache(void);
diff --git a/fs/afs/security.c b/fs/afs/security.c
index 5d8ece98561e..a6582d6a3882 100644
--- a/fs/afs/security.c
+++ b/fs/afs/security.c
@@ -49,6 +49,33 @@ struct key *afs_request_key(struct afs_cell *cell)
}
}
+/*
+ * Get a key when pathwalk is in rcuwalk mode.
+ */
+struct key *afs_request_key_rcu(struct afs_cell *cell)
+{
+ struct key *key;
+
+ _enter("{%x}", key_serial(cell->anonymous_key));
+
+ _debug("key %s", cell->anonymous_key->description);
+ key = request_key_rcu(&key_type_rxrpc, cell->anonymous_key->description);
+ if (IS_ERR(key)) {
+ if (PTR_ERR(key) != -ENOKEY) {
+ _leave(" = %ld", PTR_ERR(key));
+ return key;
+ }
+
+ /* act as anonymous user */
+ _leave(" = {%x} [anon]", key_serial(cell->anonymous_key));
+ return cell->anonymous_key;
+ } else {
+ /* act as authorised user */
+ _leave(" = {%x} [auth]", key_serial(key));
+ return key;
+ }
+}
+
/*
* Dispose of a list of permits.
*/
^ permalink raw reply related
* [PATCH 6/6] afs: Support RCU pathwalk
From: David Howells @ 2019-05-22 22:46 UTC (permalink / raw)
To: keyrings; +Cc: dhowells, linux-afs, linux-security-module, linux-kernel
In-Reply-To: <155856516286.11737.11196637682919902718.stgit@warthog.procyon.org.uk>
Make afs_permission() and afs_d_revalidate() do initial checks in RCU-mode
pathwalk to reduce latency in pathwalk elements that get done multiple
times. We don't need to query the server unless we've received a
notification from it that something has changed or the callback has
expired.
This requires that we can request a key and check permits under RCU
conditions if we need to.
Signed-off-by: David Howells <dhowells@redhat.com>
---
fs/afs/dir.c | 54 +++++++++++++++++++++++++++++++++++++-
fs/afs/security.c | 75 ++++++++++++++++++++++++++++++++++++++++++-----------
2 files changed, 112 insertions(+), 17 deletions(-)
diff --git a/fs/afs/dir.c b/fs/afs/dir.c
index 79d93a26759a..c394e7c1a8ab 100644
--- a/fs/afs/dir.c
+++ b/fs/afs/dir.c
@@ -961,6 +961,58 @@ static struct dentry *afs_lookup(struct inode *dir, struct dentry *dentry,
return d;
}
+/*
+ * Check the validity of a dentry under RCU conditions.
+ */
+static int afs_d_revalidate_rcu(struct dentry *dentry)
+{
+ struct afs_vnode *dvnode, *vnode;
+ struct dentry *parent;
+ struct inode *dir, *inode;
+ long dir_version, de_version;
+
+ _enter("%p", dentry);
+
+ /* Check the parent directory is still valid first. */
+ parent = READ_ONCE(dentry->d_parent);
+ dir = d_inode_rcu(parent);
+ if (!dir)
+ return -ECHILD;
+ dvnode = AFS_FS_I(dir);
+ if (test_bit(AFS_VNODE_DELETED, &dvnode->flags))
+ return -ECHILD;
+
+ if (!afs_check_validity(dvnode))
+ return -ECHILD;
+
+ /* We only need to invalidate a dentry if the server's copy changed
+ * behind our back. If we made the change, it's no problem. Note that
+ * on a 32-bit system, we only have 32 bits in the dentry to store the
+ * version.
+ */
+ dir_version = (long)READ_ONCE(dvnode->status.data_version);
+ de_version = (long)READ_ONCE(dentry->d_fsdata);
+ if (de_version != dir_version) {
+ dir_version = (long)READ_ONCE(dvnode->invalid_before);
+ if (de_version - dir_version < 0)
+ return -ECHILD;
+ }
+
+ /* Check to see if the vnode referred to by the dentry still
+ * has a callback.
+ */
+ if (d_really_is_positive(dentry)) {
+ inode = d_inode_rcu(dentry);
+ if (inode) {
+ vnode = AFS_FS_I(inode);
+ if (!afs_check_validity(vnode))
+ return -ECHILD;
+ }
+ }
+
+ return 1; /* Still valid */
+}
+
/*
* check that a dentry lookup hit has found a valid entry
* - NOTE! the hit can be a negative hit too, so we can't assume we have an
@@ -977,7 +1029,7 @@ static int afs_d_revalidate(struct dentry *dentry, unsigned int flags)
int ret;
if (flags & LOOKUP_RCU)
- return -ECHILD;
+ return afs_d_revalidate_rcu(dentry);
if (d_really_is_positive(dentry)) {
vnode = AFS_FS_I(d_inode(dentry));
diff --git a/fs/afs/security.c b/fs/afs/security.c
index a6582d6a3882..fab44171344f 100644
--- a/fs/afs/security.c
+++ b/fs/afs/security.c
@@ -305,6 +305,40 @@ void afs_cache_permit(struct afs_vnode *vnode, struct key *key,
return;
}
+static bool afs_check_permit_rcu(struct afs_vnode *vnode, struct key *key,
+ afs_access_t *_access)
+{
+ const struct afs_permits *permits;
+ int i;
+
+ _enter("{%llx:%llu},%x",
+ vnode->fid.vid, vnode->fid.vnode, key_serial(key));
+
+ /* check the permits to see if we've got one yet */
+ if (key == vnode->volume->cell->anonymous_key) {
+ *_access = vnode->status.anon_access;
+ _leave(" = t [anon %x]", *_access);
+ return true;
+ }
+
+ permits = rcu_dereference(vnode->permit_cache);
+ if (permits) {
+ for (i = 0; i < permits->nr_permits; i++) {
+ if (permits->permits[i].key < key)
+ continue;
+ if (permits->permits[i].key > key)
+ break;
+
+ *_access = permits->permits[i].access;
+ _leave(" = %u [perm %x]", !permits->invalidated, *_access);
+ return !permits->invalidated;
+ }
+ }
+
+ _leave(" = f");
+ return false;
+}
+
/*
* check with the fileserver to see if the directory or parent directory is
* permitted to be accessed with this authorisation, and if so, what access it
@@ -371,33 +405,42 @@ int afs_permission(struct inode *inode, int mask)
struct afs_vnode *vnode = AFS_FS_I(inode);
afs_access_t uninitialized_var(access);
struct key *key;
- int ret;
-
- if (mask & MAY_NOT_BLOCK)
- return -ECHILD;
+ int ret = 0;
_enter("{{%llx:%llu},%lx},%x,",
vnode->fid.vid, vnode->fid.vnode, vnode->flags, mask);
- key = afs_request_key(vnode->volume->cell);
- if (IS_ERR(key)) {
- _leave(" = %ld [key]", PTR_ERR(key));
- return PTR_ERR(key);
- }
+ if (mask & MAY_NOT_BLOCK) {
+ key = afs_request_key_rcu(vnode->volume->cell);
+ if (IS_ERR(key))
+ return -ECHILD;
- ret = afs_validate(vnode, key);
- if (ret < 0)
- goto error;
+ ret = -ECHILD;
+ if (!afs_check_validity(vnode) ||
+ !afs_check_permit_rcu(vnode, key, &access))
+ goto error;
+ } else {
+ key = afs_request_key(vnode->volume->cell);
+ if (IS_ERR(key)) {
+ _leave(" = %ld [key]", PTR_ERR(key));
+ return PTR_ERR(key);
+ }
- /* check the permits to see if we've got one yet */
- ret = afs_check_permit(vnode, key, &access);
- if (ret < 0)
- goto error;
+ ret = afs_validate(vnode, key);
+ if (ret < 0)
+ goto error;
+
+ /* check the permits to see if we've got one yet */
+ ret = afs_check_permit(vnode, key, &access);
+ if (ret < 0)
+ goto error;
+ }
/* interpret the access mask */
_debug("REQ %x ACC %x on %s",
mask, access, S_ISDIR(inode->i_mode) ? "dir" : "file");
+ ret = 0;
if (S_ISDIR(inode->i_mode)) {
if (mask & (MAY_EXEC | MAY_READ | MAY_CHDIR)) {
if (!(access & AFS_ACE_LOOKUP))
^ permalink raw reply related
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Sean Christopherson @ 2019-05-23 2:35 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Stephen Smalley, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
LSM List, Paul Moore, Eric Paris, selinux, Jethro Beekman,
Xing, Cedric, Hansen, Dave, Thomas Gleixner, Dr. Greg,
Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes
In-Reply-To: <CALCETrUS8xyF1JJmQs18BGTDhPRXf+s81BkMZCZwmY73r7M+zg@mail.gmail.com>
On Wed, May 22, 2019 at 03:42:45PM -0700, Andy Lutomirski wrote:
> On Wed, May 22, 2019 at 8:38 AM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> >
> > And that straight up doesn't work with the v20 driver because mmap() with
> > the enclave_fd will run through sgx_get_unmapped_area(), which also does
> > the natural alignment adjustments (the idea being that mmap() is mapping
> > the entire enclave). E.g. mmap() will map the wrong address if the offset
> > of a chunk is less than its size due to the driver adjusting the address.
>
> That presumably needs to change.
If we want to allow mmap() on a subset of the enclave, yes. I assume it's
a simple matter of respecting MAP_FIXED.
> Are we entirely missing an API to allocate a naturally aligned VA
> range? That's kind of annoying.
Yes?
> > Eliminating sgx_get_unmapped_area() means userspace is once again on the
> > hook for naturally aligning the enclave, which is less than desirable.
> >
> > Looking back at the original API discussions around a builder process[1],
> > we never fleshed out the end-to-end flow. While having a builder process
> > *sounds* reasonable, in practice it adds a lot of complexity without
> > providing much in the way of added security. E.g. in addition to the
> > above mmap() issues, since the order of EADDs affects the enclave
> > measurement, the enclave owner would need to communicate the exact steps
> > to build the enclave, or the builder would need a priori knowledge of the
> > enclave format.
> >
> > Userspace can still restrict access to /dev/sgx/enclave, e.g. by having a
> > daemon that requires additional credentials to obtain a new enclave_fd.
> > So AFAICT, the only benefit to having a dedicated builder is that it can
> > do its own whitelisting of enclaves, but since we're trending towards
> > supporting whitelisting enclaves in the kernel, e.g. via sigstruct,
> > whitelisting in userspace purely in userspace also provides marginal value.
> >
> > TL;DR: Requiring VMA backing to build an enclave seems reasonable and sane.
>
> This isn't necessarily a problem, but we pretty much have to use
> mprotect() then.
You lost me there. Who needs to mprotect() what?
> Maybe the semantics could just be that mmap() on the SGX device gives
> natural alignment, but that there is no actual constraint enforced by
> the driver as to whether mmap() happens before or after ECREATE.
> After all, it's *ugly* for user code to reserve its address range with
> an awkward giant mmap(), there's nothing fundamentally wrong with it.
>
> As far as I know from this whole discussion, we still haven't come up
> with any credible way to avoid tracking, per enclave page, whether
> that page came from unmodified PROT_EXEC memory.
Disallowing mmap() after ECREATE is credible, but apparently not
palatable. :-)
But actually, there's no need to disallow mmap() after ECREATE since the
LSM checks also apply to mmap(), e.g. FILE__EXECUTE would be needed to
mmap() any enclave pages PROT_EXEC. I guess my past self thought mmap()
bypassed LSM checks? The real problem is that mmap()'ng an existing
enclave would require FILE__WRITE and FILE__EXECUTE, which puts us back
at square one.
Tracking permissions per enclave page isn't difficult, it's the new SGX
specific LSM hooks and mprotect() interactions that I want to avoid.
Jumping back to mmap(), AIUI the fundamental issue is that we want to
allow building/running an enclave without FILE__WRITE and FILE__EXECUTE,
otherwise FILE__WRITE and FILE__EXECUTE become meaningless. Assuming I'm
not off in the weeds, that means we really just need to special case
mmap() on enclaves so it can map enclave memory using the verified page
permissions so as not to run afoul of LSM checks. All other behaviors,
e.g. mprotect(), can reuse the existing LSM checks for shared mappings.
So, what if we snapshot the permissions for each enclave page at EADD,
and then special case mmap() to propagate flags from the snapshot to the
VMA? More or less the same idea as doing mprotect_fixup() using the
source VMA during EADD. We could define the EADD semantics to match
this as well, e.g. only propagate the flags from the source VMA to the
enclave VMA if the EADD range is fully mapped with PROT_NONE. This would
allow the enclave builder concept, albeit with funky semantics, and
wouldn't require new LSM hooks.
E.g. something like this:
static inline void sgx_mmap_update_prot_flags(struct vm_area_struct *vma,
struct sgx_encl *encl)
{
struct radix_tree_iter iter;
struct sgx_encl_page *entry;
unsigned long addr;
vm_flags_t flags;
void **slot;
/*
* SGX special: if userspace is requesting PROT_NONE and pages have
* been added to the enclave, then propagate the flags snapshot from
* the enclave to the VMA. Do this if and only if all overlapped
* pages are defined and have identical permissions. Stuffing the
* VMA on PROT_NONE allows userspace to map EPC pages without being
* incorrectly rejected by LSMs due to insufficient permissions (the
* snapshottted flags have alaredy been vetted).
*/
if (vma->vm_flags & (VM_READ|VM_WRITE|VM_EXEC))
return;
flags = 0;
for (addr = vma->vm_start; addr < vma->vm_end; addr += PAGE_SIZE) {
entry = radix_tree_lookup(&encl->page_tree, addr >> PAGE_SHIFT);
if (!entry && flags)
return;
if (!flags && entry) {
if (addr == vma->vm_start) {
flags = entry->vm_flags;
continue;
}
return;
}
if (entry && flags && entry->vm_flags != flags)
return;
}
vma->vm_flags |= flags;
}
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Jarkko Sakkinen @ 2019-05-23 8:10 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Sean Christopherson, Stephen Smalley, James Morris,
Serge E. Hallyn, LSM List, Paul Moore, Eric Paris, selinux,
Jethro Beekman, Xing, Cedric, Hansen, Dave, Thomas Gleixner,
Dr. Greg, Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes
In-Reply-To: <CALCETrUS8xyF1JJmQs18BGTDhPRXf+s81BkMZCZwmY73r7M+zg@mail.gmail.com>
On Wed, May 22, 2019 at 03:42:45PM -0700, Andy Lutomirski wrote:
> As far as I know from this whole discussion, we still haven't come up
> with any credible way to avoid tracking, per enclave page, whether
> that page came from unmodified PROT_EXEC memory.
So is this in the context that the enclave is read from another VMA
and not through a file descriptor? Is that locked in?
/Jarkko
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Jarkko Sakkinen @ 2019-05-23 8:23 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Sean Christopherson, Stephen Smalley, James Morris,
Serge E. Hallyn, LSM List, Paul Moore, Eric Paris, selinux,
Jethro Beekman, Xing, Cedric, Hansen, Dave, Thomas Gleixner,
Dr. Greg, Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes
In-Reply-To: <20190523081048.GA10955@linux.intel.com>
On Thu, May 23, 2019 at 11:10:48AM +0300, Jarkko Sakkinen wrote:
> On Wed, May 22, 2019 at 03:42:45PM -0700, Andy Lutomirski wrote:
> > As far as I know from this whole discussion, we still haven't come up
> > with any credible way to avoid tracking, per enclave page, whether
> > that page came from unmodified PROT_EXEC memory.
>
> So is this in the context that the enclave is read from another VMA
> and not through a file descriptor? Is that locked in?
No need to answer. Got in page from Sean's response.
/Jarkko
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Jarkko Sakkinen @ 2019-05-23 10:26 UTC (permalink / raw)
To: Sean Christopherson
Cc: Andy Lutomirski, Stephen Smalley, James Morris, Serge E. Hallyn,
LSM List, Paul Moore, Eric Paris, selinux, Jethro Beekman,
Xing, Cedric, Hansen, Dave, Thomas Gleixner, Dr. Greg,
Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes
In-Reply-To: <20190523023517.GA31950@linux.intel.com>
On Wed, May 22, 2019 at 07:35:17PM -0700, Sean Christopherson wrote:
> But actually, there's no need to disallow mmap() after ECREATE since the
> LSM checks also apply to mmap(), e.g. FILE__EXECUTE would be needed to
> mmap() any enclave pages PROT_EXEC. I guess my past self thought mmap()
> bypassed LSM checks? The real problem is that mmap()'ng an existing
> enclave would require FILE__WRITE and FILE__EXECUTE, which puts us back
> at square one.
I'm lost with the constraints we want to set.
We can still support fork() if we take a step back from v20 and require
the mmap(). Given the recent comments, I'd guess that is the best
compromise i.e. multiple processes can still share an enclave within
the limitations of ancestor hierarchy. Is this the constraint we agree
now upon? Some emails are a bit contradicting in this sense.
> Tracking permissions per enclave page isn't difficult, it's the new SGX
> specific LSM hooks and mprotect() interactions that I want to avoid.
>
> Jumping back to mmap(), AIUI the fundamental issue is that we want to
> allow building/running an enclave without FILE__WRITE and FILE__EXECUTE,
> otherwise FILE__WRITE and FILE__EXECUTE become meaningless. Assuming I'm
> not off in the weeds, that means we really just need to special case
> mmap() on enclaves so it can map enclave memory using the verified page
> permissions so as not to run afoul of LSM checks. All other behaviors,
> e.g. mprotect(), can reuse the existing LSM checks for shared mappings.
>
> So, what if we snapshot the permissions for each enclave page at EADD,
> and then special case mmap() to propagate flags from the snapshot to the
> VMA? More or less the same idea as doing mprotect_fixup() using the
> source VMA during EADD. We could define the EADD semantics to match
> this as well, e.g. only propagate the flags from the source VMA to the
> enclave VMA if the EADD range is fully mapped with PROT_NONE. This would
> allow the enclave builder concept, albeit with funky semantics, and
> wouldn't require new LSM hooks.
Dropped off here completely. What if the mmap() is done before any of
the EADD operations?
>
> E.g. something like this:
>
> static inline void sgx_mmap_update_prot_flags(struct vm_area_struct *vma,
> struct sgx_encl *encl)
> {
> struct radix_tree_iter iter;
> struct sgx_encl_page *entry;
> unsigned long addr;
> vm_flags_t flags;
> void **slot;
>
> /*
> * SGX special: if userspace is requesting PROT_NONE and pages have
> * been added to the enclave, then propagate the flags snapshot from
> * the enclave to the VMA. Do this if and only if all overlapped
> * pages are defined and have identical permissions. Stuffing the
> * VMA on PROT_NONE allows userspace to map EPC pages without being
> * incorrectly rejected by LSMs due to insufficient permissions (the
> * snapshottted flags have alaredy been vetted).
> */
> if (vma->vm_flags & (VM_READ|VM_WRITE|VM_EXEC))
> return;
>
> flags = 0;
>
> for (addr = vma->vm_start; addr < vma->vm_end; addr += PAGE_SIZE) {
> entry = radix_tree_lookup(&encl->page_tree, addr >> PAGE_SHIFT);
>
> if (!entry && flags)
> return;
> if (!flags && entry) {
> if (addr == vma->vm_start) {
> flags = entry->vm_flags;
> continue;
> }
> return;
> }
> if (entry && flags && entry->vm_flags != flags)
> return;
>
> }
> vma->vm_flags |= flags;
> }
This looks flakky and error prone. You'd better have some "shadow VMAs"
and check that you have such matching size of the VMA you try to mmap()
and check flags from that.
Who would call this function anyhow and when?
Would be better to first agree on constraints. I have zero idea within
which kind of enviroment this snippet would live e.g.
- mmap() (before, after?)
- multi process constraint (only fork or full on versatility)
/Jarkko
^ permalink raw reply
* Re: [PATCH V7 0/4] Add support for crypto agile logs
From: Jarkko Sakkinen @ 2019-05-23 12:14 UTC (permalink / raw)
To: Matthew Garrett
Cc: linux-integrity, peterhuewe, jgg, roberto.sassu, linux-efi,
linux-security-module, linux-kernel, tweek, bsz
In-Reply-To: <20190520205501.177637-1-matthewgarrett@google.com>
On Mon, May 20, 2019 at 01:54:57PM -0700, Matthew Garrett wrote:
> Identical to previous version except without the KSAN workaround - Ard
> has a better solution for that.
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
/Jarkko
^ permalink raw reply
* [PATCH v4 0/3] initramfs: add support for xattrs in the initial ram disk
From: Roberto Sassu @ 2019-05-23 12:18 UTC (permalink / raw)
To: viro
Cc: linux-security-module, linux-integrity, initramfs, linux-api,
linux-fsdevel, linux-kernel, bug-cpio, zohar, silviu.vlasceanu,
dmitry.kasatkin, takondra, kamensky, hpa, arnd, rob,
james.w.mcmechan, niveditas98, Roberto Sassu
This patch set aims at solving the following use case: appraise files from
the initial ram disk. To do that, IMA checks the signature/hash from the
security.ima xattr. Unfortunately, this use case cannot be implemented
currently, as the CPIO format does not support xattrs.
This proposal consists in including file metadata as additional files named
METADATA!!!, for each file added to the ram disk. The CPIO parser in the
kernel recognizes these special files from the file name, and calls the
appropriate parser to add metadata to the previously extracted file. It has
been proposed to use bit 17:16 of the file mode as a way to recognize files
with metadata, but both the kernel and the cpio tool declare the file mode
as unsigned short.
The difference from v2, v3 (https://lkml.org/lkml/2019/5/9/230,
https://lkml.org/lkml/2019/5/17/466) is that file metadata are stored in
separate files instead of a single file. Given that files with metadata
must immediately follow the files metadata will be added to, image
generators have to be modified in this version.
The difference from v1 (https://lkml.org/lkml/2018/11/22/1182) is that
all files have the same name. The file metadata are added to is always the
previous one, and the image generator in user space will make sure that
files are in the correct sequence.
The difference with another proposal
(https://lore.kernel.org/patchwork/cover/888071/) is that xattrs can be
included in an image without changing the image format. Files with metadata
will appear as regular files. It will be task of the parser in the kernel
to process them.
This patch set extends the format of data defined in patch 9/15 of the last
proposal. It adds header version and type, so that new formats can be
defined and arbitrary metadata types can be processed.
The changes introduced by this patch set don't cause any compatibility
issue: kernels without the metadata parser simply extract the special files
and don't process metadata; kernels with the metadata parser don't process
metadata if the special files are not included in the image.
From the kernel space perspective, backporting this functionality to older
kernels should be very easy. It is sufficient to add two calls to the new
function do_process_metadata() in do_copy(), and to check the file name in
do_name(). From the user space perspective, unlike the previous version of
the patch set, it is required to modify the image generators in order to
include metadata as separate files.
Changelog
v3:
- include file metadata as separate files named METADATA!!!
- add the possibility to include in the ram disk arbitrary metadata types
v2:
- replace ksys_lsetxattr() with kern_path() and vfs_setxattr()
(suggested by Jann Horn)
- replace ksys_open()/ksys_read()/ksys_close() with
filp_open()/kernel_read()/fput()
(suggested by Jann Horn)
- use path variable instead of name_buf in do_readxattrs()
- set last byte of str to 0 in do_readxattrs()
- call do_readxattrs() in do_name() before replacing an existing
.xattr-list
- pass pathname to do_setxattrs()
v1:
- move xattr unmarshaling to CPIO parser
Mimi Zohar (1):
initramfs: add file metadata
Roberto Sassu (2):
initramfs: read metadata from special file METADATA!!!
gen_init_cpio: add support for file metadata
include/linux/initramfs.h | 21 ++++++
init/initramfs.c | 137 +++++++++++++++++++++++++++++++++++++-
usr/Kconfig | 8 +++
usr/Makefile | 4 +-
usr/gen_init_cpio.c | 137 ++++++++++++++++++++++++++++++++++++--
usr/gen_initramfs_list.sh | 10 ++-
6 files changed, 305 insertions(+), 12 deletions(-)
create mode 100644 include/linux/initramfs.h
--
2.17.1
^ permalink raw reply
* [PATCH v4 1/3] initramfs: add file metadata
From: Roberto Sassu @ 2019-05-23 12:18 UTC (permalink / raw)
To: viro
Cc: linux-security-module, linux-integrity, initramfs, linux-api,
linux-fsdevel, linux-kernel, bug-cpio, zohar, silviu.vlasceanu,
dmitry.kasatkin, takondra, kamensky, hpa, arnd, rob,
james.w.mcmechan, niveditas98, Roberto Sassu
In-Reply-To: <20190523121803.21638-1-roberto.sassu@huawei.com>
From: Mimi Zohar <zohar@linux.vnet.ibm.com>
This patch adds metadata to a file from a supplied buffer. The buffer might
contains multiple metadata records. The format of each record is:
<metadata len (ASCII, 8 chars)><version><type><metadata>
For now, only the TYPE_XATTR metadata type is supported. The specific
format of this metadata type is:
<xattr #N name>\0<xattr #N value>
[kamensky: fixed restoring of xattrs for symbolic links by using
sys_lsetxattr() instead of sys_setxattr()]
[sassu: removed state management, kept only do_setxattrs(), added support
for generic file metadata, replaced sys_lsetxattr() with
vfs_setxattr(), added check for entry_size, added check for
hdr->c_size, replaced strlen() with strnlen(); moved do_setxattrs()
before do_name()]
Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Victor Kamensky <kamensky@cisco.com>
Signed-off-by: Taras Kondratiuk <takondra@cisco.com>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
include/linux/initramfs.h | 21 ++++++++++
init/initramfs.c | 88 ++++++++++++++++++++++++++++++++++++++-
2 files changed, 107 insertions(+), 2 deletions(-)
create mode 100644 include/linux/initramfs.h
diff --git a/include/linux/initramfs.h b/include/linux/initramfs.h
new file mode 100644
index 000000000000..2f8cee441236
--- /dev/null
+++ b/include/linux/initramfs.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * include/linux/initramfs.h
+ *
+ * Include file for file metadata in the initial ram disk.
+ */
+#ifndef _LINUX_INITRAMFS_H
+#define _LINUX_INITRAMFS_H
+
+#define METADATA_FILENAME "METADATA!!!"
+
+enum metadata_types { TYPE_NONE, TYPE_XATTR, TYPE__LAST };
+
+struct metadata_hdr {
+ char c_size[8]; /* total size including c_size field */
+ char c_version; /* header version */
+ char c_type; /* metadata type */
+ char c_metadata[]; /* metadata */
+} __packed;
+
+#endif /*LINUX_INITRAMFS_H*/
diff --git a/init/initramfs.c b/init/initramfs.c
index 178130fd61c2..5de396a6aac0 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -10,6 +10,9 @@
#include <linux/syscalls.h>
#include <linux/utime.h>
#include <linux/file.h>
+#include <linux/namei.h>
+#include <linux/xattr.h>
+#include <linux/initramfs.h>
static ssize_t __init xwrite(int fd, const char *p, size_t count)
{
@@ -146,7 +149,7 @@ static __initdata time64_t mtime;
static __initdata unsigned long ino, major, minor, nlink;
static __initdata umode_t mode;
-static __initdata unsigned long body_len, name_len;
+static __initdata unsigned long body_len, name_len, metadata_len;
static __initdata uid_t uid;
static __initdata gid_t gid;
static __initdata unsigned rdev;
@@ -218,7 +221,7 @@ static void __init read_into(char *buf, unsigned size, enum state next)
}
}
-static __initdata char *header_buf, *symlink_buf, *name_buf;
+static __initdata char *header_buf, *symlink_buf, *name_buf, *metadata_buf;
static int __init do_start(void)
{
@@ -315,6 +318,87 @@ static int __init maybe_link(void)
return 0;
}
+static int __init do_setxattrs(char *pathname, char *buf, size_t size)
+{
+ struct path path;
+ char *xattr_name, *xattr_value;
+ size_t xattr_name_size, xattr_value_size;
+ int ret;
+
+ xattr_name = buf;
+ xattr_name_size = strnlen(xattr_name, size);
+ if (xattr_name_size == size) {
+ error("malformed xattrs");
+ return -EINVAL;
+ }
+
+ xattr_value = xattr_name + xattr_name_size + 1;
+ xattr_value_size = buf + size - xattr_value;
+
+ ret = kern_path(pathname, 0, &path);
+ if (!ret) {
+ ret = vfs_setxattr(path.dentry, xattr_name, xattr_value,
+ xattr_value_size, 0);
+
+ path_put(&path);
+ }
+
+ pr_debug("%s: %s size: %lu val: %s (ret: %d)\n", pathname,
+ xattr_name, xattr_value_size, xattr_value, ret);
+
+ return ret;
+}
+
+static int __init __maybe_unused do_parse_metadata(char *pathname)
+{
+ char *buf = metadata_buf;
+ char *bufend = metadata_buf + metadata_len;
+ struct metadata_hdr *hdr;
+ char str[sizeof(hdr->c_size) + 1];
+ size_t entry_size;
+
+ if (!metadata_len)
+ return 0;
+
+ str[sizeof(hdr->c_size)] = 0;
+
+ while (buf < bufend) {
+ int ret;
+
+ if (buf + sizeof(*hdr) > bufend) {
+ error("malformed metadata");
+ break;
+ }
+
+ hdr = (struct metadata_hdr *)buf;
+ if (hdr->c_version != 1) {
+ pr_debug("Unsupported header version\n");
+ break;
+ }
+
+ memcpy(str, hdr->c_size, sizeof(hdr->c_size));
+ ret = kstrtoul(str, 16, &entry_size);
+ if (ret || buf + entry_size > bufend || !entry_size) {
+ error("malformed xattrs");
+ break;
+ }
+
+ switch (hdr->c_type) {
+ case TYPE_XATTR:
+ do_setxattrs(pathname, buf + sizeof(*hdr),
+ entry_size - sizeof(*hdr));
+ break;
+ default:
+ pr_debug("Unsupported metadata type\n");
+ break;
+ }
+
+ buf += entry_size;
+ }
+
+ return 0;
+}
+
static __initdata int wfd;
static int __init do_name(void)
--
2.17.1
^ permalink raw reply related
* [PATCH v4 2/3] initramfs: read metadata from special file METADATA!!!
From: Roberto Sassu @ 2019-05-23 12:18 UTC (permalink / raw)
To: viro
Cc: linux-security-module, linux-integrity, initramfs, linux-api,
linux-fsdevel, linux-kernel, bug-cpio, zohar, silviu.vlasceanu,
dmitry.kasatkin, takondra, kamensky, hpa, arnd, rob,
james.w.mcmechan, niveditas98, Roberto Sassu
In-Reply-To: <20190523121803.21638-1-roberto.sassu@huawei.com>
Instead of changing the CPIO format, metadata are parsed from regular files
with special name 'METADATA!!!'. This file immediately follows the file
metadata are added to.
This patch checks if the file being extracted has the special name and, if
yes, creates a buffer with the content of that file and calls
do_parse_metadata() to parse metadata from the buffer.
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Reported-by: kbuild test robot <lkp@intel.com>
---
init/initramfs.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 48 insertions(+), 1 deletion(-)
diff --git a/init/initramfs.c b/init/initramfs.c
index 5de396a6aac0..862c03123de8 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -222,6 +222,7 @@ static void __init read_into(char *buf, unsigned size, enum state next)
}
static __initdata char *header_buf, *symlink_buf, *name_buf, *metadata_buf;
+static __initdata char *metadata_buf_ptr, *previous_name_buf;
static int __init do_start(void)
{
@@ -400,6 +401,7 @@ static int __init __maybe_unused do_parse_metadata(char *pathname)
}
static __initdata int wfd;
+static __initdata int metadata;
static int __init do_name(void)
{
@@ -408,6 +410,10 @@ static int __init do_name(void)
if (strcmp(collected, "TRAILER!!!") == 0) {
free_hash();
return 0;
+ } else if (strcmp(collected, METADATA_FILENAME) == 0) {
+ metadata = 1;
+ } else {
+ memcpy(previous_name_buf, collected, strlen(collected) + 1);
}
clean_path(collected, mode);
if (S_ISREG(mode)) {
@@ -444,11 +450,47 @@ static int __init do_name(void)
return 0;
}
+static int __init do_process_metadata(char *buf, int len, bool last)
+{
+ int ret = 0;
+
+ if (!metadata_buf) {
+ metadata_buf_ptr = metadata_buf = kmalloc(body_len, GFP_KERNEL);
+ if (!metadata_buf_ptr) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ metadata_len = body_len;
+ }
+
+ if (metadata_buf_ptr + len > metadata_buf + metadata_len) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ memcpy(metadata_buf_ptr, buf, len);
+ metadata_buf_ptr += len;
+
+ if (last)
+ do_parse_metadata(previous_name_buf);
+out:
+ if (ret < 0 || last) {
+ kfree(metadata_buf);
+ metadata_buf = NULL;
+ metadata = 0;
+ }
+
+ return ret;
+}
+
static int __init do_copy(void)
{
if (byte_count >= body_len) {
if (xwrite(wfd, victim, body_len) != body_len)
error("write error");
+ if (metadata)
+ do_process_metadata(victim, body_len, true);
ksys_close(wfd);
do_utime(vcollected, mtime);
kfree(vcollected);
@@ -458,6 +500,8 @@ static int __init do_copy(void)
} else {
if (xwrite(wfd, victim, byte_count) != byte_count)
error("write error");
+ if (metadata)
+ do_process_metadata(victim, byte_count, false);
body_len -= byte_count;
eat(byte_count);
return 1;
@@ -467,6 +511,7 @@ static int __init do_copy(void)
static int __init do_symlink(void)
{
collected[N_ALIGN(name_len) + body_len] = '\0';
+ memcpy(previous_name_buf, collected, strlen(collected) + 1);
clean_path(collected, 0);
ksys_symlink(collected + N_ALIGN(name_len), collected);
ksys_lchown(collected, uid, gid);
@@ -534,8 +579,9 @@ static char * __init unpack_to_rootfs(char *buf, unsigned long len)
header_buf = kmalloc(110, GFP_KERNEL);
symlink_buf = kmalloc(PATH_MAX + N_ALIGN(PATH_MAX) + 1, GFP_KERNEL);
name_buf = kmalloc(N_ALIGN(PATH_MAX), GFP_KERNEL);
+ previous_name_buf = kmalloc(N_ALIGN(PATH_MAX), GFP_KERNEL);
- if (!header_buf || !symlink_buf || !name_buf)
+ if (!header_buf || !symlink_buf || !name_buf || !previous_name_buf)
panic("can't allocate buffers");
state = Start;
@@ -580,6 +626,7 @@ static char * __init unpack_to_rootfs(char *buf, unsigned long len)
len -= my_inptr;
}
dir_utime();
+ kfree(previous_name_buf);
kfree(name_buf);
kfree(symlink_buf);
kfree(header_buf);
--
2.17.1
^ permalink raw reply related
* [PATCH v4 3/3] gen_init_cpio: add support for file metadata
From: Roberto Sassu @ 2019-05-23 12:18 UTC (permalink / raw)
To: viro
Cc: linux-security-module, linux-integrity, initramfs, linux-api,
linux-fsdevel, linux-kernel, bug-cpio, zohar, silviu.vlasceanu,
dmitry.kasatkin, takondra, kamensky, hpa, arnd, rob,
james.w.mcmechan, niveditas98, Roberto Sassu
In-Reply-To: <20190523121803.21638-1-roberto.sassu@huawei.com>
This patch adds support for file metadata (only TYPE_XATTR metadata type).
gen_init_cpio has been modified to read xattrs from files that will be
added to the image and to include file metadata as separate files with the
special name 'METADATA!!!'.
This behavior can be selected by setting the desired file metadata type as
value for CONFIG_INITRAMFS_FILE_METADATA.
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
usr/Kconfig | 8 +++
usr/Makefile | 4 +-
usr/gen_init_cpio.c | 137 ++++++++++++++++++++++++++++++++++++--
usr/gen_initramfs_list.sh | 10 ++-
4 files changed, 150 insertions(+), 9 deletions(-)
diff --git a/usr/Kconfig b/usr/Kconfig
index 43658b8a975e..8d9f54a16440 100644
--- a/usr/Kconfig
+++ b/usr/Kconfig
@@ -233,3 +233,11 @@ config INITRAMFS_COMPRESSION
default ".lzma" if RD_LZMA
default ".bz2" if RD_BZIP2
default ""
+
+config INITRAMFS_FILE_METADATA
+ string "File metadata type"
+ default ""
+ help
+ Specify xattr to include xattrs in the image.
+
+ If you are not sure, leave it blank.
diff --git a/usr/Makefile b/usr/Makefile
index 4a70ae43c9cb..7d5eb3c7b713 100644
--- a/usr/Makefile
+++ b/usr/Makefile
@@ -29,7 +29,9 @@ ramfs-input := $(if $(filter-out "",$(CONFIG_INITRAMFS_SOURCE)), \
$(shell echo $(CONFIG_INITRAMFS_SOURCE)),-d)
ramfs-args := \
$(if $(CONFIG_INITRAMFS_ROOT_UID), -u $(CONFIG_INITRAMFS_ROOT_UID)) \
- $(if $(CONFIG_INITRAMFS_ROOT_GID), -g $(CONFIG_INITRAMFS_ROOT_GID))
+ $(if $(CONFIG_INITRAMFS_ROOT_GID), -g $(CONFIG_INITRAMFS_ROOT_GID)) \
+ $(if $(filter-out "",$(CONFIG_INITRAMFS_FILE_METADATA)), \
+ -e $(CONFIG_INITRAMFS_FILE_METADATA))
# $(datafile_d_y) is used to identify all files included
# in initramfs and to detect if any files are added/removed.
diff --git a/usr/gen_init_cpio.c b/usr/gen_init_cpio.c
index 03b21189d58b..e93cb1093e77 100644
--- a/usr/gen_init_cpio.c
+++ b/usr/gen_init_cpio.c
@@ -3,6 +3,7 @@
#include <stdlib.h>
#include <sys/types.h>
#include <sys/stat.h>
+#include <sys/xattr.h>
#include <string.h>
#include <unistd.h>
#include <time.h>
@@ -10,6 +11,7 @@
#include <errno.h>
#include <ctype.h>
#include <limits.h>
+#include "../include/linux/initramfs.h"
/*
* Original work by Jeff Garzik
@@ -24,6 +26,115 @@
static unsigned int offset;
static unsigned int ino = 721;
static time_t default_mtime;
+static char metadata_path[] = "/tmp/cpio-metadata-XXXXXX";
+static int metadata_fd = -1;
+
+static enum metadata_types parse_metadata_type(char *arg)
+{
+ static char *metadata_type_str[TYPE__LAST] = {
+ [TYPE_NONE] = "none",
+ [TYPE_XATTR] = "xattr",
+ };
+ int i;
+
+ for (i = 0; i < TYPE__LAST; i++)
+ if (!strcmp(metadata_type_str[i], arg))
+ return i;
+
+ return TYPE_NONE;
+}
+
+static int cpio_mkfile(const char *name, const char *location,
+ unsigned int mode, uid_t uid, gid_t gid,
+ unsigned int nlinks);
+
+static int write_xattrs(const char *path)
+{
+ struct metadata_hdr hdr = { .c_version = 1, .c_type = TYPE_XATTR };
+ char str[sizeof(hdr.c_size) + 1];
+ char *xattr_list, *list_ptr, *xattr_value;
+ ssize_t list_len, name_len, value_len, len;
+ int ret = -EINVAL;
+
+ if (metadata_fd < 0)
+ return 0;
+
+ if (path == metadata_path)
+ return 0;
+
+ list_len = listxattr(path, NULL, 0);
+ if (list_len <= 0)
+ return 0;
+
+ list_ptr = xattr_list = malloc(list_len);
+ if (!list_ptr) {
+ fprintf(stderr, "out of memory\n");
+ return ret;
+ }
+
+ len = listxattr(path, xattr_list, list_len);
+ if (len != list_len)
+ goto out;
+
+ if (ftruncate(metadata_fd, 0))
+ goto out;
+
+ lseek(metadata_fd, 0, SEEK_SET);
+
+ while (list_ptr < xattr_list + list_len) {
+ name_len = strlen(list_ptr);
+
+ value_len = getxattr(path, list_ptr, NULL, 0);
+ if (value_len < 0) {
+ fprintf(stderr, "cannot get xattrs\n");
+ break;
+ }
+
+ if (value_len) {
+ xattr_value = malloc(value_len);
+ if (!xattr_value) {
+ fprintf(stderr, "out of memory\n");
+ break;
+ }
+ } else {
+ xattr_value = NULL;
+ }
+
+ len = getxattr(path, list_ptr, xattr_value, value_len);
+ if (len != value_len)
+ break;
+
+ snprintf(str, sizeof(str), "%.8lx",
+ sizeof(hdr) + name_len + 1 + value_len);
+
+ memcpy(hdr.c_size, str, sizeof(hdr.c_size));
+
+ if (write(metadata_fd, &hdr, sizeof(hdr)) != sizeof(hdr))
+ break;
+
+ if (write(metadata_fd, list_ptr, name_len + 1) != name_len + 1)
+ break;
+
+ if (write(metadata_fd, xattr_value, value_len) != value_len)
+ break;
+
+ if (fsync(metadata_fd))
+ break;
+
+ list_ptr += name_len + 1;
+ free(xattr_value);
+ xattr_value = NULL;
+ }
+
+ free(xattr_value);
+out:
+ free(xattr_list);
+
+ if (list_ptr != xattr_list + list_len)
+ return ret;
+
+ return cpio_mkfile(METADATA_FILENAME, metadata_path, S_IFREG, 0, 0, 1);
+}
struct file_handler {
const char *type;
@@ -128,7 +239,7 @@ static int cpio_mkslink(const char *name, const char *target,
push_pad();
push_string(target);
push_pad();
- return 0;
+ return write_xattrs(name);
}
static int cpio_mkslink_line(const char *line)
@@ -174,7 +285,7 @@ static int cpio_mkgeneric(const char *name, unsigned int mode,
0); /* chksum */
push_hdr(s);
push_rest(name);
- return 0;
+ return write_xattrs(name);
}
enum generic_types {
@@ -268,7 +379,7 @@ static int cpio_mknod(const char *name, unsigned int mode,
0); /* chksum */
push_hdr(s);
push_rest(name);
- return 0;
+ return write_xattrs(name);
}
static int cpio_mknod_line(const char *line)
@@ -372,8 +483,7 @@ static int cpio_mkfile(const char *name, const char *location,
name += namesize;
}
ino++;
- rc = 0;
-
+ rc = write_xattrs(location);
error:
if (filebuf) free(filebuf);
if (file >= 0) close(file);
@@ -526,10 +636,11 @@ int main (int argc, char *argv[])
int ec = 0;
int line_nr = 0;
const char *filename;
+ enum metadata_types metadata_type = TYPE_NONE;
default_mtime = time(NULL);
while (1) {
- int opt = getopt(argc, argv, "t:h");
+ int opt = getopt(argc, argv, "t:e:h");
char *invalid;
if (opt == -1)
@@ -544,6 +655,9 @@ int main (int argc, char *argv[])
exit(1);
}
break;
+ case 'e':
+ metadata_type = parse_metadata_type(optarg);
+ break;
case 'h':
case '?':
usage(argv[0]);
@@ -565,6 +679,14 @@ int main (int argc, char *argv[])
exit(1);
}
+ if (metadata_type != TYPE_NONE) {
+ metadata_fd = mkstemp(metadata_path);
+ if (metadata_fd < 0) {
+ fprintf(stderr, "cannot create temporary file\n");
+ exit(1);
+ }
+ }
+
while (fgets(line, LINE_SIZE, cpio_list)) {
int type_idx;
size_t slen = strlen(line);
@@ -620,5 +742,8 @@ int main (int argc, char *argv[])
if (ec == 0)
cpio_trailer();
+ if (metadata_type != TYPE_NONE)
+ close(metadata_fd);
+
exit(ec);
}
diff --git a/usr/gen_initramfs_list.sh b/usr/gen_initramfs_list.sh
index 0aad760fcd8c..0907a4043da9 100755
--- a/usr/gen_initramfs_list.sh
+++ b/usr/gen_initramfs_list.sh
@@ -15,7 +15,7 @@ set -e
usage() {
cat << EOF
Usage:
-$0 [-o <file>] [-u <uid>] [-g <gid>] {-d | <cpio_source>} ...
+$0 [-o <file>] [-u <uid>] [-g <gid>] {-d | <cpio_source>} [-e <type>] ...
-o <file> Create compressed initramfs file named <file> using
gen_init_cpio and compressor depending on the extension
-u <uid> User ID to map to user ID 0 (root).
@@ -28,6 +28,7 @@ $0 [-o <file>] [-u <uid>] [-g <gid>] {-d | <cpio_source>} ...
If <cpio_source> is a .cpio file it will be used
as direct input to initramfs.
-d Output the default cpio list.
+ -e <type> File metadata type to include in the cpio archive.
All options except -o and -l may be repeated and are interpreted
sequentially and immediately. -u and -g states are preserved across
@@ -283,6 +284,10 @@ while [ $# -gt 0 ]; do
default_list="$arg"
${dep_list}default_initramfs
;;
+ "-e") # file metadata type
+ metadata_arg="-e $1"
+ shift
+ ;;
"-h")
usage
exit 0
@@ -312,7 +317,8 @@ if [ ! -z ${output_file} ]; then
fi
fi
cpio_tfile="$(mktemp ${TMPDIR:-/tmp}/cpiofile.XXXXXX)"
- usr/gen_init_cpio $timestamp ${cpio_list} > ${cpio_tfile}
+ usr/gen_init_cpio $metadata_arg $timestamp \
+ ${cpio_list} > ${cpio_tfile}
else
cpio_tfile=${cpio_file}
fi
--
2.17.1
^ permalink raw reply related
* Re: [PATCH V7 0/4] Add support for crypto agile logs
From: Jarkko Sakkinen @ 2019-05-23 12:26 UTC (permalink / raw)
To: Matthew Garrett, jmorris
Cc: linux-integrity, peterhuewe, jgg, roberto.sassu, linux-efi,
linux-security-module, linux-kernel, tweek, bsz
In-Reply-To: <20190523121449.GA9997@linux.intel.com>
On Thu, May 23, 2019 at 03:14:49PM +0300, Jarkko Sakkinen wrote:
> On Mon, May 20, 2019 at 01:54:57PM -0700, Matthew Garrett wrote:
> > Identical to previous version except without the KSAN workaround - Ard
> > has a better solution for that.
>
>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Only applied to my master at this point becaues the patches from
my earlier PRs are not yet mirrored to security/next-general.
/Jarkko
^ permalink raw reply
* [PATCH v3 0/3] RFC: add init_on_alloc/init_on_free boot options
From: Alexander Potapenko @ 2019-05-23 12:42 UTC (permalink / raw)
To: akpm, cl, keescook; +Cc: kernel-hardening, linux-mm, linux-security-module
Provide init_on_alloc and init_on_free boot options.
These are aimed at preventing possible information leaks and making the
control-flow bugs that depend on uninitialized values more deterministic.
Enabling either of the options guarantees that the memory returned by the
page allocator and SL[AOU]B is initialized with zeroes.
Enabling init_on_free also guarantees that pages and heap objects are
initialized right after they're freed, so it won't be possible to access
stale data by using a dangling pointer.
As suggested by Michal Hocko, right now we don't let the heap users to
disable initialization for certain allocations. There's not enough
evidence that doing so can speed up real-life cases, and introducing
ways to opt-out may result in things going out of control.
Alexander Potapenko (3):
mm: security: introduce init_on_alloc=1 and init_on_free=1 boot
options
mm: init: report memory auto-initialization features at boot time
lib: introduce test_meminit module
.../admin-guide/kernel-parameters.txt | 8 +
drivers/infiniband/core/uverbs_ioctl.c | 2 +-
include/linux/mm.h | 22 ++
init/main.c | 24 ++
kernel/kexec_core.c | 2 +-
lib/Kconfig.debug | 8 +
lib/Makefile | 1 +
lib/test_meminit.c | 208 ++++++++++++++++++
mm/dmapool.c | 2 +-
mm/page_alloc.c | 63 +++++-
mm/slab.c | 16 +-
mm/slab.h | 16 ++
mm/slob.c | 22 +-
mm/slub.c | 27 ++-
net/core/sock.c | 2 +-
security/Kconfig.hardening | 14 ++
16 files changed, 416 insertions(+), 21 deletions(-)
create mode 100644 lib/test_meminit.c
---
v3: dropped __GFP_NO_AUTOINIT patches
--
2.21.0.1020.gf2820cf01a-goog
^ permalink raw reply
* [PATCH v3 1/3] mm: security: introduce init_on_alloc=1 and init_on_free=1 boot options
From: Alexander Potapenko @ 2019-05-23 12:42 UTC (permalink / raw)
To: akpm, cl, keescook
Cc: kernel-hardening, linux-mm, linux-security-module,
Masahiro Yamada, Michal Hocko, James Morris, Serge E. Hallyn,
Nick Desaulniers, Kostya Serebryany, Dmitry Vyukov, Sandeep Patil,
Laura Abbott, Randy Dunlap, Jann Horn, Mark Rutland
In-Reply-To: <20190523124216.40208-1-glider@google.com>
The new options are needed to prevent possible information leaks and
make control-flow bugs that depend on uninitialized values more
deterministic.
init_on_alloc=1 makes the kernel initialize newly allocated pages and heap
objects with zeroes. Initialization is done at allocation time at the
places where checks for __GFP_ZERO are performed.
init_on_free=1 makes the kernel initialize freed pages and heap objects
with zeroes upon their deletion. This helps to ensure sensitive data
doesn't leak via use-after-free accesses.
Both init_on_alloc=1 and init_on_free=1 guarantee that the allocator
returns zeroed memory. The only exception is slab caches with
constructors. Those are never zero-initialized to preserve their semantics.
For SLOB allocator init_on_free=1 also implies init_on_alloc=1 behavior,
i.e. objects are zeroed at both allocation and deallocation time.
This is done because SLOB may otherwise return multiple freelist pointers
in the allocated object. For SLAB and SLUB enabling either init_on_alloc
or init_on_free leads to one-time initialization of the object.
Both init_on_alloc and init_on_free default to zero, but those defaults
can be overridden with CONFIG_INIT_ON_ALLOC_DEFAULT_ON and
CONFIG_INIT_ON_FREE_DEFAULT_ON.
Slowdown for the new features compared to init_on_free=0,
init_on_alloc=0:
hackbench, init_on_free=1: +7.62% sys time (st.err 0.74%)
hackbench, init_on_alloc=1: +7.75% sys time (st.err 2.14%)
Linux build with -j12, init_on_free=1: +8.38% wall time (st.err 0.39%)
Linux build with -j12, init_on_free=1: +24.42% sys time (st.err 0.52%)
Linux build with -j12, init_on_alloc=1: -0.13% wall time (st.err 0.42%)
Linux build with -j12, init_on_alloc=1: +0.57% sys time (st.err 0.40%)
The slowdown for init_on_free=0, init_on_alloc=0 compared to the
baseline is within the standard error.
The new features are also going to pave the way for hardware memory
tagging (e.g. arm64's MTE), which will require both on_alloc and on_free
hooks to set the tags for heap objects. With MTE, tagging will have the
same cost as memory initialization.
Although init_on_free is rather costly, there are paranoid use-cases where
in-memory data lifetime is desired to be minimized. There are various
arguments for/against the realism of the associated threat models, but
given that we'll need the infrastructre for MTE anyway, and there are
people who want wipe-on-free behavior no matter what the performance cost,
it seems reasonable to include it in this series.
Signed-off-by: Alexander Potapenko <glider@google.com>
To: Andrew Morton <akpm@linux-foundation.org>
To: Christoph Lameter <cl@linux.com>
To: Kees Cook <keescook@chromium.org>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Kostya Serebryany <kcc@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Sandeep Patil <sspatil@android.com>
Cc: Laura Abbott <labbott@redhat.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Jann Horn <jannh@google.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: linux-mm@kvack.org
Cc: linux-security-module@vger.kernel.org
Cc: kernel-hardening@lists.openwall.com
---
v2:
- unconditionally initialize pages in kernel_init_free_pages()
- comment from Randy Dunlap: drop 'default false' lines from Kconfig.hardening
v3:
- don't call kernel_init_free_pages() from memblock_free_pages()
- adopted some Kees' comments for the patch description
---
.../admin-guide/kernel-parameters.txt | 8 +++
drivers/infiniband/core/uverbs_ioctl.c | 2 +-
include/linux/mm.h | 22 +++++++
kernel/kexec_core.c | 2 +-
mm/dmapool.c | 2 +-
mm/page_alloc.c | 63 ++++++++++++++++---
mm/slab.c | 16 ++++-
mm/slab.h | 16 +++++
mm/slob.c | 22 ++++++-
mm/slub.c | 27 ++++++--
net/core/sock.c | 2 +-
security/Kconfig.hardening | 14 +++++
12 files changed, 175 insertions(+), 21 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 52e6fbb042cc..68fb6fa41cc1 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1673,6 +1673,14 @@
initrd= [BOOT] Specify the location of the initial ramdisk
+ init_on_alloc= [MM] Fill newly allocated pages and heap objects with
+ zeroes.
+ Format: 0 | 1
+ Default set by CONFIG_INIT_ON_ALLOC_DEFAULT_ON.
+ init_on_free= [MM] Fill freed pages and heap objects with zeroes.
+ Format: 0 | 1
+ Default set by CONFIG_INIT_ON_FREE_DEFAULT_ON.
+
init_pkru= [x86] Specify the default memory protection keys rights
register contents for all processes. 0x55555554 by
default (disallow access to all but pkey 0). Can
diff --git a/drivers/infiniband/core/uverbs_ioctl.c b/drivers/infiniband/core/uverbs_ioctl.c
index 829b0c6944d8..61758201d9b2 100644
--- a/drivers/infiniband/core/uverbs_ioctl.c
+++ b/drivers/infiniband/core/uverbs_ioctl.c
@@ -127,7 +127,7 @@ __malloc void *_uverbs_alloc(struct uverbs_attr_bundle *bundle, size_t size,
res = (void *)pbundle->internal_buffer + pbundle->internal_used;
pbundle->internal_used =
ALIGN(new_used, sizeof(*pbundle->internal_buffer));
- if (flags & __GFP_ZERO)
+ if (want_init_on_alloc(flags))
memset(res, 0, size);
return res;
}
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 0e8834ac32b7..7733a341c0c4 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2685,6 +2685,28 @@ static inline void kernel_poison_pages(struct page *page, int numpages,
int enable) { }
#endif
+#ifdef CONFIG_INIT_ON_ALLOC_DEFAULT_ON
+DECLARE_STATIC_KEY_TRUE(init_on_alloc);
+#else
+DECLARE_STATIC_KEY_FALSE(init_on_alloc);
+#endif
+static inline bool want_init_on_alloc(gfp_t flags)
+{
+ if (static_branch_unlikely(&init_on_alloc))
+ return true;
+ return flags & __GFP_ZERO;
+}
+
+#ifdef CONFIG_INIT_ON_FREE_DEFAULT_ON
+DECLARE_STATIC_KEY_TRUE(init_on_free);
+#else
+DECLARE_STATIC_KEY_FALSE(init_on_free);
+#endif
+static inline bool want_init_on_free(void)
+{
+ return static_branch_unlikely(&init_on_free);
+}
+
extern bool _debug_pagealloc_enabled;
static inline bool debug_pagealloc_enabled(void)
diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index fd5c95ff9251..2f75dd0d0d81 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -315,7 +315,7 @@ static struct page *kimage_alloc_pages(gfp_t gfp_mask, unsigned int order)
arch_kexec_post_alloc_pages(page_address(pages), count,
gfp_mask);
- if (gfp_mask & __GFP_ZERO)
+ if (want_init_on_alloc(gfp_mask))
for (i = 0; i < count; i++)
clear_highpage(pages + i);
}
diff --git a/mm/dmapool.c b/mm/dmapool.c
index 76a160083506..493d151067cb 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -381,7 +381,7 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
#endif
spin_unlock_irqrestore(&pool->lock, flags);
- if (mem_flags & __GFP_ZERO)
+ if (want_init_on_alloc(mem_flags))
memset(retval, 0, pool->size);
return retval;
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 3b13d3914176..14ded6620aa0 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -135,6 +135,48 @@ unsigned long totalcma_pages __read_mostly;
int percpu_pagelist_fraction;
gfp_t gfp_allowed_mask __read_mostly = GFP_BOOT_MASK;
+#ifdef CONFIG_INIT_ON_ALLOC_DEFAULT_ON
+DEFINE_STATIC_KEY_TRUE(init_on_alloc);
+#else
+DEFINE_STATIC_KEY_FALSE(init_on_alloc);
+#endif
+#ifdef CONFIG_INIT_ON_FREE_DEFAULT_ON
+DEFINE_STATIC_KEY_TRUE(init_on_free);
+#else
+DEFINE_STATIC_KEY_FALSE(init_on_free);
+#endif
+
+static int __init early_init_on_alloc(char *buf)
+{
+ int ret;
+ bool bool_result;
+
+ if (!buf)
+ return -EINVAL;
+ ret = kstrtobool(buf, &bool_result);
+ if (bool_result)
+ static_branch_enable(&init_on_alloc);
+ else
+ static_branch_disable(&init_on_alloc);
+ return ret;
+}
+early_param("init_on_alloc", early_init_on_alloc);
+
+static int __init early_init_on_free(char *buf)
+{
+ int ret;
+ bool bool_result;
+
+ if (!buf)
+ return -EINVAL;
+ ret = kstrtobool(buf, &bool_result);
+ if (bool_result)
+ static_branch_enable(&init_on_free);
+ else
+ static_branch_disable(&init_on_free);
+ return ret;
+}
+early_param("init_on_free", early_init_on_free);
/*
* A cached value of the page's pageblock's migratetype, used when the page is
@@ -1089,6 +1131,14 @@ static int free_tail_pages_check(struct page *head_page, struct page *page)
return ret;
}
+static void kernel_init_free_pages(struct page *page, int numpages)
+{
+ int i;
+
+ for (i = 0; i < numpages; i++)
+ clear_highpage(page + i);
+}
+
static __always_inline bool free_pages_prepare(struct page *page,
unsigned int order, bool check_free)
{
@@ -1141,6 +1191,8 @@ static __always_inline bool free_pages_prepare(struct page *page,
}
arch_free_page(page, order);
kernel_poison_pages(page, 1 << order, 0);
+ if (want_init_on_free())
+ kernel_init_free_pages(page, 1 << order);
if (debug_pagealloc_enabled())
kernel_map_pages(page, 1 << order, 0);
@@ -2019,8 +2071,8 @@ static inline int check_new_page(struct page *page)
static inline bool free_pages_prezeroed(void)
{
- return IS_ENABLED(CONFIG_PAGE_POISONING_ZERO) &&
- page_poisoning_enabled();
+ return (IS_ENABLED(CONFIG_PAGE_POISONING_ZERO) &&
+ page_poisoning_enabled()) || want_init_on_free();
}
#ifdef CONFIG_DEBUG_VM
@@ -2074,13 +2126,10 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
static void prep_new_page(struct page *page, unsigned int order, gfp_t gfp_flags,
unsigned int alloc_flags)
{
- int i;
-
post_alloc_hook(page, order, gfp_flags);
- if (!free_pages_prezeroed() && (gfp_flags & __GFP_ZERO))
- for (i = 0; i < (1 << order); i++)
- clear_highpage(page + i);
+ if (!free_pages_prezeroed() && want_init_on_alloc(gfp_flags))
+ kernel_init_free_pages(page, 1 << order);
if (order && (gfp_flags & __GFP_COMP))
prep_compound_page(page, order);
diff --git a/mm/slab.c b/mm/slab.c
index 2915d912e89a..d42eb11f8f50 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -1853,6 +1853,14 @@ static bool set_objfreelist_slab_cache(struct kmem_cache *cachep,
cachep->num = 0;
+ /*
+ * If slab auto-initialization on free is enabled, store the freelist
+ * off-slab, so that its contents don't end up in one of the allocated
+ * objects.
+ */
+ if (unlikely(slab_want_init_on_free(cachep)))
+ return false;
+
if (cachep->ctor || flags & SLAB_TYPESAFE_BY_RCU)
return false;
@@ -3293,7 +3301,7 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
local_irq_restore(save_flags);
ptr = cache_alloc_debugcheck_after(cachep, flags, ptr, caller);
- if (unlikely(flags & __GFP_ZERO) && ptr)
+ if (unlikely(slab_want_init_on_alloc(flags, cachep)) && ptr)
memset(ptr, 0, cachep->object_size);
slab_post_alloc_hook(cachep, flags, 1, &ptr);
@@ -3350,7 +3358,7 @@ slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
objp = cache_alloc_debugcheck_after(cachep, flags, objp, caller);
prefetchw(objp);
- if (unlikely(flags & __GFP_ZERO) && objp)
+ if (unlikely(slab_want_init_on_alloc(flags, cachep)) && objp)
memset(objp, 0, cachep->object_size);
slab_post_alloc_hook(cachep, flags, 1, &objp);
@@ -3471,6 +3479,8 @@ void ___cache_free(struct kmem_cache *cachep, void *objp,
struct array_cache *ac = cpu_cache_get(cachep);
check_irq_off();
+ if (unlikely(slab_want_init_on_free(cachep)))
+ memset(objp, 0, cachep->object_size);
kmemleak_free_recursive(objp, cachep->flags);
objp = cache_free_debugcheck(cachep, objp, caller);
@@ -3558,7 +3568,7 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
cache_alloc_debugcheck_after_bulk(s, flags, size, p, _RET_IP_);
/* Clear memory outside IRQ disabled section */
- if (unlikely(flags & __GFP_ZERO))
+ if (unlikely(slab_want_init_on_alloc(flags, s)))
for (i = 0; i < size; i++)
memset(p[i], 0, s->object_size);
diff --git a/mm/slab.h b/mm/slab.h
index 43ac818b8592..24ae887359b8 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -524,4 +524,20 @@ static inline int cache_random_seq_create(struct kmem_cache *cachep,
static inline void cache_random_seq_destroy(struct kmem_cache *cachep) { }
#endif /* CONFIG_SLAB_FREELIST_RANDOM */
+static inline bool slab_want_init_on_alloc(gfp_t flags, struct kmem_cache *c)
+{
+ if (static_branch_unlikely(&init_on_alloc))
+ return !(c->ctor);
+ else
+ return flags & __GFP_ZERO;
+}
+
+static inline bool slab_want_init_on_free(struct kmem_cache *c)
+{
+ if (static_branch_unlikely(&init_on_free))
+ return !(c->ctor);
+ else
+ return false;
+}
+
#endif /* MM_SLAB_H */
diff --git a/mm/slob.c b/mm/slob.c
index 84aefd9b91ee..1b565ee7f479 100644
--- a/mm/slob.c
+++ b/mm/slob.c
@@ -212,6 +212,19 @@ static void slob_free_pages(void *b, int order)
free_pages((unsigned long)b, order);
}
+/*
+ * init_on_free=1 also implies initialization at allocation time.
+ * This is because newly allocated objects may contain freelist pointers
+ * somewhere in the middle.
+ */
+static inline bool slob_want_init_on_alloc(gfp_t flags, struct kmem_cache *c)
+{
+ if (static_branch_unlikely(&init_on_alloc) ||
+ static_branch_unlikely(&init_on_free))
+ return c ? (!c->ctor) : true;
+ return flags & __GFP_ZERO;
+}
+
/*
* slob_page_alloc() - Allocate a slob block within a given slob_page sp.
* @sp: Page to look in.
@@ -353,8 +366,6 @@ static void *slob_alloc(size_t size, gfp_t gfp, int align, int node)
BUG_ON(!b);
spin_unlock_irqrestore(&slob_lock, flags);
}
- if (unlikely(gfp & __GFP_ZERO))
- memset(b, 0, size);
return b;
}
@@ -389,6 +400,9 @@ static void slob_free(void *block, int size)
return;
}
+ if (unlikely(want_init_on_free()))
+ memset(block, 0, size);
+
if (!slob_page_free(sp)) {
/* This slob page is about to become partially free. Easy! */
sp->units = units;
@@ -484,6 +498,8 @@ __do_kmalloc_node(size_t size, gfp_t gfp, int node, unsigned long caller)
}
kmemleak_alloc(ret, size, 1, gfp);
+ if (unlikely(slob_want_init_on_alloc(gfp, 0)))
+ memset(ret, 0, size);
return ret;
}
@@ -582,6 +598,8 @@ static void *slob_alloc_node(struct kmem_cache *c, gfp_t flags, int node)
WARN_ON_ONCE(flags & __GFP_ZERO);
c->ctor(b);
}
+ if (unlikely(slob_want_init_on_alloc(flags, c)))
+ memset(b, 0, c->size);
kmemleak_alloc_recursive(b, c->size, 1, c->flags, flags);
return b;
diff --git a/mm/slub.c b/mm/slub.c
index cd04dbd2b5d0..5fcb3f71cf84 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1424,6 +1424,19 @@ static __always_inline bool slab_free_hook(struct kmem_cache *s, void *x)
static inline bool slab_free_freelist_hook(struct kmem_cache *s,
void **head, void **tail)
{
+
+ void *object;
+ void *next = *head;
+ void *old_tail = *tail ? *tail : *head;
+
+ if (slab_want_init_on_free(s))
+ do {
+ object = next;
+ next = get_freepointer(s, object);
+ memset(object, 0, s->size);
+ set_freepointer(s, object, next);
+ } while (object != old_tail);
+
/*
* Compiler cannot detect this function can be removed if slab_free_hook()
* evaluates to nothing. Thus, catch all relevant config debug options here.
@@ -1433,9 +1446,7 @@ static inline bool slab_free_freelist_hook(struct kmem_cache *s,
defined(CONFIG_DEBUG_OBJECTS_FREE) || \
defined(CONFIG_KASAN)
- void *object;
- void *next = *head;
- void *old_tail = *tail ? *tail : *head;
+ next = *head;
/* Head and tail of the reconstructed freelist */
*head = NULL;
@@ -2741,8 +2752,14 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s,
prefetch_freepointer(s, next_object);
stat(s, ALLOC_FASTPATH);
}
+ /*
+ * If the object has been wiped upon free, make sure it's fully
+ * initialized by zeroing out freelist pointer.
+ */
+ if (slab_want_init_on_free(s))
+ *(void **)object = 0;
- if (unlikely(gfpflags & __GFP_ZERO) && object)
+ if (unlikely(slab_want_init_on_alloc(gfpflags, s)) && object)
memset(object, 0, s->object_size);
slab_post_alloc_hook(s, gfpflags, 1, &object);
@@ -3163,7 +3180,7 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
local_irq_enable();
/* Clear memory outside IRQ disabled fastpath loop */
- if (unlikely(flags & __GFP_ZERO)) {
+ if (unlikely(slab_want_init_on_alloc(flags, s))) {
int j;
for (j = 0; j < i; j++)
diff --git a/net/core/sock.c b/net/core/sock.c
index 75b1c950b49f..9ceb90c875bc 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1602,7 +1602,7 @@ static struct sock *sk_prot_alloc(struct proto *prot, gfp_t priority,
sk = kmem_cache_alloc(slab, priority & ~__GFP_ZERO);
if (!sk)
return sk;
- if (priority & __GFP_ZERO)
+ if (want_init_on_alloc(priority))
sk_prot_clear_nulls(sk, prot->obj_size);
} else
sk = kmalloc(prot->obj_size, priority);
diff --git a/security/Kconfig.hardening b/security/Kconfig.hardening
index 0a1d4ca314f4..87883e3e3c2a 100644
--- a/security/Kconfig.hardening
+++ b/security/Kconfig.hardening
@@ -159,6 +159,20 @@ config STACKLEAK_RUNTIME_DISABLE
runtime to control kernel stack erasing for kernels built with
CONFIG_GCC_PLUGIN_STACKLEAK.
+config INIT_ON_ALLOC_DEFAULT_ON
+ bool "Set init_on_alloc=1 by default"
+ help
+ Enable init_on_alloc=1 by default, making the kernel initialize every
+ page and heap allocation with zeroes.
+ init_on_alloc can be overridden via command line.
+
+config INIT_ON_FREE_DEFAULT_ON
+ bool "Set init_on_free=1 by default"
+ help
+ Enable init_on_free=1 by default, making the kernel initialize freed
+ pages and slab memory with zeroes.
+ init_on_free can be overridden via command line.
+
endmenu
endmenu
--
2.21.0.1020.gf2820cf01a-goog
^ permalink raw reply related
* [PATCH 2/3] mm: init: report memory auto-initialization features at boot time
From: Alexander Potapenko @ 2019-05-23 12:42 UTC (permalink / raw)
To: akpm, cl, keescook
Cc: kernel-hardening, linux-mm, linux-security-module, Dmitry Vyukov,
James Morris, Jann Horn, Kostya Serebryany, Laura Abbott,
Mark Rutland, Masahiro Yamada, Matthew Wilcox, Nick Desaulniers,
Randy Dunlap, Sandeep Patil, Serge E. Hallyn, Souptick Joarder
In-Reply-To: <20190523124216.40208-1-glider@google.com>
Print the currently enabled stack and heap initialization modes.
The possible options for stack are:
- "all" for CONFIG_INIT_STACK_ALL;
- "byref_all" for CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL;
- "byref" for CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF;
- "__user" for CONFIG_GCC_PLUGIN_STRUCTLEAK_USER;
- "off" otherwise.
Depending on the values of init_on_alloc and init_on_free boottime
options we also report "heap alloc" and "heap free" as "on"/"off".
In the init_on_free mode initializing pages at boot time may take some
time, so print a notice about that as well.
Signed-off-by: Alexander Potapenko <glider@google.com>
Suggested-by: Kees Cook <keescook@chromium.org>
To: Andrew Morton <akpm@linux-foundation.org>
To: Christoph Lameter <cl@linux.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: James Morris <jmorris@namei.org>
Cc: Jann Horn <jannh@google.com>
Cc: Kostya Serebryany <kcc@google.com>
Cc: Laura Abbott <labbott@redhat.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Sandeep Patil <sspatil@android.com>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Souptick Joarder <jrdr.linux@gmail.com>
Cc: kernel-hardening@lists.openwall.com
Cc: linux-mm@kvack.org
Cc: linux-security-module@vger.kernel.org
---
init/main.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/init/main.c b/init/main.c
index 5a2c69b4d7b3..90f721c58e61 100644
--- a/init/main.c
+++ b/init/main.c
@@ -519,6 +519,29 @@ static inline void initcall_debug_enable(void)
}
#endif
+/* Report memory auto-initialization states for this boot. */
+void __init report_meminit(void)
+{
+ const char *stack;
+
+ if (IS_ENABLED(CONFIG_INIT_STACK_ALL))
+ stack = "all";
+ else if (IS_ENABLED(CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL))
+ stack = "byref_all";
+ else if (IS_ENABLED(CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF))
+ stack = "byref";
+ else if (IS_ENABLED(CONFIG_GCC_PLUGIN_STRUCTLEAK_USER))
+ stack = "__user";
+ else
+ stack = "off";
+
+ pr_info("mem auto-init: stack:%s, heap alloc:%s, heap free:%s\n",
+ stack, want_init_on_alloc(GFP_KERNEL) ? "on" : "off",
+ want_init_on_free() ? "on" : "off");
+ if (want_init_on_free())
+ pr_info("Clearing system memory may take some time...\n");
+}
+
/*
* Set up kernel memory allocators
*/
@@ -529,6 +552,7 @@ static void __init mm_init(void)
* bigger than MAX_ORDER unless SPARSEMEM.
*/
page_ext_init_flatmem();
+ report_meminit();
mem_init();
kmem_cache_init();
pgtable_init();
--
2.21.0.1020.gf2820cf01a-goog
^ permalink raw reply related
* [PATCH 3/3] lib: introduce test_meminit module
From: Alexander Potapenko @ 2019-05-23 12:42 UTC (permalink / raw)
To: akpm, cl, keescook
Cc: kernel-hardening, linux-mm, linux-security-module,
Nick Desaulniers, Kostya Serebryany, Dmitry Vyukov, Sandeep Patil,
Laura Abbott, Jann Horn
In-Reply-To: <20190523124216.40208-1-glider@google.com>
Add tests for heap and pagealloc initialization.
These can be used to check init_on_alloc and init_on_free implementations
as well as other approaches to initialization.
Expected test output in the case the kernel provides heap initialization
(e.g. when running with either init_on_alloc=1 or init_on_free=1):
test_meminit: all 10 tests in test_pages passed
test_meminit: all 40 tests in test_kvmalloc passed
test_meminit: all 20 tests in test_kmemcache passed
test_meminit: all 70 tests passed!
Signed-off-by: Alexander Potapenko <glider@google.com>
To: Kees Cook <keescook@chromium.org>
To: Andrew Morton <akpm@linux-foundation.org>
To: Christoph Lameter <cl@linux.com>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Kostya Serebryany <kcc@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Sandeep Patil <sspatil@android.com>
Cc: Laura Abbott <labbott@redhat.com>
Cc: Jann Horn <jannh@google.com>
Cc: linux-mm@kvack.org
Cc: linux-security-module@vger.kernel.org
Cc: kernel-hardening@lists.openwall.com
---
v3:
- added example test output to the description
- fixed a missing include spotted by kbuild test robot <lkp@intel.com>
- added a missing MODULE_LICENSE
- call do_kmem_cache_size() with size >= sizeof(void*) to unbreak
debug builds
---
lib/Kconfig.debug | 8 ++
lib/Makefile | 1 +
lib/test_meminit.c | 208 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 217 insertions(+)
create mode 100644 lib/test_meminit.c
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index fdfa173651eb..036e8ef03831 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2043,6 +2043,14 @@ config TEST_STACKINIT
If unsure, say N.
+config TEST_MEMINIT
+ tristate "Test level of heap/page initialization"
+ help
+ Test if the kernel is zero-initializing heap and page allocations.
+ This can be useful to test init_on_alloc and init_on_free features.
+
+ If unsure, say N.
+
endif # RUNTIME_TESTING_MENU
config MEMTEST
diff --git a/lib/Makefile b/lib/Makefile
index fb7697031a79..05980c802500 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -91,6 +91,7 @@ obj-$(CONFIG_TEST_DEBUG_VIRTUAL) += test_debug_virtual.o
obj-$(CONFIG_TEST_MEMCAT_P) += test_memcat_p.o
obj-$(CONFIG_TEST_OBJAGG) += test_objagg.o
obj-$(CONFIG_TEST_STACKINIT) += test_stackinit.o
+obj-$(CONFIG_TEST_MEMINIT) += test_meminit.o
obj-$(CONFIG_TEST_LIVEPATCH) += livepatch/
diff --git a/lib/test_meminit.c b/lib/test_meminit.c
new file mode 100644
index 000000000000..d46e2b8c8e8e
--- /dev/null
+++ b/lib/test_meminit.c
@@ -0,0 +1,208 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Test cases for SL[AOU]B/page initialization at alloc/free time.
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/mm.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/vmalloc.h>
+
+#define GARBAGE_INT (0x09A7BA9E)
+#define GARBAGE_BYTE (0x9E)
+
+#define REPORT_FAILURES_IN_FN() \
+ do { \
+ if (failures) \
+ pr_info("%s failed %d out of %d times\n", \
+ __func__, failures, num_tests); \
+ else \
+ pr_info("all %d tests in %s passed\n", \
+ num_tests, __func__); \
+ } while (0)
+
+/* Calculate the number of uninitialized bytes in the buffer. */
+static int count_nonzero_bytes(void *ptr, size_t size)
+{
+ int i, ret = 0;
+ unsigned char *p = (unsigned char *)ptr;
+
+ for (i = 0; i < size; i++)
+ if (p[i])
+ ret++;
+ return ret;
+}
+
+static void fill_with_garbage(void *ptr, size_t size)
+{
+ unsigned int *p = (unsigned int *)ptr;
+ int i = 0;
+
+ while (size >= sizeof(*p)) {
+ p[i] = GARBAGE_INT;
+ i++;
+ size -= sizeof(*p);
+ }
+ if (size)
+ memset(&p[i], GARBAGE_BYTE, size);
+}
+
+static int __init do_alloc_pages_order(int order, int *total_failures)
+{
+ struct page *page;
+ void *buf;
+ size_t size = PAGE_SIZE << order;
+
+ page = alloc_pages(GFP_KERNEL, order);
+ buf = page_address(page);
+ fill_with_garbage(buf, size);
+ __free_pages(page, order);
+
+ page = alloc_pages(GFP_KERNEL, order);
+ buf = page_address(page);
+ if (count_nonzero_bytes(buf, size))
+ (*total_failures)++;
+ fill_with_garbage(buf, size);
+ __free_pages(page, order);
+ return 1;
+}
+
+static int __init test_pages(int *total_failures)
+{
+ int failures = 0, num_tests = 0;
+ int i;
+
+ for (i = 0; i < 10; i++)
+ num_tests += do_alloc_pages_order(i, &failures);
+
+ REPORT_FAILURES_IN_FN();
+ *total_failures += failures;
+ return num_tests;
+}
+
+static int __init do_kmalloc_size(size_t size, int *total_failures)
+{
+ void *buf;
+
+ buf = kmalloc(size, GFP_KERNEL);
+ fill_with_garbage(buf, size);
+ kfree(buf);
+
+ buf = kmalloc(size, GFP_KERNEL);
+ if (count_nonzero_bytes(buf, size))
+ (*total_failures)++;
+ fill_with_garbage(buf, size);
+ kfree(buf);
+ return 1;
+}
+
+static int __init do_vmalloc_size(size_t size, int *total_failures)
+{
+ void *buf;
+
+ buf = vmalloc(size);
+ fill_with_garbage(buf, size);
+ vfree(buf);
+
+ buf = vmalloc(size);
+ if (count_nonzero_bytes(buf, size))
+ (*total_failures)++;
+ fill_with_garbage(buf, size);
+ vfree(buf);
+ return 1;
+}
+
+static int __init test_kvmalloc(int *total_failures)
+{
+ int failures = 0, num_tests = 0;
+ int i, size;
+
+ for (i = 0; i < 20; i++) {
+ size = 1 << i;
+ num_tests += do_kmalloc_size(size, &failures);
+ num_tests += do_vmalloc_size(size, &failures);
+ }
+
+ REPORT_FAILURES_IN_FN();
+ *total_failures += failures;
+ return num_tests;
+}
+
+#define CTOR_BYTES 4
+/* Initialize the first 4 bytes of the object. */
+void some_ctor(void *obj)
+{
+ memset(obj, 'A', CTOR_BYTES);
+}
+
+static int __init do_kmem_cache_size(size_t size, bool want_ctor,
+ int *total_failures)
+{
+ struct kmem_cache *c;
+ void *buf;
+ int iter, bytes = 0;
+ int fail = 0;
+
+ c = kmem_cache_create("test_cache", size, 1, 0,
+ want_ctor ? some_ctor : NULL);
+ for (iter = 0; iter < 10; iter++) {
+ buf = kmem_cache_alloc(c, GFP_KERNEL);
+ if (!want_ctor || iter == 0)
+ bytes = count_nonzero_bytes(buf, size);
+ if (want_ctor) {
+ /*
+ * Newly initialized memory must be initialized using
+ * the constructor.
+ */
+ if (iter == 0 && bytes < CTOR_BYTES)
+ fail = 1;
+ } else {
+ if (bytes)
+ fail = 1;
+ }
+ fill_with_garbage(buf, size);
+ kmem_cache_free(c, buf);
+ }
+ kmem_cache_destroy(c);
+
+ *total_failures += fail;
+ return 1;
+}
+
+static int __init test_kmemcache(int *total_failures)
+{
+ int failures = 0, num_tests = 0;
+ int i, size;
+
+ for (i = 0; i < 10; i++) {
+ size = 8 << i;
+ num_tests += do_kmem_cache_size(size, false, &failures);
+ num_tests += do_kmem_cache_size(size, true, &failures);
+ }
+ REPORT_FAILURES_IN_FN();
+ *total_failures += failures;
+ return num_tests;
+}
+
+static int __init test_meminit_init(void)
+{
+ int failures = 0, num_tests = 0;
+
+ num_tests += test_pages(&failures);
+ num_tests += test_kvmalloc(&failures);
+ num_tests += test_kmemcache(&failures);
+
+ if (failures == 0)
+ pr_info("all %d tests passed!\n", num_tests);
+ else
+ pr_info("failures: %d out of %d\n", failures, num_tests);
+
+ return failures ? -EINVAL : 0;
+}
+module_init(test_meminit_init);
+
+MODULE_LICENSE("GPL");
--
2.21.0.1020.gf2820cf01a-goog
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox