* Re: [PATCH 00/11] Keyrings, Block and USB notifications [ver #7]
From: David Howells @ 2019-09-02 12:39 UTC (permalink / raw)
To: Casey Schaufler
Cc: dhowells, viro, Stephen Smalley, Greg Kroah-Hartman,
nicolas.dichtel, raven, Christian Brauner, keyrings, linux-usb,
linux-security-module, linux-fsdevel, linux-api, linux-block,
linux-kernel
In-Reply-To: <563ae8b4-753a-179d-4f6d-94d2dd058f3b@schaufler-ca.com>
Casey Schaufler <casey@schaufler-ca.com> wrote:
> > Tests for the key/keyring events can be found on the keyutils next branch:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/keyutils.git/log/?h=next
>
> I'm having trouble with the "make install" on Fedora. Is there an
> unusual dependency?
What's the symptom you're seeing? Is it this:
install -D -m 0644 libkeyutils.a /tmp/opt/lib64 libcrypt.so.2 => /lib64/libcrypt.so.2 (0x00007f7dcbf6d000)/libkeyutils.a
/bin/sh: -c: line 0: syntax error near unexpected token `('
/bin/sh: -c: line 0: `install -D -m 0644 libkeyutils.a /tmp/opt/lib64 libcrypt.so.2 => /lib64/libcrypt.so.2 (0x00007f7dcbf6d000)/libkeyutils.a'
David
^ permalink raw reply
* [PATCH] keys: Fix missing null pointer check in request_key_auth_describe()
From: David Howells @ 2019-09-02 12:37 UTC (permalink / raw)
To: torvalds
Cc: Sachin Sant, Hillf Danton, Sachin Sant, dhowells, keyrings,
linux-security-module, linux-kernel
From: Hillf Danton <hdanton@sina.com>
If a request_key authentication token key gets revoked, there's a window in
which request_key_auth_describe() can see it with a NULL payload - but it
makes no check for this and something like the following oops may occur:
BUG: Kernel NULL pointer dereference at 0x00000038
Faulting instruction address: 0xc0000000004ddf30
Oops: Kernel access of bad area, sig: 11 [#1]
...
NIP [...] request_key_auth_describe+0x90/0xd0
LR [...] request_key_auth_describe+0x54/0xd0
Call Trace:
[...] request_key_auth_describe+0x54/0xd0 (unreliable)
[...] proc_keys_show+0x308/0x4c0
[...] seq_read+0x3d0/0x540
[...] proc_reg_read+0x90/0x110
[...] __vfs_read+0x3c/0x70
[...] vfs_read+0xb4/0x1b0
[...] ksys_read+0x7c/0x130
[...] system_call+0x5c/0x70
Fix this by checking for a NULL pointer when describing such a key.
Also make the read routine check for a NULL pointer to be on the safe side.
[DH: Modified to not take already-held rcu lock and modified to also check
in the read routine]
Fixes: 04c567d9313e ("[PATCH] Keys: Fix race between two instantiators of a key")
Reported-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
Signed-off-by: Hillf Danton <hdanton@sina.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Tested-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
---
security/keys/request_key_auth.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/security/keys/request_key_auth.c b/security/keys/request_key_auth.c
index e73ec040e250..ecba39c93fd9 100644
--- a/security/keys/request_key_auth.c
+++ b/security/keys/request_key_auth.c
@@ -66,6 +66,9 @@ static void request_key_auth_describe(const struct key *key,
{
struct request_key_auth *rka = dereference_key_rcu(key);
+ if (!rka)
+ return;
+
seq_puts(m, "key:");
seq_puts(m, key->description);
if (key_is_positive(key))
@@ -83,6 +86,9 @@ static long request_key_auth_read(const struct key *key,
size_t datalen;
long ret;
+ if (!rka)
+ return -EKEYREVOKED;
+
datalen = rka->callout_len;
ret = datalen;
^ permalink raw reply related
* Re: [PATCH 1/3] ima: keep the integrity state of open files up to date
From: kbuild test robot @ 2019-09-02 11:31 UTC (permalink / raw)
To: Janne Karhunen
Cc: kbuild-all, linux-integrity, linux-security-module, zohar,
linux-mm, viro, Janne Karhunen, Konsta Karsisto
In-Reply-To: <20190902094540.12786-1-janne.karhunen@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1396 bytes --]
Hi Janne,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[cannot apply to v5.3-rc6 next-20190830]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Janne-Karhunen/ima-keep-the-integrity-state-of-open-files-up-to-date/20190902-182420
config: i386-defconfig (attached as .config)
compiler: gcc-7 (Debian 7.4.0-11) 7.4.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from security/integrity/iint.c:22:0:
>> security/integrity/integrity.h:122:5: warning: "CONFIG_IMA_MEASUREMENT_LATENCY" is not defined, evaluates to 0 [-Wundef]
#if CONFIG_IMA_MEASUREMENT_LATENCY == 0
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/CONFIG_IMA_MEASUREMENT_LATENCY +122 security/integrity/integrity.h
121
> 122 #if CONFIG_IMA_MEASUREMENT_LATENCY == 0
123 #define IMA_LATENCY_INCREMENT 100
124 #else
125 #define IMA_LATENCY_INCREMENT CONFIG_IMA_MEASUREMENT_LATENCY
126 #endif
127
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28046 bytes --]
^ permalink raw reply
* [PATCH 3/3] ima: update the file measurement on writes
From: Janne Karhunen @ 2019-09-02 9:45 UTC (permalink / raw)
To: linux-integrity, linux-security-module, zohar, linux-mm, viro
Cc: Konsta Karsisto, Janne Karhunen
In-Reply-To: <20190902094540.12786-1-janne.karhunen@gmail.com>
From: Konsta Karsisto <konsta.karsisto@gmail.com>
Hook do_writepages() in order to do IMA measurement of an inode
that is written out.
Depends on commit 72649b7862a7 ("ima: keep the integrity state of open files up to date")'
Signed-off-by: Konsta Karsisto <konsta.karsisto@gmail.com>
Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
---
include/linux/ima.h | 10 +++
mm/page-writeback.c | 7 ++
security/integrity/iint.c | 3 +
security/integrity/ima/ima.h | 14 ++++
security/integrity/ima/ima_main.c | 115 ++++++++++++++++++++++++++++--
security/integrity/integrity.h | 6 ++
6 files changed, 150 insertions(+), 5 deletions(-)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 6736844e90d3..9d83f4beac7f 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -96,6 +96,8 @@ static inline void ima_kexec_cmdline(const void *buf, int size) {}
#if ((defined CONFIG_IMA) && defined(CONFIG_IMA_MEASURE_WRITES))
void ima_file_update(struct file *file);
void ima_file_delayed_update(struct file *file);
+void ima_inode_update(struct inode *inode);
+void ima_inode_delayed_update(struct inode *inode);
#else
static inline void ima_file_update(struct file *file)
{
@@ -105,6 +107,14 @@ static inline void ima_file_delayed_update(struct file *file)
{
return;
}
+static inline void ima_inode_update(struct inode *inode)
+{
+ return;
+}
+static inline void ima_inode_delayed_update(struct inode *inode)
+{
+ return;
+}
#endif
#ifndef CONFIG_IMA_KEXEC
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 1804f64ff43c..d5041c625529 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -38,6 +38,7 @@
#include <linux/sched/rt.h>
#include <linux/sched/signal.h>
#include <linux/mm_inline.h>
+#include <linux/ima.h>
#include <trace/events/writeback.h>
#include "internal.h"
@@ -2347,6 +2348,12 @@ int do_writepages(struct address_space *mapping, struct writeback_control *wbc)
cond_resched();
congestion_wait(BLK_RW_ASYNC, HZ/50);
}
+ if (ret == 0) {
+ if (wbc->sync_mode == WB_SYNC_ALL)
+ ima_inode_update(mapping->host);
+ else
+ ima_inode_delayed_update(mapping->host);
+ }
return ret;
}
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index e12c4900510f..bac15a2b8bee 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -82,6 +82,8 @@ static void iint_free(struct integrity_iint_cache *iint)
iint->ima_creds_status = INTEGRITY_UNKNOWN;
iint->evm_status = INTEGRITY_UNKNOWN;
iint->measured_pcrs = 0;
+ WARN_ON(iint->ima_work.file);
+ WARN_ON(!list_empty(&iint->file_list));
kmem_cache_free(iint_cache, iint);
}
@@ -161,6 +163,7 @@ static void init_once(void *foo)
iint->ima_read_status = INTEGRITY_UNKNOWN;
iint->ima_creds_status = INTEGRITY_UNKNOWN;
iint->evm_status = INTEGRITY_UNKNOWN;
+ INIT_LIST_HEAD(&iint->file_list);
mutex_init(&iint->mutex);
}
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 195e67631f70..04ba4888b764 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -162,6 +162,10 @@ int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
void *lsm_data);
#if ((defined CONFIG_IMA) && defined(CONFIG_IMA_MEASURE_WRITES))
void ima_cancel_measurement(struct integrity_iint_cache *iint);
+void ima_get_file(struct integrity_iint_cache *iint,
+ struct file *file);
+void ima_put_file(struct integrity_iint_cache *iint,
+ struct file *file);
#else
static inline void ima_cancel_measurement(struct integrity_iint_cache *iint)
{
@@ -172,6 +176,16 @@ static inline void ima_init_measurement(struct integrity_iint_cache *iint,
{
return;
}
+static inline void ima_get_file(struct integrity_iint_cache *iint,
+ struct file *file)
+{
+ return;
+}
+static inline void ima_put_file(struct integrity_iint_cache *iint,
+ struct file *file)
+{
+ return;
+}
#endif
/*
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 46d28cdb6466..affc74a07125 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -12,7 +12,8 @@
*
* File: ima_main.c
* implements the IMA hooks: ima_bprm_check, ima_file_mmap,
- * ima_file_delayed_update, ima_file_update and ima_file_check.
+ * ima_file_update, ima_file_delayed_update, ima_inode_update,
+ * ima_inode_delayed_update and ima_file_check.
*/
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -157,6 +158,7 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint,
ima_cancel_measurement(iint);
mutex_lock(&iint->mutex);
+ ima_put_file(iint, file);
if (atomic_read(&inode->i_writecount) == 1) {
update = test_and_clear_bit(IMA_UPDATE_XATTR,
&iint->atomic_flags);
@@ -295,6 +297,12 @@ static int process_measurement(struct file *file, const struct cred *cred,
set_bit(IMA_UPDATE_XATTR, &iint->atomic_flags);
}
+ if (must_appraise && (file->f_mode & FMODE_WRITE))
+ set_bit(IMA_UPDATE_XATTR, &iint->atomic_flags);
+
+ /* Cache file for measurements triggered from inode writeback */
+ ima_get_file(iint, file);
+
/* Nothing to do, just return existing appraised status */
if (!action) {
if (must_appraise) {
@@ -362,12 +370,9 @@ static int process_measurement(struct file *file, const struct cred *cred,
out:
if (pathbuf)
__putname(pathbuf);
- if (must_appraise) {
+ if (must_appraise)
if (rc && (ima_appraise & IMA_APPRAISE_ENFORCE))
return -EACCES;
- if (file->f_mode & FMODE_WRITE)
- set_bit(IMA_UPDATE_XATTR, &iint->atomic_flags);
- }
return 0;
}
@@ -425,6 +430,42 @@ int ima_bprm_check(struct linux_binprm *bprm)
}
#ifdef CONFIG_IMA_MEASURE_WRITES
+void ima_get_file(struct integrity_iint_cache *iint,
+ struct file *file)
+{
+ struct ima_fl_entry *e;
+
+ if (!iint || !file)
+ return;
+ if (!(file->f_mode & FMODE_WRITE) ||
+ !test_bit(IMA_UPDATE_XATTR, &iint->atomic_flags))
+ return;
+
+ list_for_each_entry(e, &iint->file_list, list) {
+ if (e->file == file)
+ return;
+ }
+ e = kmalloc(sizeof(*e), GFP_KERNEL);
+ if (!e)
+ return;
+ e->file = file;
+ list_add(&e->list, &iint->file_list);
+}
+
+void ima_put_file(struct integrity_iint_cache *iint,
+ struct file *file)
+{
+ struct ima_fl_entry *e;
+
+ list_for_each_entry(e, &iint->file_list, list) {
+ if (e->file == file) {
+ list_del(&e->list);
+ kfree(e);
+ break;
+ }
+ }
+}
+
static unsigned long ima_inode_update_delay(struct inode *inode)
{
unsigned long blocks, msecs;
@@ -454,6 +495,7 @@ void ima_cancel_measurement(struct integrity_iint_cache *iint)
return;
cancel_delayed_work_sync(&iint->ima_work.work);
+ iint->ima_work.file = NULL;
iint->ima_work.state = IMA_WORK_CANCELLED;
}
@@ -499,6 +541,69 @@ void ima_file_delayed_update(struct file *file)
}
EXPORT_SYMBOL_GPL(ima_file_delayed_update);
+/**
+ * ima_inode_delayed_update - delayed measurement update of an inode
+ * @inode: dirty inode chosen for writeback
+ *
+ * Schedule work to measure the first available 'struct file' cached
+ * in the iint entry that references this inode. This allows IMA to
+ * track inode writebacks.
+ *
+ * Note that we haven't incremented the refcount for the files we keep
+ * track of in order to not mess up the normal file refcounting. If we
+ * see a file whose f_count is already zero, we simply skip it. If we
+ * fail to find any available file reference, the measurement will be
+ * handled by the ima_check_last_writer().
+ */
+void ima_inode_delayed_update(struct inode *inode)
+{
+ struct integrity_iint_cache *iint;
+ struct ima_fl_entry *e;
+ bool found = false;
+
+ iint = integrity_iint_find(inode);
+ if (!iint)
+ return;
+
+ if (iint->ima_work.state == IMA_WORK_ACTIVE)
+ return;
+
+ mutex_lock(&iint->mutex);
+ list_for_each_entry(e, &iint->file_list, list) {
+ if (file_count(e->file) == 0)
+ continue;
+ found = true;
+ break;
+ }
+ mutex_unlock(&iint->mutex);
+ if (found && e->file)
+ ima_file_delayed_update(e->file);
+}
+EXPORT_SYMBOL_GPL(ima_inode_delayed_update);
+
+void ima_inode_update(struct inode *inode)
+{
+ struct integrity_iint_cache *iint;
+ struct ima_fl_entry *e;
+ bool found = false;
+
+ iint = integrity_iint_find(inode);
+ if (!iint)
+ return;
+
+ mutex_lock(&iint->mutex);
+ list_for_each_entry(e, &iint->file_list, list) {
+ if (file_count(e->file) == 0)
+ continue;
+ found = true;
+ break;
+ }
+ mutex_unlock(&iint->mutex);
+ if (found && e->file)
+ ima_file_update(e->file);
+}
+EXPORT_SYMBOL_GPL(ima_inode_update);
+
/**
* ima_file_update - update the file measurement
* @file: pointer to file structure being updated
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 0f80c3d2e079..17af72683b87 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -132,6 +132,11 @@ struct ima_work_entry {
uint8_t state;
};
+struct ima_fl_entry {
+ struct list_head list;
+ struct file *file;
+};
+
/* integrity data associated with an inode */
struct integrity_iint_cache {
struct rb_node rb_node; /* rooted in integrity_iint_tree */
@@ -149,6 +154,7 @@ struct integrity_iint_cache {
enum integrity_status evm_status:4;
struct ima_digest_data *ima_hash;
struct ima_work_entry ima_work;
+ struct list_head file_list;
};
/* rbtree tree calls to lookup, insert, delete
--
2.17.1
^ permalink raw reply related
* [PATCH 2/3] ima: update the file measurement on truncate
From: Janne Karhunen @ 2019-09-02 9:45 UTC (permalink / raw)
To: linux-integrity, linux-security-module, zohar, linux-mm, viro
Cc: Janne Karhunen, Konsta Karsisto
In-Reply-To: <20190902094540.12786-1-janne.karhunen@gmail.com>
Let IMA know when a file is being opened with truncate
or truncated directly.
Depends on commit 72649b7862a7 ("ima: keep the integrity state of open files up to date")'
Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
Signed-off-by: Konsta Karsisto <konsta.karsisto@gmail.com>
---
fs/namei.c | 5 ++++-
fs/open.c | 3 +++
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/fs/namei.c b/fs/namei.c
index 209c51a5226c..0994fe26bef1 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3418,8 +3418,11 @@ static int do_last(struct nameidata *nd,
goto out;
opened:
error = ima_file_check(file, op->acc_mode);
- if (!error && will_truncate)
+ if (!error && will_truncate) {
error = handle_truncate(file);
+ if (!error)
+ ima_file_update(file);
+ }
out:
if (unlikely(error > 0)) {
WARN_ON(1);
diff --git a/fs/open.c b/fs/open.c
index a59abe3c669a..98c2d4629371 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -63,6 +63,9 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
/* Note any delegations or leases have already been broken: */
ret = notify_change(dentry, &newattrs, NULL);
inode_unlock(dentry->d_inode);
+
+ if (filp)
+ ima_file_update(filp);
return ret;
}
--
2.17.1
^ permalink raw reply related
* [PATCH 1/3] ima: keep the integrity state of open files up to date
From: Janne Karhunen @ 2019-09-02 9:45 UTC (permalink / raw)
To: linux-integrity, linux-security-module, zohar, linux-mm, viro
Cc: Janne Karhunen, Konsta Karsisto
When a file is open for writing, kernel crash or power outage
is guaranteed to corrupt the inode integrity state leading to
file appraisal failure on the subsequent boot. Add some basic
infrastructure to keep the integrity measurements up to date
as the files are written to.
Core file operations (open, close, sync, msync, truncate) are
now allowed to update the measurement immediately. In order
to maintain sufficient write performance for writes, add a
latency tunable delayed work workqueue for computing the
measurements.
Changelog v2:
- Make write measurements optional
- Add support for MMIO measurements
- Handle all writes via page flush
- Add work cancellation support, files are properly closed
after last_writer checks out. This fixes a userspace break
where the file was still open for writing after closing it.
- Fix/unify IMA_UPDATE_XATTR usage
Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
Signed-off-by: Konsta Karsisto <konsta.karsisto@gmail.com>
---
include/linux/ima.h | 14 +++
security/integrity/ima/Kconfig | 30 ++++++
security/integrity/ima/ima.h | 13 +++
security/integrity/ima/ima_appraise.c | 13 ++-
security/integrity/ima/ima_main.c | 128 +++++++++++++++++++++++++-
security/integrity/integrity.h | 18 ++++
6 files changed, 213 insertions(+), 3 deletions(-)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index a20ad398d260..6736844e90d3 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -93,6 +93,20 @@ static inline void ima_post_path_mknod(struct dentry *dentry)
static inline void ima_kexec_cmdline(const void *buf, int size) {}
#endif /* CONFIG_IMA */
+#if ((defined CONFIG_IMA) && defined(CONFIG_IMA_MEASURE_WRITES))
+void ima_file_update(struct file *file);
+void ima_file_delayed_update(struct file *file);
+#else
+static inline void ima_file_update(struct file *file)
+{
+ return;
+}
+static inline void ima_file_delayed_update(struct file *file)
+{
+ return;
+}
+#endif
+
#ifndef CONFIG_IMA_KEXEC
struct kimage;
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 897bafc59a33..df1cf684a442 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -310,3 +310,33 @@ config IMA_APPRAISE_SIGNED_INIT
default n
help
This option requires user-space init to be signed.
+
+config IMA_MEASURE_WRITES
+ bool "Measure file writes (EXPERIMENTAL)"
+ depends on IMA
+ depends on EVM
+ default n
+ help
+ By default IMA measures files only when they close or sync.
+ Choose this option if you want the integrity measurements on
+ the disk to update when the files are still open for writing.
+
+config IMA_MEASUREMENT_LATENCY
+ int
+ depends on IMA
+ range 0 60000
+ default 50
+ help
+ This value defines the measurement interval when files are
+ being written. Value is in milliseconds.
+
+config IMA_MEASUREMENT_LATENCY_CEILING
+ int
+ depends on IMA
+ range 100 60000
+ default 5000
+ help
+ In order to maintain high write performance for large files,
+ IMA increases the measurement interval as the file size grows.
+ This value defines the ceiling for the measurement delay in
+ milliseconds.
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 19769bf5f6ab..195e67631f70 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -160,6 +160,19 @@ void ima_init_template_list(void);
int __init ima_init_digests(void);
int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
void *lsm_data);
+#if ((defined CONFIG_IMA) && defined(CONFIG_IMA_MEASURE_WRITES))
+void ima_cancel_measurement(struct integrity_iint_cache *iint);
+#else
+static inline void ima_cancel_measurement(struct integrity_iint_cache *iint)
+{
+ return;
+}
+static inline void ima_init_measurement(struct integrity_iint_cache *iint,
+ struct dentry *dentry)
+{
+ return;
+}
+#endif
/*
* used to protect h_table and sha_table
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 136ae4e0ee92..6c137fb5289b 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -78,6 +78,15 @@ static int ima_fix_xattr(struct dentry *dentry,
return rc;
}
+#ifdef CONFIG_IMA_MEASURE_WRITES
+inline void ima_init_measurement(struct integrity_iint_cache *iint,
+ struct dentry *dentry)
+{
+ if (test_bit(IMA_UPDATE_XATTR, &iint->atomic_flags))
+ ima_fix_xattr(dentry, iint);
+}
+#endif
+
/* Return specific func appraised cached result */
enum integrity_status ima_get_cache_status(struct integrity_iint_cache *iint,
enum ima_hooks func)
@@ -341,8 +350,10 @@ int ima_appraise_measurement(enum ima_hooks func,
iint->flags |= IMA_NEW_FILE;
if ((iint->flags & IMA_NEW_FILE) &&
(!(iint->flags & IMA_DIGSIG_REQUIRED) ||
- (inode->i_size == 0)))
+ (inode->i_size == 0))) {
+ ima_init_measurement(iint, dentry);
status = INTEGRITY_PASS;
+ }
goto out;
}
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 79c01516211b..46d28cdb6466 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -12,7 +12,7 @@
*
* File: ima_main.c
* implements the IMA hooks: ima_bprm_check, ima_file_mmap,
- * and ima_file_check.
+ * ima_file_delayed_update, ima_file_update and ima_file_check.
*/
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -26,6 +26,8 @@
#include <linux/xattr.h>
#include <linux/ima.h>
#include <linux/iversion.h>
+#include <linux/workqueue.h>
+#include <linux/sizes.h>
#include <linux/fs.h>
#include "ima.h"
@@ -42,6 +44,7 @@ static int hash_setup_done;
static struct notifier_block ima_lsm_policy_notifier = {
.notifier_call = ima_lsm_policy_change,
};
+static struct workqueue_struct *ima_update_wq;
static int __init hash_setup(char *str)
{
@@ -151,6 +154,7 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint,
if (!(mode & FMODE_WRITE))
return;
+ ima_cancel_measurement(iint);
mutex_lock(&iint->mutex);
if (atomic_read(&inode->i_writecount) == 1) {
@@ -420,6 +424,117 @@ int ima_bprm_check(struct linux_binprm *bprm)
MAY_EXEC, CREDS_CHECK);
}
+#ifdef CONFIG_IMA_MEASURE_WRITES
+static unsigned long ima_inode_update_delay(struct inode *inode)
+{
+ unsigned long blocks, msecs;
+
+ blocks = i_size_read(inode) / SZ_1M + 1;
+ msecs = blocks * IMA_LATENCY_INCREMENT;
+ if (msecs > CONFIG_IMA_MEASUREMENT_LATENCY_CEILING)
+ msecs = CONFIG_IMA_MEASUREMENT_LATENCY_CEILING;
+
+ return msecs;
+}
+
+static void ima_delayed_update_handler(struct work_struct *work)
+{
+ struct ima_work_entry *entry;
+
+ entry = container_of(work, typeof(*entry), work.work);
+
+ ima_file_update(entry->file);
+ entry->file = NULL;
+ entry->state = IMA_WORK_INACTIVE;
+}
+
+void ima_cancel_measurement(struct integrity_iint_cache *iint)
+{
+ if (iint->ima_work.state != IMA_WORK_ACTIVE)
+ return;
+
+ cancel_delayed_work_sync(&iint->ima_work.work);
+ iint->ima_work.state = IMA_WORK_CANCELLED;
+}
+
+/**
+ * ima_file_delayed_update
+ * @file: pointer to file structure being updated
+ */
+void ima_file_delayed_update(struct file *file)
+{
+ struct inode *inode = file_inode(file);
+ struct integrity_iint_cache *iint;
+ unsigned long msecs;
+ bool creq;
+
+ if (!ima_policy_flag || !S_ISREG(inode->i_mode))
+ return;
+
+ iint = integrity_iint_find(inode);
+ if (!iint)
+ return;
+
+ if (!test_bit(IMA_UPDATE_XATTR, &iint->atomic_flags))
+ return;
+
+ mutex_lock(&iint->mutex);
+ if (iint->ima_work.state == IMA_WORK_ACTIVE)
+ goto out;
+
+ msecs = ima_inode_update_delay(inode);
+ iint->ima_work.file = file;
+ iint->ima_work.state = IMA_WORK_ACTIVE;
+ INIT_DELAYED_WORK(&iint->ima_work.work, ima_delayed_update_handler);
+
+ creq = queue_delayed_work(ima_update_wq,
+ &iint->ima_work.work,
+ msecs_to_jiffies(msecs));
+ if (creq == false) {
+ iint->ima_work.file = NULL;
+ iint->ima_work.state = IMA_WORK_INACTIVE;
+ }
+out:
+ mutex_unlock(&iint->mutex);
+}
+EXPORT_SYMBOL_GPL(ima_file_delayed_update);
+
+/**
+ * ima_file_update - update the file measurement
+ * @file: pointer to file structure being updated
+ */
+void ima_file_update(struct file *file)
+{
+ struct inode *inode = file_inode(file);
+ struct integrity_iint_cache *iint;
+ bool should_measure = true;
+ u64 i_version;
+
+ if (!ima_policy_flag || !S_ISREG(inode->i_mode))
+ return;
+
+ iint = integrity_iint_find(inode);
+ if (!iint)
+ return;
+
+ if (!test_bit(IMA_UPDATE_XATTR, &iint->atomic_flags))
+ return;
+
+ mutex_lock(&iint->mutex);
+ if (IS_I_VERSION(inode)) {
+ i_version = inode_query_iversion(inode);
+ if (i_version == iint->version)
+ should_measure = false;
+ }
+ if (should_measure) {
+ iint->flags &= ~IMA_COLLECTED;
+ ima_update_xattr(iint, file);
+ }
+ mutex_unlock(&iint->mutex);
+}
+EXPORT_SYMBOL_GPL(ima_file_update);
+#endif /* CONFIG_IMA_MEASURE_WRITES */
+
/**
* ima_path_check - based on policy, collect/store measurement.
* @file: pointer to the file to be measured
@@ -716,9 +831,18 @@ static int __init init_ima(void)
if (error)
pr_warn("Couldn't register LSM notifier, error %d\n", error);
- if (!error)
+ if (!error) {
ima_update_policy_flag();
+ ima_update_wq = alloc_workqueue("ima-update-wq",
+ WQ_MEM_RECLAIM |
+ WQ_CPU_INTENSIVE,
+ 0);
+ if (!ima_update_wq) {
+ pr_err("Failed to allocate write measurement workqueue\n");
+ error = -ENOMEM;
+ }
+ }
return error;
}
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index d9323d31a3a8..0f80c3d2e079 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -64,6 +64,11 @@
#define IMA_DIGSIG 3
#define IMA_MUST_MEASURE 4
+/* delayed measurement job state */
+#define IMA_WORK_INACTIVE 0
+#define IMA_WORK_ACTIVE 1
+#define IMA_WORK_CANCELLED 2
+
enum evm_ima_xattr_type {
IMA_XATTR_DIGEST = 0x01,
EVM_XATTR_HMAC,
@@ -115,6 +120,18 @@ struct signature_v2_hdr {
uint8_t sig[0]; /* signature payload */
} __packed;
+#if CONFIG_IMA_MEASUREMENT_LATENCY == 0
+#define IMA_LATENCY_INCREMENT 100
+#else
+#define IMA_LATENCY_INCREMENT CONFIG_IMA_MEASUREMENT_LATENCY
+#endif
+
+struct ima_work_entry {
+ struct delayed_work work;
+ struct file *file;
+ uint8_t state;
+};
+
/* integrity data associated with an inode */
struct integrity_iint_cache {
struct rb_node rb_node; /* rooted in integrity_iint_tree */
@@ -131,6 +148,7 @@ struct integrity_iint_cache {
enum integrity_status ima_creds_status:4;
enum integrity_status evm_status:4;
struct ima_digest_data *ima_hash;
+ struct ima_work_entry ima_work;
};
/* rbtree tree calls to lookup, insert, delete
--
2.17.1
^ permalink raw reply related
* Re: [PATCH 1/2] staging: comedi: Restrict COMEDI_DEVCONFIG when the kernel is locked down
From: Ian Abbott @ 2019-09-02 9:26 UTC (permalink / raw)
To: Ben Hutchings, jmorris
Cc: linux-security-module, Matthew Garrett, David Howells,
H Hartley Sweeten
In-Reply-To: <f2a2ba9da5b55cae57f1aa62983a7e6598aca128.camel@decadent.org.uk>
On 31/08/2019 10:50, Ben Hutchings wrote:
> On Fri, 2019-08-30 at 18:35 +0100, Ian Abbott wrote:
>> On 30/08/2019 16:45, Ben Hutchings wrote:
>>> The COMEDI_DEVCONFIG ioctl can be used to configure I/O addresses and
>>> other hardware settings for non plug-and-play devices such as ISA
>>> cards. This should be disabled to preserve the kernel's integrity
>>> when it is locked down.
>>
>> I haven't boned up on the lockdown mechanism yet, but just FYI, this is
>> only possible if the "comedi_num_legacy_minors" module parameter is
>> non-zero (which it isn't by default).
>
> So do you think would it make more sense to set the HWPARAM flag on
> that module parameter? That should have the same effect although it
> doesn't seem to quite fit the intent of that flag.
HWPARAM would prohibit the creation of a few special comedi devices such
as those created by the "comedi_test" and "comedi_bond" drivers.
(Although one dummy device does get created by the "comedi_test" module
when it is loaded, and I don't know if anyone actually uses the
"comedi_bond" driver!)
But then again, the changes to COMEDI_DEVCONFIG also prohibits the
creation of those special devices, so I don't suppose it matters either way.
>
>>> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
>>> Cc: Matthew Garrett <mjg59@google.com>
>>> Cc: David Howells <dhowells@redhat.com>
>>> Cc: Ian Abbott <abbotti@mev.co.uk>
>>> Cc: H Hartley Sweeten <hsweeten@visionengravers.com>
>>> ---
>>> drivers/staging/comedi/comedi_fops.c | 6 ++++++
>>> include/linux/security.h | 1 +
>>> security/lockdown/lockdown.c | 1 +
>>> 3 files changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
>>> index f6d1287c7b83..fdf030e53035 100644
>>> --- a/drivers/staging/comedi/comedi_fops.c
>>> +++ b/drivers/staging/comedi/comedi_fops.c
>>> @@ -27,6 +27,7 @@
>>>
>>> #include <linux/io.h>
>>> #include <linux/uaccess.h>
>>> +#include <linux/security.h>
>>>
>>> #include "comedi_internal.h"
>>>
>>> @@ -813,11 +814,16 @@ static int do_devconfig_ioctl(struct comedi_device *dev,
>>> struct comedi_devconfig __user *arg)
>>> {
>>> struct comedi_devconfig it;
>>> + int ret;
>>>
>>> lockdep_assert_held(&dev->mutex);
>>> if (!capable(CAP_SYS_ADMIN))
>>> return -EPERM;
>>>
>>> + ret = security_locked_down(LOCKDOWN_COMEDI_DEVCONFIG);
>>> + if (ret)
>>> + return ret;
>>> +
>>
>> You might consider moving that check to be done after the following 'if
>> (!arg)' block, since that should be safe. (It detaches an already
>> configured device from the comedi core.)
> [...]
>
> How would it have been configured, though?
It works on automatically registered comedi devices too. I suppose that
could be done via the "unbind" file in the driver, but that goes through
a different path and is a bit harder to use.
--
-=( Ian Abbott <abbotti@mev.co.uk> || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address: )=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
^ permalink raw reply
* Re: [RFC/RFT v4 5/5] KEYS: trusted: Add generic trusted keys framework
From: Sumit Garg @ 2019-09-02 5:07 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: keyrings, linux-integrity,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE,
linux-security-module, Mimi Zohar, dhowells, Herbert Xu, davem,
peterhuewe, jgg, jejb, Arnd Bergmann, Greg Kroah-Hartman,
James Morris, Serge E. Hallyn, Casey Schaufler, Ard Biesheuvel,
Daniel Thompson, Linux Kernel Mailing List,
tee-dev @ lists . linaro . org
In-Reply-To: <20190830172405.rafhm362tsuufbqb@linux.intel.com>
On Fri, 30 Aug 2019 at 22:54, Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Fri, Aug 30, 2019 at 08:20:31PM +0300, Jarkko Sakkinen wrote:
> > On Fri, Aug 30, 2019 at 02:49:31PM +0530, Sumit Garg wrote:
> > > Any comments/feedback on this patch before I send next version of TEE
> > > patch-set with this patch included?
> >
> > Unfortunately don't have time before LPC to go deep with the follow up.
> >
> > I will look into this in detail after LPC.
No worries, I will wait for your feedback.
>
> I'll ping you once your first row of patches are in my tree so you
> can rebase these on top of that.
>
Thanks.
-Sumit
> /JArkko
^ permalink raw reply
* Re: [PATCH] netlabel: remove redundant assignment to pointer iter
From: David Miller @ 2019-09-01 18:45 UTC (permalink / raw)
To: colin.king
Cc: paul, netdev, linux-security-module, kernel-janitors,
linux-kernel
In-Reply-To: <20190901155205.16877-1-colin.king@canonical.com>
From: Colin King <colin.king@canonical.com>
Date: Sun, 1 Sep 2019 16:52:05 +0100
> From: Colin Ian King <colin.king@canonical.com>
>
> Pointer iter is being initialized with a value that is never read and
> is being re-assigned a little later on. The assignment is redundant
> and hence can be removed.
>
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Applied to net-next.
^ permalink raw reply
* Re: [PATCH] netlabel: remove redundant assignment to pointer iter
From: Paul Moore @ 2019-09-01 18:23 UTC (permalink / raw)
To: Colin King
Cc: David S . Miller, netdev, linux-security-module, kernel-janitors,
linux-kernel
In-Reply-To: <20190901155205.16877-1-colin.king@canonical.com>
On Sun, Sep 1, 2019 at 11:52 AM Colin King <colin.king@canonical.com> wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Pointer iter is being initialized with a value that is never read and
> is being re-assigned a little later on. The assignment is redundant
> and hence can be removed.
>
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> net/netlabel/netlabel_kapi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Acked-by: Paul Moore <paul@paul-moore.com>
> diff --git a/net/netlabel/netlabel_kapi.c b/net/netlabel/netlabel_kapi.c
> index 2b0ef55cf89e..409a3ae47ce2 100644
> --- a/net/netlabel/netlabel_kapi.c
> +++ b/net/netlabel/netlabel_kapi.c
> @@ -607,7 +607,7 @@ static struct netlbl_lsm_catmap *_netlbl_catmap_getnode(
> */
> int netlbl_catmap_walk(struct netlbl_lsm_catmap *catmap, u32 offset)
> {
> - struct netlbl_lsm_catmap *iter = catmap;
> + struct netlbl_lsm_catmap *iter;
> u32 idx;
> u32 bit;
> NETLBL_CATMAP_MAPTYPE bitmap;
> --
> 2.20.1
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH] netlabel: remove redundant assignment to pointer iter
From: Paul Moore @ 2019-09-01 18:22 UTC (permalink / raw)
To: Christophe JAILLET
Cc: Colin King, David S . Miller, netdev, linux-security-module,
kernel-janitors, linux-kernel
In-Reply-To: <de0cd774-8fce-d69a-8ea4-3c7b2ee3a918@wanadoo.fr>
On Sun, Sep 1, 2019 at 1:16 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
> Le 01/09/2019 à 18:04, Paul Moore a écrit :
>
> On Sun, Sep 1, 2019 at 11:52 AM Colin King <colin.king@canonical.com> wrote:
>
> From: Colin Ian King <colin.king@canonical.com>
>
> Pointer iter is being initialized with a value that is never read and
> is being re-assigned a little later on. The assignment is redundant
> and hence can be removed.
>
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> net/netlabel/netlabel_kapi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> This patch doesn't seem correct to me, at least not in current form.
> At the top of _netlbl_catmap_getnode() is a check to see if iter is
> NULL (as well as a few other checks on iter after that); this patch
> would break that code.
>
> Perhaps we can get rid of the iter/catmap assignment when we define
> iter, but I don't think this patch is the right way to do it.
>
> diff --git a/net/netlabel/netlabel_kapi.c b/net/netlabel/netlabel_kapi.c
> index 2b0ef55cf89e..409a3ae47ce2 100644
> --- a/net/netlabel/netlabel_kapi.c
> +++ b/net/netlabel/netlabel_kapi.c
> @@ -607,7 +607,7 @@ static struct netlbl_lsm_catmap *_netlbl_catmap_getnode(
> */
> int netlbl_catmap_walk(struct netlbl_lsm_catmap *catmap, u32 offset)
> {
> - struct netlbl_lsm_catmap *iter = catmap;
> + struct netlbl_lsm_catmap *iter;
> u32 idx;
> u32 bit;
> NETLBL_CATMAP_MAPTYPE bitmap;
> --
> 2.20.1
>
> 'iter' is reassigned a value between the declaration and the NULL test, so removing the fist initialisation looks good to me.
This is what I get when I try to review patches quickly while doing
other things on the weekend <sigh> ... yes, you are correct, I was
looking at _netlbl_catmap_getnode() and not netlbl_catmap_walk(); my
apologies.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH] netlabel: remove redundant assignment to pointer iter
From: Christophe JAILLET @ 2019-09-01 17:20 UTC (permalink / raw)
To: Paul Moore, Colin King
Cc: David S . Miller, netdev, linux-security-module, kernel-janitors,
linux-kernel
In-Reply-To: <CAHC9VhSVKEJ-EBAry5fVN3Ux22occGQ5jxbFBecMsR+q7+UT5A@mail.gmail.com>
Le 01/09/2019 à 18:04, Paul Moore a écrit :
> On Sun, Sep 1, 2019 at 11:52 AM Colin King <colin.king@canonical.com> wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> Pointer iter is being initialized with a value that is never read and
>> is being re-assigned a little later on. The assignment is redundant
>> and hence can be removed.
>>
>> Addresses-Coverity: ("Unused value")
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>> net/netlabel/netlabel_kapi.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
> This patch doesn't seem correct to me, at least not in current form.
> At the top of _netlbl_catmap_getnode() is a check to see if iter is
> NULL (as well as a few other checks on iter after that); this patch
> would break that code.
>
> Perhaps we can get rid of the iter/catmap assignment when we define
> iter, but I don't think this patch is the right way to do it.
>
>> diff --git a/net/netlabel/netlabel_kapi.c b/net/netlabel/netlabel_kapi.c
>> index 2b0ef55cf89e..409a3ae47ce2 100644
>> --- a/net/netlabel/netlabel_kapi.c
>> +++ b/net/netlabel/netlabel_kapi.c
>> @@ -607,7 +607,7 @@ static struct netlbl_lsm_catmap *_netlbl_catmap_getnode(
>> */
>> int netlbl_catmap_walk(struct netlbl_lsm_catmap *catmap, u32 offset)
>> {
>> - struct netlbl_lsm_catmap *iter = catmap;
>> + struct netlbl_lsm_catmap *iter;
>> u32 idx;
>> u32 bit;
>> NETLBL_CATMAP_MAPTYPE bitmap;
>> --
>> 2.20.1
>
Hi,
'iter' is reassigned a value between the declaration and the NULL test, so removing the first initialization looks good to me.
int netlbl_catmap_walk(struct netlbl_lsm_catmap *catmap, u32 offset)
{
|
struct netlbl_lsm_catmap *iter = catmap;
u32 idx;
u32 bit;
NETLBL_CATMAP_MAPTYPE bitmap;
iter = _netlbl_catmap_getnode(&catmap, offset, _CM_F_WALK, 0); <-- Here
if (iter == NULL)
return -ENOENT; This is dead code since commit d960a6184a92 ("netlabel: fix the catmap walking functions") where the call to _netlbl_catmap_getnode has been introduced.
Just my 2c,
CJ|
^ permalink raw reply
* Re: [PATCH] netlabel: remove redundant assignment to pointer iter
From: Paul Moore @ 2019-09-01 16:04 UTC (permalink / raw)
To: Colin King
Cc: David S . Miller, netdev, linux-security-module, kernel-janitors,
linux-kernel
In-Reply-To: <20190901155205.16877-1-colin.king@canonical.com>
On Sun, Sep 1, 2019 at 11:52 AM Colin King <colin.king@canonical.com> wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Pointer iter is being initialized with a value that is never read and
> is being re-assigned a little later on. The assignment is redundant
> and hence can be removed.
>
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> net/netlabel/netlabel_kapi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
This patch doesn't seem correct to me, at least not in current form.
At the top of _netlbl_catmap_getnode() is a check to see if iter is
NULL (as well as a few other checks on iter after that); this patch
would break that code.
Perhaps we can get rid of the iter/catmap assignment when we define
iter, but I don't think this patch is the right way to do it.
> diff --git a/net/netlabel/netlabel_kapi.c b/net/netlabel/netlabel_kapi.c
> index 2b0ef55cf89e..409a3ae47ce2 100644
> --- a/net/netlabel/netlabel_kapi.c
> +++ b/net/netlabel/netlabel_kapi.c
> @@ -607,7 +607,7 @@ static struct netlbl_lsm_catmap *_netlbl_catmap_getnode(
> */
> int netlbl_catmap_walk(struct netlbl_lsm_catmap *catmap, u32 offset)
> {
> - struct netlbl_lsm_catmap *iter = catmap;
> + struct netlbl_lsm_catmap *iter;
> u32 idx;
> u32 bit;
> NETLBL_CATMAP_MAPTYPE bitmap;
> --
> 2.20.1
--
paul moore
www.paul-moore.com
^ permalink raw reply
* [PATCH] netlabel: remove redundant assignment to pointer iter
From: Colin King @ 2019-09-01 15:52 UTC (permalink / raw)
To: Paul Moore, David S . Miller, netdev, linux-security-module
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Pointer iter is being initialized with a value that is never read and
is being re-assigned a little later on. The assignment is redundant
and hence can be removed.
Addresses-Coverity: ("Unused value")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
net/netlabel/netlabel_kapi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/netlabel/netlabel_kapi.c b/net/netlabel/netlabel_kapi.c
index 2b0ef55cf89e..409a3ae47ce2 100644
--- a/net/netlabel/netlabel_kapi.c
+++ b/net/netlabel/netlabel_kapi.c
@@ -607,7 +607,7 @@ static struct netlbl_lsm_catmap *_netlbl_catmap_getnode(
*/
int netlbl_catmap_walk(struct netlbl_lsm_catmap *catmap, u32 offset)
{
- struct netlbl_lsm_catmap *iter = catmap;
+ struct netlbl_lsm_catmap *iter;
u32 idx;
u32 bit;
NETLBL_CATMAP_MAPTYPE bitmap;
--
2.20.1
^ permalink raw reply related
* Re: [PATCH 1/2] staging: comedi: Restrict COMEDI_DEVCONFIG when the kernel is locked down
From: Ben Hutchings @ 2019-08-31 9:50 UTC (permalink / raw)
To: Ian Abbott, jmorris
Cc: linux-security-module, Matthew Garrett, David Howells,
H Hartley Sweeten
In-Reply-To: <ea330943-9d3e-d01d-a6cf-8de08e042ec6@mev.co.uk>
[-- Attachment #1: Type: text/plain, Size: 2359 bytes --]
On Fri, 2019-08-30 at 18:35 +0100, Ian Abbott wrote:
> On 30/08/2019 16:45, Ben Hutchings wrote:
> > The COMEDI_DEVCONFIG ioctl can be used to configure I/O addresses and
> > other hardware settings for non plug-and-play devices such as ISA
> > cards. This should be disabled to preserve the kernel's integrity
> > when it is locked down.
>
> I haven't boned up on the lockdown mechanism yet, but just FYI, this is
> only possible if the "comedi_num_legacy_minors" module parameter is
> non-zero (which it isn't by default).
So do you think would it make more sense to set the HWPARAM flag on
that module parameter? That should have the same effect although it
doesn't seem to quite fit the intent of that flag.
> > Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> > Cc: Matthew Garrett <mjg59@google.com>
> > Cc: David Howells <dhowells@redhat.com>
> > Cc: Ian Abbott <abbotti@mev.co.uk>
> > Cc: H Hartley Sweeten <hsweeten@visionengravers.com>
> > ---
> > drivers/staging/comedi/comedi_fops.c | 6 ++++++
> > include/linux/security.h | 1 +
> > security/lockdown/lockdown.c | 1 +
> > 3 files changed, 8 insertions(+)
> >
> > diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
> > index f6d1287c7b83..fdf030e53035 100644
> > --- a/drivers/staging/comedi/comedi_fops.c
> > +++ b/drivers/staging/comedi/comedi_fops.c
> > @@ -27,6 +27,7 @@
> >
> > #include <linux/io.h>
> > #include <linux/uaccess.h>
> > +#include <linux/security.h>
> >
> > #include "comedi_internal.h"
> >
> > @@ -813,11 +814,16 @@ static int do_devconfig_ioctl(struct comedi_device *dev,
> > struct comedi_devconfig __user *arg)
> > {
> > struct comedi_devconfig it;
> > + int ret;
> >
> > lockdep_assert_held(&dev->mutex);
> > if (!capable(CAP_SYS_ADMIN))
> > return -EPERM;
> >
> > + ret = security_locked_down(LOCKDOWN_COMEDI_DEVCONFIG);
> > + if (ret)
> > + return ret;
> > +
>
> You might consider moving that check to be done after the following 'if
> (!arg)' block, since that should be safe. (It detaches an already
> configured device from the comedi core.)
[...]
How would it have been configured, though?
Ben.
--
Ben Hutchings
You can't have everything. Where would you put it?
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 00/11] Keyrings, Block and USB notifications [ver #7]
From: Casey Schaufler @ 2019-08-30 22:09 UTC (permalink / raw)
To: David Howells, viro
Cc: Stephen Smalley, Greg Kroah-Hartman, nicolas.dichtel, raven,
Christian Brauner, keyrings, linux-usb, linux-security-module,
linux-fsdevel, linux-api, linux-block, linux-kernel, casey
In-Reply-To: <156717343223.2204.15875738850129174524.stgit@warthog.procyon.org.uk>
On 8/30/2019 6:57 AM, David Howells wrote:
> Here's a set of patches to add a general notification queue concept and to
> add sources of events for:
>
> (1) Key/keyring events, such as creating, linking and removal of keys.
>
> (2) General device events (single common queue) including:
>
> - Block layer events, such as device errors
>
> - USB subsystem events, such as device/bus attach/remove, device
> reset, device errors.
>
> Tests for the key/keyring events can be found on the keyutils next branch:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/keyutils.git/log/?h=next
I'm having trouble with the "make install" on Fedora. Is there an
unusual dependency?
>
> Notifications are done automatically inside of the testing infrastructure
> on every change to that every test makes to a key or keyring.
>
> Manual pages can be found there also, including pages for watch_queue(7)
> and the watch_devices(2) system call (these should be transferred to the
> manpages package if taken upstream).
>
> LSM hooks are included:
>
> (1) A set of hooks are provided that allow an LSM to rule on whether or
> not a watch may be set. Each of these hooks takes a different
> "watched object" parameter, so they're not really shareable. The LSM
> should use current's credentials. [Wanted by SELinux & Smack]
>
> (2) A hook is provided to allow an LSM to rule on whether or not a
> particular message may be posted to a particular queue. This is given
> the credentials from the event generator (which may be the system) and
> the watch setter. [Wanted by Smack]
>
> I've provided a preliminary attempt to provide SELinux and Smack with
> implementations of some of these hooks.
>
>
> Design decisions:
>
> (1) A misc chardev is used to create and open a ring buffer:
>
> fd = open("/dev/watch_queue", O_RDWR);
>
> which is then configured and mmap'd into userspace:
>
> ioctl(fd, IOC_WATCH_QUEUE_SET_SIZE, BUF_SIZE);
> ioctl(fd, IOC_WATCH_QUEUE_SET_FILTER, &filter);
> buf = mmap(NULL, BUF_SIZE * page_size, PROT_READ | PROT_WRITE,
> MAP_SHARED, fd, 0);
>
> The fd cannot be read or written (though there is a facility to use
> write to inject records for debugging) and userspace just pulls data
> directly out of the buffer.
>
> (2) The ring index pointers are stored inside the ring and are thus
> accessible to userspace. Userspace should only update the tail
> pointer and never the head pointer or risk breaking the buffer. The
> kernel checks that the pointers appear valid before trying to use
> them. A 'skip' record is maintained around the pointers.
>
> (3) poll() can be used to wait for data to appear in the buffer.
>
> (4) Records in the buffer are binary, typed and have a length so that they
> can be of varying size.
>
> This means that multiple heterogeneous sources can share a common
> buffer. Tags may be specified when a watchpoint is created to help
> distinguish the sources.
>
> (5) The queue is reusable as there are 16 million types available, of
> which I've used just a few, so there is scope for others to be used.
>
> (6) Records are filterable as types have up to 256 subtypes that can be
> individually filtered. Other filtration is also available.
>
> (7) Each time the buffer is opened, a new buffer is created - this means
> that there's no interference between watchers.
>
> (8) When recording a notification, the kernel will not sleep, but will
> rather mark a queue as overrun if there's insufficient space, thereby
> avoiding userspace causing the kernel to hang.
>
> (9) The 'watchpoint' should be specific where possible, meaning that you
> specify the object that you want to watch.
>
> (10) The buffer is created and then watchpoints are attached to it, using
> one of:
>
> keyctl_watch_key(KEY_SPEC_SESSION_KEYRING, fd, 0x01);
> watch_devices(fd, 0x02, 0);
>
> where in both cases, fd indicates the queue and the number after is a
> tag between 0 and 255.
>
> (11) The watch must be removed if either the watch buffer is destroyed or
> the watched object is destroyed.
>
>
> Things I want to avoid:
>
> (1) Introducing features that make the core VFS dependent on the network
> stack or networking namespaces (ie. usage of netlink).
>
> (2) Dumping all this stuff into dmesg and having a daemon that sits there
> parsing the output and distributing it as this then puts the
> responsibility for security into userspace and makes handling
> namespaces tricky. Further, dmesg might not exist or might be
> inaccessible inside a container.
>
> (3) Letting users see events they shouldn't be able to see.
>
>
> The patches can be found here also:
>
> http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=notifications-core
>
> Changes:
>
> ver #7:
>
> (*) Removed the 'watch' argument from the security_watch_key() and
> security_watch_devices() hooks as current_cred() can be used instead
> of watch->cred.
>
> ver #6:
>
> (*) Fix mmap bug in watch_queue driver.
>
> (*) Add an extended removal notification that can transmit an identifier
> to userspace (such as a key ID).
>
> (*) Don't produce a instantiation notification in mark_key_instantiated()
> but rather do it in the caller to prevent key updates from producing
> an instantiate notification as well as an update notification.
>
> (*) Set the right number of filters in the sample program.
>
> (*) Provide preliminary hook implementations for SELinux and Smack.
>
> ver #5:
>
> (*) Split the superblock watch and mount watch parts out into their own
> branch (notifications-mount) as they really need certain fsinfo()
> attributes.
>
> (*) Rearrange the watch notification UAPI header to push the length down
> to bits 0-5 and remove the lost-message bits. The userspace's watch
> ID tag is moved to bits 8-15 and then the message type is allocated
> all of bits 16-31 for its own purposes.
>
> The lost-message bit is moved over to the header, rather than being
> placed in the next message to be generated and given its own word so
> it can be cleared with xchg(,0) for parisc.
>
> (*) The security_post_notification() hook is no longer called with the
> spinlock held and softirqs disabled - though the RCU readlock is still
> held.
>
> (*) Buffer pages are now accounted towards RLIMIT_MEMLOCK and CAP_IPC_LOCK
> will skip the overuse check.
>
> (*) The buffer is marked VM_DONTEXPAND.
>
> (*) Save the watch-setter's creds in struct watch and give that to the LSM
> hook for posting a message.
>
> ver #4:
>
> (*) Split the basic UAPI bits out into their own patch and then split the
> LSM hooks out into an intermediate patch. Add LSM hooks for setting
> watches.
>
> Rename the *_notify() system calls to watch_*() for consistency.
>
> ver #3:
>
> (*) I've added a USB notification source and reformulated the block
> notification source so that there's now a common watch list, for which
> the system call is now device_notify().
>
> I've assigned a pair of unused ioctl numbers in the 'W' series to the
> ioctls added by this series.
>
> I've also added a description of the kernel API to the documentation.
>
> ver #2:
>
> (*) I've fixed various issues raised by Jann Horn and GregKH and moved to
> krefs for refcounting. I've added some security features to try and
> give Casey Schaufler the LSM control he wants.
>
> David
> ---
> David Howells (11):
> uapi: General notification ring definitions
> security: Add hooks to rule on setting a watch
> security: Add a hook for the point of notification insertion
> General notification queue with user mmap()'able ring buffer
> keys: Add a notification facility
> Add a general, global device notification watch list
> block: Add block layer notifications
> usb: Add USB subsystem notifications
> Add sample notification program
> selinux: Implement the watch_key security hook
> smack: Implement the watch_key and post_notification hooks [untested]
>
>
> Documentation/ioctl/ioctl-number.rst | 1
> Documentation/security/keys/core.rst | 58 ++
> Documentation/watch_queue.rst | 460 ++++++++++++++
> arch/alpha/kernel/syscalls/syscall.tbl | 1
> arch/arm/tools/syscall.tbl | 1
> arch/ia64/kernel/syscalls/syscall.tbl | 1
> arch/m68k/kernel/syscalls/syscall.tbl | 1
> arch/microblaze/kernel/syscalls/syscall.tbl | 1
> arch/mips/kernel/syscalls/syscall_n32.tbl | 1
> arch/mips/kernel/syscalls/syscall_n64.tbl | 1
> arch/mips/kernel/syscalls/syscall_o32.tbl | 1
> arch/parisc/kernel/syscalls/syscall.tbl | 1
> arch/powerpc/kernel/syscalls/syscall.tbl | 1
> arch/s390/kernel/syscalls/syscall.tbl | 1
> arch/sh/kernel/syscalls/syscall.tbl | 1
> arch/sparc/kernel/syscalls/syscall.tbl | 1
> arch/x86/entry/syscalls/syscall_32.tbl | 1
> arch/x86/entry/syscalls/syscall_64.tbl | 1
> arch/xtensa/kernel/syscalls/syscall.tbl | 1
> block/Kconfig | 9
> block/blk-core.c | 29 +
> drivers/base/Kconfig | 9
> drivers/base/Makefile | 1
> drivers/base/watch.c | 90 +++
> drivers/misc/Kconfig | 13
> drivers/misc/Makefile | 1
> drivers/misc/watch_queue.c | 893 +++++++++++++++++++++++++++
> drivers/usb/core/Kconfig | 9
> drivers/usb/core/devio.c | 56 ++
> drivers/usb/core/hub.c | 4
> include/linux/blkdev.h | 15
> include/linux/device.h | 7
> include/linux/key.h | 3
> include/linux/lsm_audit.h | 1
> include/linux/lsm_hooks.h | 38 +
> include/linux/security.h | 32 +
> include/linux/syscalls.h | 1
> include/linux/usb.h | 18 +
> include/linux/watch_queue.h | 94 +++
> include/uapi/asm-generic/unistd.h | 4
> include/uapi/linux/keyctl.h | 2
> include/uapi/linux/watch_queue.h | 183 ++++++
> kernel/sys_ni.c | 1
> samples/Kconfig | 6
> samples/Makefile | 1
> samples/watch_queue/Makefile | 8
> samples/watch_queue/watch_test.c | 233 +++++++
> security/keys/Kconfig | 9
> security/keys/compat.c | 3
> security/keys/gc.c | 5
> security/keys/internal.h | 30 +
> security/keys/key.c | 38 +
> security/keys/keyctl.c | 99 +++
> security/keys/keyring.c | 20 -
> security/keys/request_key.c | 4
> security/security.c | 23 +
> security/selinux/hooks.c | 14
> security/smack/smack_lsm.c | 82 ++
> 58 files changed, 2593 insertions(+), 30 deletions(-)
> create mode 100644 Documentation/watch_queue.rst
> create mode 100644 drivers/base/watch.c
> create mode 100644 drivers/misc/watch_queue.c
> create mode 100644 include/linux/watch_queue.h
> create mode 100644 include/uapi/linux/watch_queue.h
> create mode 100644 samples/watch_queue/Makefile
> create mode 100644 samples/watch_queue/watch_test.c
>
^ permalink raw reply
* Re: [PATCH 1/2] rtnetlink: gate MAC address with an LSM hook
From: Paul Moore @ 2019-08-30 21:46 UTC (permalink / raw)
To: Michal Kubecek
Cc: netdev, Jeffrey Vander Stoep, David Miller, LSM List, selinux
In-Reply-To: <20190829074516.GM29594@unicorn.suse.cz>
On Thu, Aug 29, 2019 at 3:45 AM Michal Kubecek <mkubecek@suse.cz> wrote:
> On Tue, Aug 27, 2019 at 04:47:04PM -0400, Paul Moore wrote:
> >
> > I'm also not a big fan of inserting the hook in rtnl_fill_ifinfo(); as
> > presented it is way too specific for a LSM hook for me to be happy.
> > However, I do agree that giving the LSMs some control over netlink
> > messages makes sense. As others have pointed out, it's all a matter
> > of where to place the hook.
> >
> > If we only care about netlink messages which leverage nlattrs I
> > suppose one option that I haven't seen mentioned would be to place a
> > hook in nla_put(). While it is a bit of an odd place for a hook, it
> > would allow the LSM easy access to the skb and attribute type to make
> > decisions, and all of the callers should already be checking the
> > return code (although we would need to verify this). One notable
> > drawback (not the only one) is that the hook is going to get hit
> > multiple times for each message.
>
> For most messages, "multiple times" would mean tens, for many even
> hundreds of calls. For each, you would have to check corresponding
> socket (and possibly also genetlink header) to see which netlink based
> protocol it is and often even parse existing part of the message to get
> the context (because the same numeric attribute type can mean something
> completely different if it appears in a nested attribute).
>
> Also, nla_put() (or rather __nla_put()) is not used for all attributes,
> one may also use nla_reserve() and then compose the attribute date in
> place.
I never said it was a great idea, just an idea ;)
Honestly I'm just trying to spur some discussion on this so we can
hopefully arrive at a solution which allows a LSM to control kernel
generated netlink messages that we can all accept.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH] audit, security: allow LSMs to selectively enable audit collection
From: Paul Moore @ 2019-08-30 21:36 UTC (permalink / raw)
To: Casey Schaufler
Cc: Stephen Smalley, Aaron Goidel, James Morris, Serge Hallyn,
keescook, rgb, linux-audit, linux-security-module, selinux
In-Reply-To: <c5156aaf-e361-65e6-3ca5-89f17fee54bd@schaufler-ca.com>
On Fri, Aug 30, 2019 at 11:31 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 8/30/2019 6:44 AM, Stephen Smalley wrote:
> > On 8/15/19 1:41 PM, Aaron Goidel wrote:
> >> Presently, there is no way for LSMs to enable collection of supplemental
> >> audit records such as path and inode information when a permission denial
> >> occurs. Provide a LSM hook to allow LSMs to selectively enable collection
> >> on a per-task basis, even if the audit configuration would otherwise
> >> disable auditing of a task and/or contains no audit filter rules. If the
> >> hook returns a non-zero result, collect all available audit information. If
> >> the hook generates its own audit record, then supplemental audit
> >> information will be emitted at syscall exit.
> >>
> >> In SELinux, we implement this hook by returning the result of a permission
> >> check on the process. If the new process2:audit_enable permission is
> >> allowed by the policy, then audit collection will be enabled for that
> >> process. Otherwise, SELinux will defer to the audit configuration.
> >
> > Any feedback on this RFC patch? I know Paul provided some thoughts on the general topic of LSM/audit enablement in the other patch thread but I haven't seen any response to this patch.
>
> Audit policy should be independent of security module policy.
> I shouldn't have to change SELinux policy to enable this data
> collection. I should be able to change the audit configuration
> to get this if I want it.
The idea is that the LSM can request that the audit subsystem
selectively enable auditing (per-task, and hopefully per-record-type);
the audit policy can still be configured as you normally. This is to
work around people (and distros) that disable audit, yet still want to
audit some information (yeah, I know ...).
--
paul moore
www.paul-moore.com
^ permalink raw reply
* general protection fault in smack_socket_sendmsg
From: syzbot @ 2019-08-30 19:40 UTC (permalink / raw)
To: casey, jmorris, linux-kernel, linux-security-module, serge,
syzkaller-bugs
Hello,
syzbot found the following crash on:
HEAD commit: 6525771f Merge tag 'arc-5.3-rc7' of git://git.kernel.org/p..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=11486cea600000
kernel config: https://syzkaller.appspot.com/x/.config?x=58485246ad14eafe
dashboard link: https://syzkaller.appspot.com/bug?extid=5fd781d646d4fcbdfeb0
compiler: clang version 9.0.0 (/home/glider/llvm/clang
80fee25776c2fb61e74c1ecb1a523375c2500b69)
Unfortunately, I don't have any reproducer for this crash yet.
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+5fd781d646d4fcbdfeb0@syzkaller.appspotmail.com
kasan: CONFIG_KASAN_INLINE enabled
kasan: GPF could be caused by NULL-ptr deref or user memory access
general protection fault: 0000 [#1] PREEMPT SMP KASAN
CPU: 0 PID: 11983 Comm: kworker/0:0 Not tainted 5.3.0-rc6+ #94
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Workqueue: krxrpcd rxrpc_peer_keepalive_worker
RIP: 0010:smack_socket_sendmsg+0x5b/0x480 security/smack/smack_lsm.c:3677
Code: e8 6a 07 71 fe 4c 89 e8 48 c1 e8 03 42 80 3c 38 00 74 08 4c 89 ef e8
b4 ff a9 fe 4d 8b 65 00 48 83 c3 18 48 89 d8 48 c1 e8 03 <42> 80 3c 38 00
74 08 48 89 df e8 96 ff a9 fe 4c 8b 33 49 8d 9e 08
RSP: 0018:ffff8881daa1f9c8 EFLAGS: 00010206
RAX: 0000000000000003 RBX: 0000000000000018 RCX: ffff888048882500
RDX: 0000000000000000 RSI: ffff8881daa1fb18 RDI: 0000000000000000
RBP: ffff8881daa1fa80 R08: ffffffff8350cc90 R09: ffff8881daa1fb86
R10: ffffed103b543f72 R11: 0000000000000000 R12: ffff88803704c594
R13: ffff8881daa1fb18 R14: dffffc0000000000 R15: dffffc0000000000
FS: 0000000000000000(0000) GS:ffff8880aea00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f7d4f2b5028 CR3: 000000005e835000 CR4: 00000000001426f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
security_socket_sendmsg+0x6c/0xd0 security/security.c:1973
sock_sendmsg net/socket.c:654 [inline]
kernel_sendmsg+0x77/0x140 net/socket.c:677
rxrpc_send_keepalive+0x254/0x3c0 net/rxrpc/output.c:655
rxrpc_peer_keepalive_dispatch net/rxrpc/peer_event.c:369 [inline]
rxrpc_peer_keepalive_worker+0x76e/0xb40 net/rxrpc/peer_event.c:430
process_one_work+0x7ef/0x10e0 kernel/workqueue.c:2269
worker_thread+0xc01/0x1630 kernel/workqueue.c:2415
kthread+0x332/0x350 kernel/kthread.c:255
ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
Modules linked in:
---[ end trace 61235a384085b26a ]---
RIP: 0010:smack_socket_sendmsg+0x5b/0x480 security/smack/smack_lsm.c:3677
Code: e8 6a 07 71 fe 4c 89 e8 48 c1 e8 03 42 80 3c 38 00 74 08 4c 89 ef e8
b4 ff a9 fe 4d 8b 65 00 48 83 c3 18 48 89 d8 48 c1 e8 03 <42> 80 3c 38 00
74 08 48 89 df e8 96 ff a9 fe 4c 8b 33 49 8d 9e 08
RSP: 0018:ffff8881daa1f9c8 EFLAGS: 00010206
RAX: 0000000000000003 RBX: 0000000000000018 RCX: ffff888048882500
RDX: 0000000000000000 RSI: ffff8881daa1fb18 RDI: 0000000000000000
RBP: ffff8881daa1fa80 R08: ffffffff8350cc90 R09: ffff8881daa1fb86
R10: ffffed103b543f72 R11: 0000000000000000 R12: ffff88803704c594
R13: ffff8881daa1fb18 R14: dffffc0000000000 R15: dffffc0000000000
FS: 0000000000000000(0000) GS:ffff8880aea00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000960004 CR3: 0000000022fd0000 CR4: 00000000001426f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
^ permalink raw reply
* Re: [PATCH 1/2] staging: comedi: Restrict COMEDI_DEVCONFIG when the kernel is locked down
From: Ian Abbott @ 2019-08-30 17:35 UTC (permalink / raw)
To: Ben Hutchings, jmorris
Cc: linux-security-module, Matthew Garrett, David Howells,
H Hartley Sweeten
In-Reply-To: <20190830154549.vss6h5tlrl6d5r5y@decadent.org.uk>
On 30/08/2019 16:45, Ben Hutchings wrote:
> The COMEDI_DEVCONFIG ioctl can be used to configure I/O addresses and
> other hardware settings for non plug-and-play devices such as ISA
> cards. This should be disabled to preserve the kernel's integrity
> when it is locked down.
I haven't boned up on the lockdown mechanism yet, but just FYI, this is
only possible if the "comedi_num_legacy_minors" module parameter is
non-zero (which it isn't by default).
>
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> Cc: Matthew Garrett <mjg59@google.com>
> Cc: David Howells <dhowells@redhat.com>
> Cc: Ian Abbott <abbotti@mev.co.uk>
> Cc: H Hartley Sweeten <hsweeten@visionengravers.com>
> ---
> drivers/staging/comedi/comedi_fops.c | 6 ++++++
> include/linux/security.h | 1 +
> security/lockdown/lockdown.c | 1 +
> 3 files changed, 8 insertions(+)
>
> diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
> index f6d1287c7b83..fdf030e53035 100644
> --- a/drivers/staging/comedi/comedi_fops.c
> +++ b/drivers/staging/comedi/comedi_fops.c
> @@ -27,6 +27,7 @@
>
> #include <linux/io.h>
> #include <linux/uaccess.h>
> +#include <linux/security.h>
>
> #include "comedi_internal.h"
>
> @@ -813,11 +814,16 @@ static int do_devconfig_ioctl(struct comedi_device *dev,
> struct comedi_devconfig __user *arg)
> {
> struct comedi_devconfig it;
> + int ret;
>
> lockdep_assert_held(&dev->mutex);
> if (!capable(CAP_SYS_ADMIN))
> return -EPERM;
>
> + ret = security_locked_down(LOCKDOWN_COMEDI_DEVCONFIG);
> + if (ret)
> + return ret;
> +
You might consider moving that check to be done after the following 'if
(!arg)' block, since that should be safe. (It detaches an already
configured device from the comedi core.)
> if (!arg) {
> if (is_device_busy(dev))
> return -EBUSY;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 429f9f03372b..b16365dccfc5 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -113,6 +113,7 @@ enum lockdown_reason {
> LOCKDOWN_ACPI_TABLES,
> LOCKDOWN_PCMCIA_CIS,
> LOCKDOWN_TIOCSSERIAL,
> + LOCKDOWN_COMEDI_DEVCONFIG,
> LOCKDOWN_MODULE_PARAMETERS,
> LOCKDOWN_MMIOTRACE,
> LOCKDOWN_DEBUGFS,
> diff --git a/security/lockdown/lockdown.c b/security/lockdown/lockdown.c
> index 0068cec77c05..971bb99b9051 100644
> --- a/security/lockdown/lockdown.c
> +++ b/security/lockdown/lockdown.c
> @@ -28,6 +28,7 @@ static char *lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
> [LOCKDOWN_ACPI_TABLES] = "modifying ACPI tables",
> [LOCKDOWN_PCMCIA_CIS] = "direct PCMCIA CIS storage",
> [LOCKDOWN_TIOCSSERIAL] = "reconfiguration of serial port IO",
> + [LOCKDOWN_COMEDI_DEVCONFIG] = "reconfiguration of Comedi legacy device",
> [LOCKDOWN_MODULE_PARAMETERS] = "unsafe module parameters",
> [LOCKDOWN_MMIOTRACE] = "unsafe mmio",
> [LOCKDOWN_DEBUGFS] = "debugfs access",
>
--
-=( Ian Abbott <abbotti@mev.co.uk> || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address: )=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
^ permalink raw reply
* Re: [RFC/RFT v4 5/5] KEYS: trusted: Add generic trusted keys framework
From: Jarkko Sakkinen @ 2019-08-30 17:24 UTC (permalink / raw)
To: Sumit Garg
Cc: keyrings, linux-integrity,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE,
linux-security-module, Mimi Zohar, dhowells, Herbert Xu, davem,
peterhuewe, jgg, jejb, Arnd Bergmann, Greg Kroah-Hartman,
James Morris, Serge E. Hallyn, Casey Schaufler, Ard Biesheuvel,
Daniel Thompson, Linux Kernel Mailing List,
tee-dev @ lists . linaro . org
In-Reply-To: <20190830172031.dm5icfyakko6eqak@linux.intel.com>
On Fri, Aug 30, 2019 at 08:20:31PM +0300, Jarkko Sakkinen wrote:
> On Fri, Aug 30, 2019 at 02:49:31PM +0530, Sumit Garg wrote:
> > Any comments/feedback on this patch before I send next version of TEE
> > patch-set with this patch included?
>
> Unfortunately don't have time before LPC to go deep with the follow up.
>
> I will look into this in detail after LPC.
I'll ping you once your first row of patches are in my tree so you
can rebase these on top of that.
/JArkko
^ permalink raw reply
* Re: [RFC/RFT v4 5/5] KEYS: trusted: Add generic trusted keys framework
From: Jarkko Sakkinen @ 2019-08-30 17:20 UTC (permalink / raw)
To: Sumit Garg
Cc: keyrings, linux-integrity,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE,
linux-security-module, Mimi Zohar, dhowells, Herbert Xu, davem,
peterhuewe, jgg, jejb, Arnd Bergmann, Greg Kroah-Hartman,
James Morris, Serge E. Hallyn, Casey Schaufler, Ard Biesheuvel,
Daniel Thompson, Linux Kernel Mailing List,
tee-dev @ lists . linaro . org
In-Reply-To: <CAFA6WYO7Z-Enmnqt8zA_+VV_p=mAc+AotTetv9hhf2xHm0mR9g@mail.gmail.com>
On Fri, Aug 30, 2019 at 02:49:31PM +0530, Sumit Garg wrote:
> Any comments/feedback on this patch before I send next version of TEE
> patch-set with this patch included?
Unfortunately don't have time before LPC to go deep with the follow up.
I will look into this in detail after LPC.
/JArkko
^ permalink raw reply
* Re: [PATCH] tpm_tis: Fix interrupt probing
From: Jarkko Sakkinen @ 2019-08-30 17:00 UTC (permalink / raw)
To: Stefan Berger
Cc: Stefan Berger, linux-integrity, linux-security-module,
linux-kernel
In-Reply-To: <039601c6-e22a-d113-0eb2-ee2a206f5b73@linux.ibm.com>
On Thu, Aug 29, 2019 at 08:11:27PM -0400, Stefan Berger wrote:
> On 8/29/19 12:10 PM, Jarkko Sakkinen wrote:
> > On Thu, Aug 29, 2019 at 04:20:21PM +0300, Jarkko Sakkinen wrote:
> > > On Tue, Aug 27, 2019 at 03:34:36PM -0400, Stefan Berger wrote:
> > > > On 8/27/19 11:19 AM, Jarkko Sakkinen wrote:
> > > > > On Tue, Aug 27, 2019 at 04:14:00PM +0300, Jarkko Sakkinen wrote:
> > > > > > On Tue, Aug 20, 2019 at 08:25:17AM -0400, Stefan Berger wrote:
> > > > > > > From: Stefan Berger <stefanb@linux.ibm.com>
> > > > > > >
> > > > > > > The interrupt probing of the TPM TIS was broken since we are trying to
> > > > > > > run it without an active locality and without the TPM_CHIP_FLAG_IRQ set.
> > > > > > >
> > > > > > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > > > > > Need these:
> > > > > >
> > > > > > Cc: linux-stable@vger.kernel.org
> > > > > > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> > > > > >
> > > > > > Thank you. I'll apply this to my tree.
> > > > > >
> > > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > > The commit went in the following form:
> > > > >
> > > > > http://git.infradead.org/users/jjs/linux-tpmdd.git/commit/9b558deab2c5d7dc23d5f7a4064892ede482ad32
> > > > I saw you dropped the stetting of the IRQ flag - I needed it, otherwise it
> > > > wouldn't execute certain code paths.
> > > I explained why I removed that part. There was no any reasoning for
> > > it. Also, it cannot be in the same commit if it fixes a diffent
> > > issue.
> > AFAIK they go with different fixes-tags.
>
> I sent a separate patch for this. It looks like this bug goes back to when
> the TPM_CHIP_FLAG_IRQ was introduced in March 2019?!
Thank you!
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
/Jarkko
^ permalink raw reply
* Re: [PATCH V40 23/29] bpf: Restrict bpf when kernel lockdown is in confidentiality mode
From: David Howells @ 2019-08-30 16:32 UTC (permalink / raw)
To: Matthew Garrett
Cc: dhowells, jmorris, linux-security-module, linux-kernel, linux-api,
Alexei Starovoitov, Matthew Garrett, Kees Cook, netdev,
Chun-Yi Lee, Daniel Borkmann
In-Reply-To: <20190820001805.241928-24-matthewgarrett@google.com>
Matthew Garrett <matthewgarrett@google.com> wrote:
> From: David Howells <dhowells@redhat.com>
>
> bpf_read() and bpf_read_str() could potentially be abused to (eg) allow
> private keys in kernel memory to be leaked. Disable them if the kernel
> has been locked down in confidentiality mode.
>
> Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> cc: netdev@vger.kernel.org
> cc: Chun-Yi Lee <jlee@suse.com>
> cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Signed-off-by: James Morris <jmorris@namei.org>
Signed-off-by: David Howells <dhowells@redhat.com>
^ permalink raw reply
* Re: [PATCH V40 04/29] lockdown: Enforce module signatures if the kernel is locked down
From: David Howells @ 2019-08-30 16:31 UTC (permalink / raw)
To: Matthew Garrett
Cc: dhowells, jmorris, linux-security-module, linux-kernel, linux-api,
Kees Cook, Jessica Yu
In-Reply-To: <20190820001805.241928-5-matthewgarrett@google.com>
Matthew Garrett <matthewgarrett@google.com> wrote:
> enum lockdown_reason {
> LOCKDOWN_NONE,
> + LOCKDOWN_MODULE_SIGNATURE,
> LOCKDOWN_INTEGRITY_MAX,
> LOCKDOWN_CONFIDENTIALITY_MAX,
> };
Aren't you mixing disjoint sets?
> + [LOCKDOWN_MODULE_SIGNATURE] = "unsigned module loading",
Wouldn't it be better to pass this string as a parameter to
security_locked_down()?
David
^ permalink raw reply
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