* [PATCH v2] netlabel: remove unused param from audit_log_format()
From: Alex Dewar @ 2020-08-28 13:55 UTC (permalink / raw)
Cc: Alex Dewar, Paul Moore, David S. Miller, Jakub Kicinski, netdev,
linux-security-module, linux-kernel
In-Reply-To: <CAHC9VhRtTykJVze_93ed+n+v14Ai9J5Mbre9nGEc2rkqbqKc_g@mail.gmail.com>
Commit d3b990b7f327 ("netlabel: fix problems with mapping removal")
added a check to return an error if ret_val != 0, before ret_val is
later used in a log message. Now it will unconditionally print "...
res=1". So just drop the check.
Addresses-Coverity: ("Dead code")
Fixes: d3b990b7f327 ("netlabel: fix problems with mapping removal")
Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
---
v2: Still print the res field, because it's useful (Paul)
net/netlabel/netlabel_domainhash.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/net/netlabel/netlabel_domainhash.c b/net/netlabel/netlabel_domainhash.c
index f73a8382c275e..dc8c39f51f7d3 100644
--- a/net/netlabel/netlabel_domainhash.c
+++ b/net/netlabel/netlabel_domainhash.c
@@ -612,9 +612,8 @@ int netlbl_domhsh_remove_entry(struct netlbl_dom_map *entry,
audit_buf = netlbl_audit_start_common(AUDIT_MAC_MAP_DEL, audit_info);
if (audit_buf != NULL) {
audit_log_format(audit_buf,
- " nlbl_domain=%s res=%u",
- entry->domain ? entry->domain : "(default)",
- ret_val == 0 ? 1 : 0);
+ " nlbl_domain=%s res=1",
+ entry->domain ? entry->domain : "(default)");
audit_log_end(audit_buf);
}
--
2.28.0
^ permalink raw reply related
* Re: [PATCH v2] netlabel: remove unused param from audit_log_format()
From: Paul Moore @ 2020-08-28 14:35 UTC (permalink / raw)
To: Alex Dewar
Cc: David S. Miller, Jakub Kicinski, netdev, linux-security-module,
linux-kernel
In-Reply-To: <20200828135523.12867-1-alex.dewar90@gmail.com>
On Fri, Aug 28, 2020 at 9:56 AM Alex Dewar <alex.dewar90@gmail.com> wrote:
>
> Commit d3b990b7f327 ("netlabel: fix problems with mapping removal")
> added a check to return an error if ret_val != 0, before ret_val is
> later used in a log message. Now it will unconditionally print "...
> res=1". So just drop the check.
>
> Addresses-Coverity: ("Dead code")
> Fixes: d3b990b7f327 ("netlabel: fix problems with mapping removal")
> Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
> ---
> v2: Still print the res field, because it's useful (Paul)
>
> net/netlabel/netlabel_domainhash.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
Thanks Alex.
Acked-by: Paul Moore <paul@paul-moore.com>
> diff --git a/net/netlabel/netlabel_domainhash.c b/net/netlabel/netlabel_domainhash.c
> index f73a8382c275e..dc8c39f51f7d3 100644
> --- a/net/netlabel/netlabel_domainhash.c
> +++ b/net/netlabel/netlabel_domainhash.c
> @@ -612,9 +612,8 @@ int netlbl_domhsh_remove_entry(struct netlbl_dom_map *entry,
> audit_buf = netlbl_audit_start_common(AUDIT_MAC_MAP_DEL, audit_info);
> if (audit_buf != NULL) {
> audit_log_format(audit_buf,
> - " nlbl_domain=%s res=%u",
> - entry->domain ? entry->domain : "(default)",
> - ret_val == 0 ? 1 : 0);
> + " nlbl_domain=%s res=1",
> + entry->domain ? entry->domain : "(default)");
> audit_log_end(audit_buf);
> }
>
> --
> 2.28.0
>
--
paul moore
www.paul-moore.com
^ permalink raw reply
* [PATCH v2 0/3] IMA: Generalize early boot data measurement
From: Lakshmi Ramasubramanian @ 2020-08-28 16:00 UTC (permalink / raw)
To: zohar, stephen.smalley.work, casey
Cc: tyhicks, tusharsu, sashal, jmorris, linux-integrity, selinux,
linux-security-module, linux-kernel
The current implementation of early boot measurement in the IMA
subsystem is very specific to asymmetric keys. It does not handle
early boot measurement of data from other subsystems such as
Linux Security Module (LSM), Device-Mapper, etc. As a result, data
provided by these subsystems during system boot are not measured by IMA.
This patch series makes the early boot key measurement functions generic
such that they can be used to measure any early boot data. The functions
in ima_queue_keys.c are refactored to a new file ima_queue_data.c.
The kernel configuration CONFIG_IMA_QUEUE_EARLY_BOOT_KEYS is renamed to
CONFIG_IMA_QUEUE_EARLY_BOOT_DATA so it can be used for enabling any
early boot data measurement. Since measurement of asymmetric keys is
the first consumer of early boot measurement, this kernel configuration
is enabled if IMA_MEASURE_ASYMMETRIC_KEYS and SYSTEM_TRUSTED_KEYRING are
both enabled.
The IMA hook to measure kernel critical data ima_measure_critical_data()
is updated to utilize early boot measurement support.
This series is based on the following repo/branch:
repo: https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git
branch: next-integrity
commit d012a7190fc1 ("Linux 5.9-rc2")
This patch is dependent on the following patch series:
https://patchwork.kernel.org/patch/11709527/
https://patchwork.kernel.org/patch/11742047/
Change Log:
v2:
=> Split the patches to first rename the file and functions,
and then introduce new arguments, followed by adding queuing
support in ima_measure_critical_data().
Lakshmi Ramasubramanian (3):
IMA: Generalize early boot measurement of asymmetric keys
IMA: Support measurement of generic data during early boot
IMA: Support early boot measurement of critical data
security/integrity/ima/Kconfig | 2 +-
security/integrity/ima/Makefile | 2 +-
security/integrity/ima/ima.h | 39 ++--
security/integrity/ima/ima_asymmetric_keys.c | 7 +-
security/integrity/ima/ima_init.c | 2 +-
security/integrity/ima/ima_main.c | 10 +
security/integrity/ima/ima_policy.c | 2 +-
security/integrity/ima/ima_queue_data.c | 191 +++++++++++++++++++
security/integrity/ima/ima_queue_keys.c | 175 -----------------
9 files changed, 232 insertions(+), 198 deletions(-)
create mode 100644 security/integrity/ima/ima_queue_data.c
delete mode 100644 security/integrity/ima/ima_queue_keys.c
--
2.28.0
^ permalink raw reply
* [PATCH v2 3/3] IMA: Support early boot measurement of critical data
From: Lakshmi Ramasubramanian @ 2020-08-28 16:00 UTC (permalink / raw)
To: zohar, stephen.smalley.work, casey
Cc: tyhicks, tusharsu, sashal, jmorris, linux-integrity, selinux,
linux-security-module, linux-kernel
In-Reply-To: <20200828160021.11537-1-nramas@linux.microsoft.com>
The IMA hook, namely ima_measure_critical_data(), to measure kernel
critical data requires a custom IMA policy to be loaded.
Update ima_measure_critical_data() to utilize early boot measurement
support to defer processing data if a custom IMA policy is not yet
loaded.
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
security/integrity/ima/ima_main.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 41be4d1d839e..ce0ef310c575 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -885,12 +885,22 @@ int ima_measure_critical_data(const char *event_name,
const void *buf, int buf_len,
bool measure_buf_hash)
{
+ bool queued = false;
+
if (!event_name || !event_data_source || !buf || !buf_len)
return -EINVAL;
if (!ima_kernel_data_source_is_supported(event_data_source))
return -EPERM;
+ if (ima_should_queue_data())
+ queued = ima_queue_data(event_name, buf, buf_len,
+ event_data_source, CRITICAL_DATA,
+ measure_buf_hash);
+
+ if (queued)
+ return 0;
+
return process_buffer_measurement(NULL, buf, buf_len, event_name,
CRITICAL_DATA, 0, event_data_source,
measure_buf_hash);
--
2.28.0
^ permalink raw reply related
* [PATCH v2 2/3] IMA: Support measurement of generic data during early boot
From: Lakshmi Ramasubramanian @ 2020-08-28 16:00 UTC (permalink / raw)
To: zohar, stephen.smalley.work, casey
Cc: tyhicks, tusharsu, sashal, jmorris, linux-integrity, selinux,
linux-security-module, linux-kernel
In-Reply-To: <20200828160021.11537-1-nramas@linux.microsoft.com>
Early boot measurement of data is currently very specific to key
measurement. To make it generic to handle any early boot data
measurement, additional arguments need to be saved in the queue
for deferred processing. This includes the IMA hook func,
data specific to the given func, and a boolean flag to indicate
whether the given payload needs to be measured or a hash of
the payload needs to be measured.
Add new fields to ima_data_entry struct to pass additional data for
the deferred handling of queued data. Update the queue functions to
handle the new data saved in the ima_data_entry struct.
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
security/integrity/ima/ima.h | 11 ++++++--
security/integrity/ima/ima_asymmetric_keys.c | 3 ++-
security/integrity/ima/ima_queue_data.c | 28 ++++++++++++++------
3 files changed, 31 insertions(+), 11 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 872a06821c98..422fe833037d 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -266,18 +266,25 @@ struct ima_data_entry {
void *payload;
size_t payload_len;
const char *event_name;
+ const char *event_data;
+ enum ima_hooks func;
+ bool measure_payload_hash;
};
void ima_init_data_queue(void);
bool ima_should_queue_data(void);
bool ima_queue_data(const char *event_name, const void *payload,
- size_t payload_len);
+ size_t payload_len, const char *event_data,
+ enum ima_hooks func, bool measure_payload_hash);
void ima_process_queued_data(void);
#else
static inline void ima_init_data_queue(void) {}
static inline bool ima_should_queue_data(void) { return false; }
static inline bool ima_queue_data(const char *event_name,
const void *payload,
- size_t payload_len) { return false; }
+ size_t payload_len,
+ const char *event_data,
+ enum ima_hooks func,
+ bool measure_payload_hash) { return false; }
static inline void ima_process_queued_data(void) {}
#endif /* CONFIG_IMA_QUEUE_EARLY_BOOT_DATA */
diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
index d91fddec5ae8..65423754765f 100644
--- a/security/integrity/ima/ima_asymmetric_keys.c
+++ b/security/integrity/ima/ima_asymmetric_keys.c
@@ -39,7 +39,8 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
if (ima_should_queue_data())
queued = ima_queue_data(keyring->description, payload,
- payload_len);
+ payload_len, keyring->description,
+ KEY_CHECK, false);
if (queued)
return;
diff --git a/security/integrity/ima/ima_queue_data.c b/security/integrity/ima/ima_queue_data.c
index 563e26f66180..d466ee742377 100644
--- a/security/integrity/ima/ima_queue_data.c
+++ b/security/integrity/ima/ima_queue_data.c
@@ -60,6 +60,7 @@ static void ima_free_data_entry(struct ima_data_entry *entry)
kvfree(entry->payload);
kfree(entry->event_name);
+ kfree(entry->event_data);
kfree(entry);
}
@@ -74,7 +75,10 @@ static void *ima_kvmemdup(const void *src, size_t len)
static struct ima_data_entry *ima_alloc_data_entry(const char *event_name,
const void *payload,
- size_t payload_len)
+ size_t payload_len,
+ const char *event_data,
+ enum ima_hooks func,
+ bool measure_payload_hash)
{
struct ima_data_entry *entry;
@@ -88,9 +92,15 @@ static struct ima_data_entry *ima_alloc_data_entry(const char *event_name,
*/
entry->payload = ima_kvmemdup(payload, payload_len);
entry->event_name = kstrdup(event_name, GFP_KERNEL);
+ if (event_data)
+ entry->event_data = kstrdup(event_data, GFP_KERNEL);
+
entry->payload_len = payload_len;
+ entry->func = func;
+ entry->measure_payload_hash = measure_payload_hash;
- if (!entry->payload || !entry->event_name)
+ if (!entry->payload || !entry->event_name ||
+ (event_data && !entry->event_data))
goto out;
INIT_LIST_HEAD(&entry->list);
@@ -98,19 +108,21 @@ static struct ima_data_entry *ima_alloc_data_entry(const char *event_name,
out:
integrity_audit_message(AUDIT_INTEGRITY_PCR, NULL,
- event_name, func_measure_str(KEY_CHECK),
+ event_name, func_measure_str(func),
"ENOMEM", -ENOMEM, 0, -ENOMEM);
ima_free_data_entry(entry);
return NULL;
}
bool ima_queue_data(const char *event_name, const void *payload,
- size_t payload_len)
+ size_t payload_len, const char *event_data,
+ enum ima_hooks func, bool measure_payload_hash)
{
bool queued = false;
struct ima_data_entry *entry;
- entry = ima_alloc_data_entry(event_name, payload, payload_len);
+ entry = ima_alloc_data_entry(event_name, payload, payload_len,
+ event_data, func, measure_payload_hash);
if (!entry)
return false;
@@ -165,9 +177,9 @@ void ima_process_queued_data(void)
process_buffer_measurement(NULL, entry->payload,
entry->payload_len,
entry->event_name,
- KEY_CHECK, 0,
- entry->event_name,
- false);
+ entry->func, 0,
+ entry->event_data,
+ entry->measure_payload_hash);
list_del(&entry->list);
ima_free_data_entry(entry);
}
--
2.28.0
^ permalink raw reply related
* [PATCH v2 1/3] IMA: Generalize early boot measurement of asymmetric keys
From: Lakshmi Ramasubramanian @ 2020-08-28 16:00 UTC (permalink / raw)
To: zohar, stephen.smalley.work, casey
Cc: tyhicks, tusharsu, sashal, jmorris, linux-integrity, selinux,
linux-security-module, linux-kernel
In-Reply-To: <20200828160021.11537-1-nramas@linux.microsoft.com>
The current implementation of early boot measurement in
the IMA subsystem is very specific to asymmetric keys. It does not
handle early boot measurement of data from other subsystems such as
Linux Security Module (LSM), Device-Mapper, etc. As a result, data
provided by these subsystems during system boot are not measured by IMA.
Update the early boot key measurement functions to handle any early
boot data. Refactor the code from ima_queue_keys.c to a new file
ima_queue_data.c. Rename the kernel configuration
CONFIG_IMA_QUEUE_EARLY_BOOT_KEYS to CONFIG_IMA_QUEUE_EARLY_BOOT_DATA
so it can be used for enabling any early boot data measurement.
Since measurement of asymmetric keys is the first consumer of early
boot measurement, enable this kernel configuration if
IMA_MEASURE_ASYMMETRIC_KEYS and SYSTEM_TRUSTED_KEYRING are both enabled.
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
security/integrity/ima/Kconfig | 2 +-
security/integrity/ima/Makefile | 2 +-
security/integrity/ima/ima.h | 32 ++--
security/integrity/ima/ima_asymmetric_keys.c | 6 +-
security/integrity/ima/ima_init.c | 2 +-
security/integrity/ima/ima_policy.c | 2 +-
security/integrity/ima/ima_queue_data.c | 179 +++++++++++++++++++
security/integrity/ima/ima_queue_keys.c | 175 ------------------
8 files changed, 202 insertions(+), 198 deletions(-)
create mode 100644 security/integrity/ima/ima_queue_data.c
delete mode 100644 security/integrity/ima/ima_queue_keys.c
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 12e9250c1bec..953314d145bb 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -322,7 +322,7 @@ config IMA_MEASURE_ASYMMETRIC_KEYS
depends on ASYMMETRIC_PUBLIC_KEY_SUBTYPE=y
default y
-config IMA_QUEUE_EARLY_BOOT_KEYS
+config IMA_QUEUE_EARLY_BOOT_DATA
bool
depends on IMA_MEASURE_ASYMMETRIC_KEYS
depends on SYSTEM_TRUSTED_KEYRING
diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
index 67dabca670e2..6a0f9ef93bf0 100644
--- a/security/integrity/ima/Makefile
+++ b/security/integrity/ima/Makefile
@@ -13,4 +13,4 @@ ima-$(CONFIG_IMA_APPRAISE_MODSIG) += ima_modsig.o
ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o
ima-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
ima-$(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS) += ima_asymmetric_keys.o
-ima-$(CONFIG_IMA_QUEUE_EARLY_BOOT_KEYS) += ima_queue_keys.o
+ima-$(CONFIG_IMA_QUEUE_EARLY_BOOT_DATA) += ima_queue_data.o
\ No newline at end of file
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index ecb0a1e7378f..872a06821c98 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -257,29 +257,29 @@ static inline bool ima_kernel_data_source_is_supported(const char *source)
return false;
}
-#ifdef CONFIG_IMA_QUEUE_EARLY_BOOT_KEYS
+#ifdef CONFIG_IMA_QUEUE_EARLY_BOOT_DATA
/*
- * To track keys that need to be measured.
+ * To track data that need to be measured.
*/
-struct ima_key_entry {
+struct ima_data_entry {
struct list_head list;
void *payload;
size_t payload_len;
- char *keyring_name;
+ const char *event_name;
};
-void ima_init_key_queue(void);
-bool ima_should_queue_key(void);
-bool ima_queue_key(struct key *keyring, const void *payload,
- size_t payload_len);
-void ima_process_queued_keys(void);
+void ima_init_data_queue(void);
+bool ima_should_queue_data(void);
+bool ima_queue_data(const char *event_name, const void *payload,
+ size_t payload_len);
+void ima_process_queued_data(void);
#else
-static inline void ima_init_key_queue(void) {}
-static inline bool ima_should_queue_key(void) { return false; }
-static inline bool ima_queue_key(struct key *keyring,
- const void *payload,
- size_t payload_len) { return false; }
-static inline void ima_process_queued_keys(void) {}
-#endif /* CONFIG_IMA_QUEUE_EARLY_BOOT_KEYS */
+static inline void ima_init_data_queue(void) {}
+static inline bool ima_should_queue_data(void) { return false; }
+static inline bool ima_queue_data(const char *event_name,
+ const void *payload,
+ size_t payload_len) { return false; }
+static inline void ima_process_queued_data(void) {}
+#endif /* CONFIG_IMA_QUEUE_EARLY_BOOT_DATA */
/* LIM API function definitions */
int ima_get_action(struct inode *inode, const struct cred *cred, u32 secid,
diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
index a74095793936..d91fddec5ae8 100644
--- a/security/integrity/ima/ima_asymmetric_keys.c
+++ b/security/integrity/ima/ima_asymmetric_keys.c
@@ -37,9 +37,9 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
if (!payload || (payload_len == 0))
return;
- if (ima_should_queue_key())
- queued = ima_queue_key(keyring, payload, payload_len);
-
+ if (ima_should_queue_data())
+ queued = ima_queue_data(keyring->description, payload,
+ payload_len);
if (queued)
return;
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 4902fe7bd570..892894bf4af3 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -145,7 +145,7 @@ int __init ima_init(void)
if (rc != 0)
return rc;
- ima_init_key_queue();
+ ima_init_data_queue();
return rc;
}
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 0c5202c1f26e..e536ac528558 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -906,7 +906,7 @@ void ima_update_policy(void)
ima_update_policy_flag();
/* Custom IMA policy has been loaded */
- ima_process_queued_keys();
+ ima_process_queued_data();
}
/* Keep the enumeration in sync with the policy_tokens! */
diff --git a/security/integrity/ima/ima_queue_data.c b/security/integrity/ima/ima_queue_data.c
new file mode 100644
index 000000000000..563e26f66180
--- /dev/null
+++ b/security/integrity/ima/ima_queue_data.c
@@ -0,0 +1,179 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2020 Microsoft Corporation
+ *
+ * Author: Lakshmi Ramasubramanian (nramas@linux.microsoft.com)
+ *
+ * File: ima_queue_data.c
+ * Enables deferred processing of data
+ */
+
+#include <linux/workqueue.h>
+#include "ima.h"
+
+/*
+ * Flag to indicate whether data can be processed
+ * right away or should be queued for processing later.
+ */
+static bool ima_process_data;
+
+/*
+ * To synchronize access to the list of data that need to be measured
+ */
+static DEFINE_MUTEX(ima_data_lock);
+static LIST_HEAD(ima_queued_data);
+
+/*
+ * If custom IMA policy is not loaded then data queued up
+ * for measurement should be freed. This worker is used
+ * for handling this scenario.
+ */
+static long ima_data_queue_timeout = 300000; /* 5 Minutes */
+static void ima_data_handler(struct work_struct *work);
+static DECLARE_DELAYED_WORK(ima_data_delayed_work, ima_data_handler);
+static bool timer_expired;
+
+/*
+ * This worker function frees data that may still be
+ * queued up in case custom IMA policy was not loaded.
+ */
+static void ima_data_handler(struct work_struct *work)
+{
+ timer_expired = true;
+ ima_process_queued_data();
+}
+
+/*
+ * This function sets up a worker to free queued data in case
+ * custom IMA policy was never loaded.
+ */
+void ima_init_data_queue(void)
+{
+ schedule_delayed_work(&ima_data_delayed_work,
+ msecs_to_jiffies(ima_data_queue_timeout));
+}
+
+static void ima_free_data_entry(struct ima_data_entry *entry)
+{
+ if (!entry)
+ return;
+
+ kvfree(entry->payload);
+ kfree(entry->event_name);
+ kfree(entry);
+}
+
+static void *ima_kvmemdup(const void *src, size_t len)
+{
+ void *p = kvmalloc(len, GFP_KERNEL);
+
+ if (p)
+ memcpy(p, src, len);
+ return p;
+}
+
+static struct ima_data_entry *ima_alloc_data_entry(const char *event_name,
+ const void *payload,
+ size_t payload_len)
+{
+ struct ima_data_entry *entry;
+
+ entry = kzalloc(sizeof(*entry), GFP_KERNEL);
+ if (!entry)
+ goto out;
+
+ /*
+ * Payload size may exceed the limit supported by kmalloc.
+ * So use kvmalloc instead.
+ */
+ entry->payload = ima_kvmemdup(payload, payload_len);
+ entry->event_name = kstrdup(event_name, GFP_KERNEL);
+ entry->payload_len = payload_len;
+
+ if (!entry->payload || !entry->event_name)
+ goto out;
+
+ INIT_LIST_HEAD(&entry->list);
+ return entry;
+
+out:
+ integrity_audit_message(AUDIT_INTEGRITY_PCR, NULL,
+ event_name, func_measure_str(KEY_CHECK),
+ "ENOMEM", -ENOMEM, 0, -ENOMEM);
+ ima_free_data_entry(entry);
+ return NULL;
+}
+
+bool ima_queue_data(const char *event_name, const void *payload,
+ size_t payload_len)
+{
+ bool queued = false;
+ struct ima_data_entry *entry;
+
+ entry = ima_alloc_data_entry(event_name, payload, payload_len);
+ if (!entry)
+ return false;
+
+ mutex_lock(&ima_data_lock);
+ if (!ima_process_data) {
+ list_add_tail(&entry->list, &ima_queued_data);
+ queued = true;
+ }
+ mutex_unlock(&ima_data_lock);
+
+ if (!queued)
+ ima_free_data_entry(entry);
+
+ return queued;
+}
+
+/*
+ * ima_process_queued_data() - process data queued for measurement
+ *
+ * This function sets ima_process_data to true and processes queued data.
+ * From here on data will be processed right away (not queued).
+ */
+void ima_process_queued_data(void)
+{
+ struct ima_data_entry *entry, *tmp;
+ bool process = false;
+
+ if (ima_process_data)
+ return;
+
+ /*
+ * Since ima_process_data is set to true, any new data will be
+ * processed immediately and not be queued to ima_queued_data list.
+ * First one setting the ima_process_data flag to true will
+ * process the queued data.
+ */
+ mutex_lock(&ima_data_lock);
+ if (!ima_process_data) {
+ ima_process_data = true;
+ process = true;
+ }
+ mutex_unlock(&ima_data_lock);
+
+ if (!process)
+ return;
+
+ if (!timer_expired)
+ cancel_delayed_work_sync(&ima_data_delayed_work);
+
+ list_for_each_entry_safe(entry, tmp, &ima_queued_data, list) {
+ if (!timer_expired)
+ process_buffer_measurement(NULL, entry->payload,
+ entry->payload_len,
+ entry->event_name,
+ KEY_CHECK, 0,
+ entry->event_name,
+ false);
+ list_del(&entry->list);
+ ima_free_data_entry(entry);
+ }
+}
+
+inline bool ima_should_queue_data(void)
+{
+ return !ima_process_data;
+}
diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c
deleted file mode 100644
index c2f2ad34f9b7..000000000000
--- a/security/integrity/ima/ima_queue_keys.c
+++ /dev/null
@@ -1,175 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0+
-/*
- * Copyright (C) 2019 Microsoft Corporation
- *
- * Author: Lakshmi Ramasubramanian (nramas@linux.microsoft.com)
- *
- * File: ima_queue_keys.c
- * Enables deferred processing of keys
- */
-
-#include <linux/workqueue.h>
-#include <keys/asymmetric-type.h>
-#include "ima.h"
-
-/*
- * Flag to indicate whether a key can be processed
- * right away or should be queued for processing later.
- */
-static bool ima_process_keys;
-
-/*
- * To synchronize access to the list of keys that need to be measured
- */
-static DEFINE_MUTEX(ima_keys_lock);
-static LIST_HEAD(ima_keys);
-
-/*
- * If custom IMA policy is not loaded then keys queued up
- * for measurement should be freed. This worker is used
- * for handling this scenario.
- */
-static long ima_key_queue_timeout = 300000; /* 5 Minutes */
-static void ima_keys_handler(struct work_struct *work);
-static DECLARE_DELAYED_WORK(ima_keys_delayed_work, ima_keys_handler);
-static bool timer_expired;
-
-/*
- * This worker function frees keys that may still be
- * queued up in case custom IMA policy was not loaded.
- */
-static void ima_keys_handler(struct work_struct *work)
-{
- timer_expired = true;
- ima_process_queued_keys();
-}
-
-/*
- * This function sets up a worker to free queued keys in case
- * custom IMA policy was never loaded.
- */
-void ima_init_key_queue(void)
-{
- schedule_delayed_work(&ima_keys_delayed_work,
- msecs_to_jiffies(ima_key_queue_timeout));
-}
-
-static void ima_free_key_entry(struct ima_key_entry *entry)
-{
- if (entry) {
- kfree(entry->payload);
- kfree(entry->keyring_name);
- kfree(entry);
- }
-}
-
-static struct ima_key_entry *ima_alloc_key_entry(struct key *keyring,
- const void *payload,
- size_t payload_len)
-{
- int rc = 0;
- const char *audit_cause = "ENOMEM";
- struct ima_key_entry *entry;
-
- entry = kzalloc(sizeof(*entry), GFP_KERNEL);
- if (entry) {
- entry->payload = kmemdup(payload, payload_len, GFP_KERNEL);
- entry->keyring_name = kstrdup(keyring->description,
- GFP_KERNEL);
- entry->payload_len = payload_len;
- }
-
- if ((entry == NULL) || (entry->payload == NULL) ||
- (entry->keyring_name == NULL)) {
- rc = -ENOMEM;
- goto out;
- }
-
- INIT_LIST_HEAD(&entry->list);
-
-out:
- if (rc) {
- integrity_audit_message(AUDIT_INTEGRITY_PCR, NULL,
- keyring->description,
- func_measure_str(KEY_CHECK),
- audit_cause, rc, 0, rc);
- ima_free_key_entry(entry);
- entry = NULL;
- }
-
- return entry;
-}
-
-bool ima_queue_key(struct key *keyring, const void *payload,
- size_t payload_len)
-{
- bool queued = false;
- struct ima_key_entry *entry;
-
- entry = ima_alloc_key_entry(keyring, payload, payload_len);
- if (!entry)
- return false;
-
- mutex_lock(&ima_keys_lock);
- if (!ima_process_keys) {
- list_add_tail(&entry->list, &ima_keys);
- queued = true;
- }
- mutex_unlock(&ima_keys_lock);
-
- if (!queued)
- ima_free_key_entry(entry);
-
- return queued;
-}
-
-/*
- * ima_process_queued_keys() - process keys queued for measurement
- *
- * This function sets ima_process_keys to true and processes queued keys.
- * From here on keys will be processed right away (not queued).
- */
-void ima_process_queued_keys(void)
-{
- struct ima_key_entry *entry, *tmp;
- bool process = false;
-
- if (ima_process_keys)
- return;
-
- /*
- * Since ima_process_keys is set to true, any new key will be
- * processed immediately and not be queued to ima_keys list.
- * First one setting the ima_process_keys flag to true will
- * process the queued keys.
- */
- mutex_lock(&ima_keys_lock);
- if (!ima_process_keys) {
- ima_process_keys = true;
- process = true;
- }
- mutex_unlock(&ima_keys_lock);
-
- if (!process)
- return;
-
- if (!timer_expired)
- cancel_delayed_work_sync(&ima_keys_delayed_work);
-
- list_for_each_entry_safe(entry, tmp, &ima_keys, list) {
- if (!timer_expired)
- process_buffer_measurement(NULL, entry->payload,
- entry->payload_len,
- entry->keyring_name,
- KEY_CHECK, 0,
- entry->keyring_name,
- false);
- list_del(&entry->list);
- ima_free_key_entry(entry);
- }
-}
-
-inline bool ima_should_queue_key(void)
-{
- return !ima_process_keys;
-}
--
2.28.0
^ permalink raw reply related
* Re: [PATCH v2] netlabel: remove unused param from audit_log_format()
From: David Miller @ 2020-08-28 16:09 UTC (permalink / raw)
To: alex.dewar90; +Cc: paul, kuba, netdev, linux-security-module, linux-kernel
In-Reply-To: <20200828135523.12867-1-alex.dewar90@gmail.com>
From: Alex Dewar <alex.dewar90@gmail.com>
Date: Fri, 28 Aug 2020 14:55:23 +0100
> Commit d3b990b7f327 ("netlabel: fix problems with mapping removal")
> added a check to return an error if ret_val != 0, before ret_val is
> later used in a log message. Now it will unconditionally print "...
> res=1". So just drop the check.
>
> Addresses-Coverity: ("Dead code")
> Fixes: d3b990b7f327 ("netlabel: fix problems with mapping removal")
> Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
> ---
> v2: Still print the res field, because it's useful (Paul)
Applied to net-next, thank you.
^ permalink raw reply
* Re: [PATCH v2] block: grant IOPRIO_CLASS_RT to CAP_SYS_NICE
From: Bart Van Assche @ 2020-08-30 1:00 UTC (permalink / raw)
To: Khazhismel Kumykov, Jens Axboe
Cc: Serge Hallyn, Paolo Valente, linux-block, linux-kernel,
linux-security-module, linux-api
In-Reply-To: <20200824221034.2170308-1-khazhy@google.com>
On 2020-08-24 15:10, Khazhismel Kumykov wrote:
> CAP_SYS_ADMIN is too broad, and ionice fits into CAP_SYS_NICE's grouping.
>
> Retain CAP_SYS_ADMIN permission for backwards compatibility.
>
> Signed-off-by: Khazhismel Kumykov <khazhy@google.com>
> ---
> block/ioprio.c | 2 +-
> include/uapi/linux/capability.h | 2 ++
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> v2: fix embarrassing logic mistake
> diff --git a/block/ioprio.c b/block/ioprio.c
> index 77bcab11dce5..276496246fe9 100644
> --- a/block/ioprio.c
> +++ b/block/ioprio.c
> @@ -69,7 +69,7 @@ int ioprio_check_cap(int ioprio)
>
> switch (class) {
> case IOPRIO_CLASS_RT:
> - if (!capable(CAP_SYS_ADMIN))
> + if (!capable(CAP_SYS_NICE) && !capable(CAP_SYS_ADMIN))
> return -EPERM;
> /* fall through */
> /* rt has prio field too */
> diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
> index 395dd0df8d08..c6ca33034147 100644
> --- a/include/uapi/linux/capability.h
> +++ b/include/uapi/linux/capability.h
> @@ -288,6 +288,8 @@ struct vfs_ns_cap_data {
> processes and setting the scheduling algorithm used by another
> process. */
> /* Allow setting cpu affinity on other processes */
> +/* Allow setting realtime ioprio class */
> +/* Allow setting ioprio class on other processes */
>
> #define CAP_SYS_NICE 23
From https://www.kernel.org/doc/man-pages/linux-api-ml.html:
"all Linux kernel patches that change userspace interfaces should be CCed
to linux-api@vger.kernel.org"
So I have added the linux-api mailing list to the Cc-list. Anyway:
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
^ permalink raw reply
* ping//Re: [PATCH] security: fix some spelling mistakes in the comments by codespell
From: Xiaoming Ni @ 2020-08-31 1:01 UTC (permalink / raw)
To: serge, jmorris, zohar, dmitry.kasatkin, casey, takedakn,
penguin-kernel, linux-security-module, linux-kernel,
linux-integrity
Cc: wangle6
In-Reply-To: <20200822030534.2577-1-nixiaoming@huawei.com>
ping
On 2020/8/22 11:05, Xiaoming Ni wrote:
> ecurity/commoncap.c: capabilties ==> capabilities
> security/lsm_audit.c: programers ==> programmers
> security/tomoyo/audit.c: stuct ==> struct
> security/tomoyo/common.c: Poiter ==> Pointer
> security/smack/smack_lsm.c: overriden ==> overridden overridden
> security/smack/smackfs.c: overriden ==> overridden
> security/integrity/ima/ima_template_lib.c: algoritm ==> algorithm
>
> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
> ---
> security/commoncap.c | 2 +-
> security/integrity/ima/ima_template_lib.c | 2 +-
> security/lsm_audit.c | 2 +-
> security/smack/smack_lsm.c | 2 +-
> security/smack/smackfs.c | 2 +-
> security/tomoyo/audit.c | 2 +-
> security/tomoyo/common.c | 2 +-
> 7 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/security/commoncap.c b/security/commoncap.c
> index 59bf3c1674c8..2c3a0f1b6876 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -1046,7 +1046,7 @@ int cap_task_fix_setuid(struct cred *new, const struct cred *old, int flags)
> break;
>
> case LSM_SETID_FS:
> - /* juggle the capabilties to follow FSUID changes, unless
> + /* juggle the capabilities to follow FSUID changes, unless
> * otherwise suppressed
> *
> * FIXME - is fsuser used for all CAP_FS_MASK capabilities?
> diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
> index c022ee9e2a4e..9513564ee0b2 100644
> --- a/security/integrity/ima/ima_template_lib.c
> +++ b/security/integrity/ima/ima_template_lib.c
> @@ -231,7 +231,7 @@ static int ima_eventdigest_init_common(const u8 *digest, u32 digestsize,
> * digest formats:
> * - DATA_FMT_DIGEST: digest
> * - DATA_FMT_DIGEST_WITH_ALGO: [<hash algo>] + ':' + '\0' + digest,
> - * where <hash algo> is provided if the hash algoritm is not
> + * where <hash algo> is provided if the hash algorithm is not
> * SHA1 or MD5
> */
> u8 buffer[CRYPTO_MAX_ALG_NAME + 2 + IMA_MAX_DIGEST_SIZE] = { 0 };
> diff --git a/security/lsm_audit.c b/security/lsm_audit.c
> index 53d0d183db8f..a0ff2e441069 100644
> --- a/security/lsm_audit.c
> +++ b/security/lsm_audit.c
> @@ -212,7 +212,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
> char comm[sizeof(current->comm)];
>
> /*
> - * To keep stack sizes in check force programers to notice if they
> + * To keep stack sizes in check force programmers to notice if they
> * start making this union too large! See struct lsm_network_audit
> * as an example of how to deal with large data.
> */
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 8c0893eb5aa8..960d4641239c 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1784,7 +1784,7 @@ static int smack_file_send_sigiotask(struct task_struct *tsk,
> */
> file = container_of(fown, struct file, f_owner);
>
> - /* we don't log here as rc can be overriden */
> + /* we don't log here as rc can be overridden */
> blob = smack_file(file);
> skp = *blob;
> rc = smk_access(skp, tkp, MAY_DELIVER, NULL);
> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> index 9c4308077574..5c581ec814ac 100644
> --- a/security/smack/smackfs.c
> +++ b/security/smack/smackfs.c
> @@ -111,7 +111,7 @@ struct smack_known *smack_syslog_label;
> /*
> * Ptrace current rule
> * SMACK_PTRACE_DEFAULT regular smack ptrace rules (/proc based)
> - * SMACK_PTRACE_EXACT labels must match, but can be overriden with
> + * SMACK_PTRACE_EXACT labels must match, but can be overridden with
> * CAP_SYS_PTRACE
> * SMACK_PTRACE_DRACONIAN lables must match, CAP_SYS_PTRACE has no effect
> */
> diff --git a/security/tomoyo/audit.c b/security/tomoyo/audit.c
> index 3c96e8402e94..b51bad121c11 100644
> --- a/security/tomoyo/audit.c
> +++ b/security/tomoyo/audit.c
> @@ -311,7 +311,7 @@ static LIST_HEAD(tomoyo_log);
> /* Lock for "struct list_head tomoyo_log". */
> static DEFINE_SPINLOCK(tomoyo_log_lock);
>
> -/* Length of "stuct list_head tomoyo_log". */
> +/* Length of "struct list_head tomoyo_log". */
> static unsigned int tomoyo_log_count;
>
> /**
> diff --git a/security/tomoyo/common.c b/security/tomoyo/common.c
> index 4bee32bfe16d..38bdc0ffc312 100644
> --- a/security/tomoyo/common.c
> +++ b/security/tomoyo/common.c
> @@ -2608,7 +2608,7 @@ ssize_t tomoyo_read_control(struct tomoyo_io_buffer *head, char __user *buffer,
> /**
> * tomoyo_parse_policy - Parse a policy line.
> *
> - * @head: Poiter to "struct tomoyo_io_buffer".
> + * @head: Pointer to "struct tomoyo_io_buffer".
> * @line: Line to parse.
> *
> * Returns 0 on success, negative value otherwise.
>
^ permalink raw reply
* RE: [PATCH 03/11] evm: Refuse EVM_ALLOW_METADATA_WRITES only if the HMAC key is loaded
From: Roberto Sassu @ 2020-08-31 8:24 UTC (permalink / raw)
To: Mimi Zohar, mjg59@google.com
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, stable@vger.kernel.org,
Silviu Vlasceanu
In-Reply-To: <caedd49bc2080a2fb8b16b9ecacab67d11e68fd7.camel@linux.ibm.com>
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Friday, August 21, 2020 10:15 PM
> Hi Roberto,
>
> On Thu, 2020-06-18 at 18:01 +0200, Roberto Sassu wrote:
> > Granting metadata write is safe if the HMAC key is not loaded, as it won't
> > let an attacker obtain a valid HMAC from corrupted xattrs.
> evm_write_key()
> > however does not allow it if any key is loaded, including a public key,
> > which should not be a problem.
> >
>
> Why is the existing hebavior a problem? What is the problem being
> solved?
Hi Mimi
currently it is not possible to set EVM_ALLOW_METADATA_WRITES when
only a public key is loaded and the HMAC key is not. The patch removes
this limitation.
> > This patch allows setting EVM_ALLOW_METADATA_WRITES if the
> EVM_INIT_HMAC
> > flag is not set.
> >
> > Cc: stable@vger.kernel.org # 4.16.x
> > Fixes: ae1ba1676b88e ("EVM: Allow userland to permit modification of
> EVM-protected metadata")
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> > security/integrity/evm/evm_secfs.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/security/integrity/evm/evm_secfs.c
> b/security/integrity/evm/evm_secfs.c
> > index cfc3075769bb..92fe26ace797 100644
> > --- a/security/integrity/evm/evm_secfs.c
> > +++ b/security/integrity/evm/evm_secfs.c
> > @@ -84,7 +84,7 @@ static ssize_t evm_write_key(struct file *file, const
> char __user *buf,
> > * keys are loaded.
> > */
> > if ((i & EVM_ALLOW_METADATA_WRITES) &&
> > - ((evm_initialized & EVM_KEY_MASK) != 0) &&
> > + ((evm_initialized & EVM_INIT_HMAC) != 0) &&
> > !(evm_initialized & EVM_ALLOW_METADATA_WRITES))
> > return -EPERM;
>
> >
>
> Documentation/ABI/testing/evm needs to be updated as well.
Ok.
Thanks
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
> thanks,
>
> Mimi
>
>
^ permalink raw reply
* RE: [PATCH 06/11] evm: Allow setxattr() and setattr() if metadata digest won't change
From: Roberto Sassu @ 2020-08-31 8:51 UTC (permalink / raw)
To: Mimi Zohar, mjg59@google.com
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu
In-Reply-To: <802588e6892951076babbc48aa0320032e4b1926.camel@linux.ibm.com>
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Monday, August 24, 2020 2:17 PM
> On Thu, 2020-06-18 at 18:04 +0200, Roberto Sassu wrote:
> > If metadata are immutable, they cannot be changed. If metadata are
> already
> > set to the final value before cp and tar restore the value from the source,
> > those applications display an error even if the operation is legitimate
> > (they don't change the value).
>
> "metadata" is singular. The first sentence would be clearer by using
> the specific metadata. What problem are you trying to solve? It
> doesn't look like you're trying to solve the problem of writing the EVM
> portable signatures without an exiting HMAC.
>
> >
> > This patch determines whether setxattr()/setattr() change metadata and,
> if
> > not, allows the operations even if metadata are immutable.
>
> Doesn't setxattr/setattr always change file metadata? Please describe
> the real problem.
Yes. The problem is that tar/cp change metadata even if its value is
already correct after the file has been created. These operations
will be denied because metadata is immutable and verification
succeeds.
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> > security/integrity/evm/evm_main.c | 72
> +++++++++++++++++++++++++++++++
> > 1 file changed, 72 insertions(+)
> >
> > diff --git a/security/integrity/evm/evm_main.c
> b/security/integrity/evm/evm_main.c
> > index 30072030f05d..41cc6a4aaaab 100644
> > --- a/security/integrity/evm/evm_main.c
> > +++ b/security/integrity/evm/evm_main.c
> > @@ -18,6 +18,7 @@
> > #include <linux/integrity.h>
> > #include <linux/evm.h>
> > #include <linux/magic.h>
> > +#include <linux/posix_acl_xattr.h>
> >
> > #include <crypto/hash.h>
> > #include <crypto/hash_info.h>
> > @@ -305,6 +306,56 @@ static enum integrity_status
> evm_verify_current_integrity(struct dentry *dentry)
> > return evm_verify_hmac(dentry, NULL, NULL, 0, NULL);
> > }
> >
> > +static int evm_xattr_acl_change(struct dentry *dentry, const char
> *xattr_name,
> > + const void *xattr_value, size_t
> xattr_value_len)
> > +{
> > + umode_t mode;
> > + struct posix_acl *acl = NULL, *acl_res;
> > + struct inode *inode = d_backing_inode(dentry);
> > + int rc;
> > +
> > + /* UID/GID in ACL have been already converted from user to init ns
> */
> > + acl = posix_acl_from_xattr(&init_user_ns, xattr_value,
> xattr_value_len);
> > + if (!acl)
> > + return 1;
> > +
> > + acl_res = acl;
> > + rc = posix_acl_update_mode(inode, &mode, &acl_res);
> > +
> > + posix_acl_release(acl);
> > +
> > + if (rc)
> > + return 1;
> > +
> > + if (acl_res && inode->i_mode != mode)
> > + return 1;
> > +
> > + return 0;
> > +}
> > +
> > +static int evm_xattr_change(struct dentry *dentry, const char
> *xattr_name,
> > + const void *xattr_value, size_t xattr_value_len)
> > +{
> > + char *xattr_data = NULL;
> > + int rc = 0;
> > +
> > + if (posix_xattr_acl(xattr_name))
> > + return evm_xattr_acl_change(dentry, xattr_name,
> xattr_value,
> > + xattr_value_len);
> > +
> > + rc = vfs_getxattr_alloc(dentry, xattr_name, &xattr_data, 0,
> GFP_NOFS);
> > + if (rc < 0)
> > + return 1;
> > +
> > + if (rc == xattr_value_len)
> > + rc = memcmp(xattr_value, xattr_data, rc);
> > + else
> > + rc = 1;
> > +
> > + kfree(xattr_data);
> > + return rc;
> > +}
> > +
> > /*
> > * evm_protect_xattr - protect the EVM extended attribute
> > *
> > @@ -361,6 +412,10 @@ static int evm_protect_xattr(struct dentry
> *dentry, const char *xattr_name,
> > if (evm_status == INTEGRITY_FAIL_IMMUTABLE)
> > return 0;
> >
> > + if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
> > + !evm_xattr_change(dentry, xattr_name, xattr_value,
> xattr_value_len))
> > + return 0;
> > +
> > if (evm_status != INTEGRITY_PASS)
> > integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> d_backing_inode(dentry),
> > dentry->d_name.name,
> "appraise_metadata",
> > @@ -477,6 +532,19 @@ void evm_inode_post_removexattr(struct dentry
> *dentry, const char *xattr_name)
> > evm_update_evmxattr(dentry, xattr_name, NULL, 0);
> > }
> >
> > +static int evm_attr_change(struct dentry *dentry, struct iattr *attr)
>
> static functions don't normally require a function comment, but in this
> case it wouldn't hurt to clarify why the uid, gid, mode bits are set,
> but aren't being modified.
> Similarly a function comment would help with the readability of
> evm_xattr_acl_change().
Ok.
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
> Mimi
>
> > +{
> > + struct inode *inode = d_backing_inode(dentry);
> > + unsigned int ia_valid = attr->ia_valid;
> > +
> > + if ((!(ia_valid & ATTR_UID) || uid_eq(attr->ia_uid, inode->i_uid))
> &&
> > + (!(ia_valid & ATTR_GID) || gid_eq(attr->ia_gid, inode->i_gid)) &&
> > + (!(ia_valid & ATTR_MODE) || attr->ia_mode == inode->i_mode))
> > + return 0;
> > +
> > + return 1;
> > +}
> > +
> > /**
> > * evm_inode_setattr - prevent updating an invalid EVM extended
> attribute
> > * @dentry: pointer to the affected dentry
> > @@ -506,6 +574,10 @@ int evm_inode_setattr(struct dentry *dentry,
> struct iattr *attr)
> > (evm_status == INTEGRITY_FAIL_IMMUTABLE))
> > return 0;
> >
> > + if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
> > + !evm_attr_change(dentry, attr))
> > + return 0;
> > +
> > integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> d_backing_inode(dentry),
> > dentry->d_name.name, "appraise_metadata",
> > integrity_status_msg[evm_status], -EPERM, 0);
>
^ permalink raw reply
* RE: [PATCH 02/11] evm: Load EVM key in ima_load_x509() to avoid appraisal
From: Roberto Sassu @ 2020-08-31 9:44 UTC (permalink / raw)
To: Mimi Zohar, mjg59@google.com
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu
In-Reply-To: <8a1773d7707639d275fff138736d57472e26ade5.camel@linux.ibm.com>
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Friday, August 21, 2020 8:45 PM
> On Thu, 2020-06-18 at 18:01 +0200, Roberto Sassu wrote:
> > Public keys do not need to be appraised by IMA as the restriction on the
> > IMA/EVM keyrings ensures that a key is loaded only if it is signed with a
> > key in the primary or secondary keyring.
> >
> > However, when evm_load_x509() is loaded, appraisal is already enabled
> and
> > a valid IMA signature must be added to the EVM key to pass verification.
> >
> > Since the restriction is applied on both IMA and EVM keyrings, it is safe
> > to disable appraisal also when the EVM key is loaded. This patch calls
> > evm_load_x509() inside ima_load_x509() if CONFIG_IMA_LOAD_X509 is
> defined.
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> > security/integrity/iint.c | 2 ++
> > security/integrity/ima/ima_init.c | 4 ++++
> > 2 files changed, 6 insertions(+)
> >
> > diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> > index e12c4900510f..4765a266ba96 100644
> > --- a/security/integrity/iint.c
> > +++ b/security/integrity/iint.c
> > @@ -212,7 +212,9 @@ int integrity_kernel_read(struct file *file, loff_t
> offset,
> > void __init integrity_load_keys(void)
> > {
> > ima_load_x509();
> > +#ifndef CONFIG_IMA_LOAD_X509
> > evm_load_x509();
> > +#endif
> > }
> >
> > static int __init integrity_fs_init(void)
> > diff --git a/security/integrity/ima/ima_init.c
> b/security/integrity/ima/ima_init.c
> > index 4902fe7bd570..9d29a1680da8 100644
> > --- a/security/integrity/ima/ima_init.c
> > +++ b/security/integrity/ima/ima_init.c
> > @@ -106,6 +106,10 @@ void __init ima_load_x509(void)
> >
> > ima_policy_flag &= ~unset_flags;
> > integrity_load_x509(INTEGRITY_KEYRING_IMA,
> CONFIG_IMA_X509_PATH);
> > +
> > + /* load also EVM key to avoid appraisal */
> > + evm_load_x509();
> > +
> > ima_policy_flag |= unset_flags;
> > }
> > #endif
>
> As much as possible IMA and EVM should remain independent of each
> other. Modifying integrity_load_x509() doesn't help. This looks like
> a good reason for calling another EVM function from within IMA.
Can I add your Reviewed-by?
Thanks
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
^ permalink raw reply
* Re: [PATCH v3 2/6] IMA: change process_buffer_measurement return type from void to int
From: Mimi Zohar @ 2020-08-31 11:36 UTC (permalink / raw)
To: Tushar Sugandhi, stephen.smalley.work, casey, agk, snitzer,
gmazyland
Cc: tyhicks, sashal, jmorris, nramas, linux-integrity, selinux,
linux-security-module, linux-kernel, dm-devel
In-Reply-To: <20200828015704.6629-3-tusharsu@linux.microsoft.com>
On Thu, 2020-08-27 at 18:57 -0700, Tushar Sugandhi wrote:
> process_buffer_measurement() does not return the result of the operation.
> Therefore, the consumers of this function cannot act on it, if needed.
>
> Update return type of process_buffer_measurement() from void to int.
Failure to measure may be audited, but should never fail. This is one
of the main differences between secure and trusted boot concepts.
Notice in process_measurement() that -EACCES is only returned for
appraisal.
Returning a failure from process_buffer_measurement() in itself isn't a
problem, as long as the failure isn't returned to the LSM/IMA hook.
However, just as the callers of process_measurement() originally
processed the result, that processing was moved into
process_measurement() [1].
Mimi
[1] 750943a30714 ima: remove enforce checking duplication
>
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> ---
> security/integrity/ima/ima.h | 6 +++---
> security/integrity/ima/ima_main.c | 14 +++++++-------
> 2 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 8875085db689..83ed57147e68 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -265,9 +265,9 @@ void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
> struct evm_ima_xattr_data *xattr_value,
> int xattr_len, const struct modsig *modsig, int pcr,
> struct ima_template_desc *template_desc);
> -void process_buffer_measurement(struct inode *inode, const void *buf, int size,
> - const char *eventname, enum ima_hooks func,
> - int pcr, const char *func_data);
> +int process_buffer_measurement(struct inode *inode, const void *buf, int size,
> + const char *eventname, enum ima_hooks func,
> + int pcr, const char *func_data);
> void ima_audit_measurement(struct integrity_iint_cache *iint,
> const unsigned char *filename);
> int ima_alloc_init_template(struct ima_event_data *event_data,
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index c870fd6d2f83..0979a62a9257 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -736,9 +736,9 @@ int ima_load_data(enum kernel_load_data_id id)
> *
> * Based on policy, the buffer is measured into the ima log.
> */
> -void process_buffer_measurement(struct inode *inode, const void *buf, int size,
> - const char *eventname, enum ima_hooks func,
> - int pcr, const char *func_data)
> +int process_buffer_measurement(struct inode *inode, const void *buf, int size,
> + const char *eventname, enum ima_hooks func,
> + int pcr, const char *func_data)
> {
> int ret = 0;
> const char *audit_cause = "ENOMEM";
> @@ -758,7 +758,7 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size,
> u32 secid;
>
> if (!ima_policy_flag)
> - return;
> + return 0;
>
> /*
> * Both LSM hooks and auxilary based buffer measurements are
> @@ -772,7 +772,7 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size,
> action = ima_get_action(inode, current_cred(), secid, 0, func,
> &pcr, &template, func_data);
> if (!(action & IMA_MEASURE))
> - return;
> + return 0;
> }
>
> if (!pcr)
> @@ -787,7 +787,7 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size,
> pr_err("template %s init failed, result: %d\n",
> (strlen(template->name) ?
> template->name : template->fmt), ret);
> - return;
> + return ret;
> }
> }
>
> @@ -819,7 +819,7 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size,
> func_measure_str(func),
> audit_cause, ret, 0, ret);
>
> - return;
> + return ret;
> }
>
> /**
^ permalink raw reply
* Re: [PATCH v3 1/6] IMA: generalize keyring specific measurement constructs
From: Mimi Zohar @ 2020-08-31 11:55 UTC (permalink / raw)
To: Tushar Sugandhi, stephen.smalley.work, casey, agk, snitzer,
gmazyland
Cc: tyhicks, sashal, jmorris, nramas, linux-integrity, selinux,
linux-security-module, linux-kernel, dm-devel
In-Reply-To: <20200828015704.6629-2-tusharsu@linux.microsoft.com>
On Thu, 2020-08-27 at 18:56 -0700, Tushar Sugandhi wrote:
> IMA functions such as ima_match_keyring(), process_buffer_measurement(),
> ima_match_policy() etc. handle data specific to keyrings. Currently,
> these constructs are not generic to handle any func specific data.
> This makes it harder to extend without code duplication.
>
> Refactor the keyring specific measurement constructs to be generic and
> reusable in other measurement scenarios.
Mostly this patch changes the variable name from keyring to func_data,
which is good. Other changes should be minimized.
>
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> ---
<snip>
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index fe1df373c113..8866e84d0062 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -451,15 +451,21 @@ int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
> }
>
> /**
> - * ima_match_keyring - determine whether the keyring matches the measure rule
> - * @rule: a pointer to a rule
> - * @keyring: name of the keyring to match against the measure rule
> + * ima_match_rule_data - determine whether the given func_data matches
> + * the measure rule data
> + * @rule: IMA policy rule
> + * @opt_list: rule data to match func_data against
> + * @func_data: data to match against the measure rule data
> + * @allow_empty_opt_list: If true matches all func_data
> * @cred: a pointer to a credentials structure for user validation
> *
> - * Returns true if keyring matches one in the rule, false otherwise.
> + * Returns true if func_data matches one in the rule, false otherwise.
> */
> -static bool ima_match_keyring(struct ima_rule_entry *rule,
> - const char *keyring, const struct cred *cred)
> +static bool ima_match_rule_data(struct ima_rule_entry *rule,
> + const struct ima_rule_opt_list *opt_list,
Ok
> + const char *func_data,
> + bool allow_empty_opt_list,
As the policy is loaded, shouldn't the rules should be checked, not
here on usage?
Mimi
> + const struct cred *cred)
> {
> bool matched = false;
> size_t i;
>
^ permalink raw reply
* Re: [PATCH] SELinux: Measure state and hash of policy using IMA
From: Stephen Smalley @ 2020-08-31 14:47 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: Paul Moore, Ondrej Mosnacek, Mimi Zohar, Casey Schaufler,
Tyler Hicks, tusharsu, Sasha Levin, James Morris, linux-integrity,
SElinux list, LSM List, linux-kernel
In-Reply-To: <CAEjxPJ4TkEEKG+pXwUjyysov1s1mFk4jbGGVyC7ghmpfd3TJ4w@mail.gmail.com>
On Wed, Aug 26, 2020 at 8:51 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Tue, Aug 25, 2020 at 4:49 PM Lakshmi Ramasubramanian
> <nramas@linux.microsoft.com> wrote:
> >
> > On 8/24/20 3:18 PM, Paul Moore wrote:
> >
> > Hi Paul,
> >
> > >>>>> Is Ondrej's re-try approach I need to use to workaround policy reload issue?
> > >>>>
> > >>>> No, I think perhaps we should move the mutex to selinux_state instead
> > >>>> of selinux_fs_info. selinux_fs_info has a pointer to selinux_state so
> > >>>> it can then use it indirectly. Note that your patches are going to
> > >>>> conflict with other ongoing work in the selinux next branch that is
> > >>>> refactoring policy load and converting the policy rwlock to RCU.
> > >>>
> > >>> Yeah, and I'm experimenting with a patch on top of Stephen's RCU work
> > >>> that would allow you to do this in a straightforward way without even
> > >>> messing with the fsi->mutex. My patch may or may not be eventually
> > >>> committed, but either way I'd recommend holding off on this for a
> > >>> while until the dust settles around the RCU conversion.
> > >>
> > >> I can make the SELinux\IMA changes in "selinux next branch" taking
> > >> dependencies on Stephen's patches + relevant IMA patches.
> > >
> > > I know it can be frustrating to hear what I'm about to say, but the
> > > best option is probably just to wait a little to let things settle in
> > > the SELinux -next branch. There is a lot of stuff going on right now
> > > with patches flooding in (at least "flooding" from a SELinux kernel
> > > development perspective) and we/I've haven't gotten through all of
> > > them yet.
> > >
> >
> > Could you please let me know when the current set of changes in SELinux
> > next branch would be completed and be ready to take new changes?
> >
> > I mean, roughly - would it be a month from now or you expect that to
> > take longer?
>
> I can't speak for Paul but I would expect it to be sooner rather than
> later. Ondrej has some follow ups on top of my policy rcu conversion
> but then it should be good to go.
I think the major changes are now merged although there are still a
couple of changes coming from Ondrej that could affect your code. For
your purposes, the important things to note are:
1) The mutex has moved from selinux_fs_info to selinux_state and is
now named policy_mutex. You will need to take it around your call to
security_read_policy_kernel().
2) security_policydb_len() was removed and security_read_policy() just
directly reads the policydb len. You can do the same from your
security_read_policy_kernel() variant.
3) Ondrej has a pending change to move the policycap[] array from
selinux_state to selinux_policy so that it can be atomically updated
with the policy.
4) Ondrej has a pending change to eliminate the separate initialized
boolean from selinux_state and just test whether selinux_state.policy
is non-NULL but as long as you are using selinux_initialized() to
test, your code should be unaffected.
^ permalink raw reply
* Re: [PATCH] SELinux: Measure state and hash of policy using IMA
From: Lakshmi Ramasubramanian @ 2020-08-31 16:39 UTC (permalink / raw)
To: Stephen Smalley
Cc: Paul Moore, Ondrej Mosnacek, Mimi Zohar, Casey Schaufler,
Tyler Hicks, tusharsu, Sasha Levin, James Morris, linux-integrity,
SElinux list, LSM List, linux-kernel
In-Reply-To: <CAEjxPJ6GkUot29g5qq2GVYzmY2xwfTvVJkNP2kK54OcW7tkz1Q@mail.gmail.com>
On 8/31/20 7:47 AM, Stephen Smalley wrote:
>>>>
>>>
>>> Could you please let me know when the current set of changes in SELinux
>>> next branch would be completed and be ready to take new changes?
>>>
>>> I mean, roughly - would it be a month from now or you expect that to
>>> take longer?
>>
>> I can't speak for Paul but I would expect it to be sooner rather than
>> later. Ondrej has some follow ups on top of my policy rcu conversion
>> but then it should be good to go.
>
> I think the major changes are now merged although there are still a
> couple of changes coming from Ondrej that could affect your code. For
> your purposes, the important things to note are:
>
> 1) The mutex has moved from selinux_fs_info to selinux_state and is
> now named policy_mutex. You will need to take it around your call to
> security_read_policy_kernel().
>
> 2) security_policydb_len() was removed and security_read_policy() just
> directly reads the policydb len. You can do the same from your
> security_read_policy_kernel() variant.
>
> 3) Ondrej has a pending change to move the policycap[] array from
> selinux_state to selinux_policy so that it can be atomically updated
> with the policy.
>
> 4) Ondrej has a pending change to eliminate the separate initialized
> boolean from selinux_state and just test whether selinux_state.policy
> is non-NULL but as long as you are using selinux_initialized() to
> test, your code should be unaffected.
>
Thanks a lot for the update Stephen.
I will start updating the IMA measurement changes in selinux next
branch. Will post the patches this week.
-lakshmi
^ permalink raw reply
* Re: [PATCH v3 3/6] IMA: update process_buffer_measurement to measure buffer hash
From: Mimi Zohar @ 2020-08-31 17:02 UTC (permalink / raw)
To: Tushar Sugandhi, stephen.smalley.work, casey, agk, snitzer,
gmazyland
Cc: tyhicks, sashal, jmorris, nramas, linux-integrity, selinux,
linux-security-module, linux-kernel, dm-devel
In-Reply-To: <20200828015704.6629-4-tusharsu@linux.microsoft.com>
On Thu, 2020-08-27 at 18:57 -0700, Tushar Sugandhi wrote:
> process_buffer_measurement() currently only measures the input buffer.
> When the buffer being measured is too large, it may result in bloated
> IMA logs.
The subject of this sentence refers to an individual record, while
"bloated" refers to the measurement list. A "bloated" measurement list
would contain too many or unnecessary records. Your concern seems to
be with the size of the individual record, not the number of
measurement list entries.
Measuring the hash of the buffer data is similar to measuring the file
data. In the case of the file data, however, the attestation server
may rely on a white list manifest/DB or the file signature to verify
the file data hash. For buffer measurements, how will the attestation
server ascertain what is a valid buffer hash?
Hint: I assume, correct me if I'm wrong, the measurement list record
template data is not meant to be verified, but used to detect if the "critical data" changed.
Please update the patch description accordingly.
>
> Introduce a boolean parameter measure_buf_hash to support measuring
> hash of a buffer, which would be much smaller, instead of the buffer
> itself.
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> ---
<snip>
> +++ b/security/integrity/ima/ima_main.c
> @@ -733,17 +733,21 @@ int ima_load_data(enum kernel_load_data_id id)
> * @func: IMA hook
> * @pcr: pcr to extend the measurement
> * @func_data: private data specific to @func, can be NULL.
> + * @measure_buf_hash: if set to true - will measure hash of the buf,
> + * instead of buf
> *
> * Based on policy, the buffer is measured into the ima log.
> */
> int process_buffer_measurement(struct inode *inode, const void *buf, int size,
> const char *eventname, enum ima_hooks func,
> - int pcr, const char *func_data)
> + int pcr, const char *func_data,
> + bool measure_buf_hash)
> {
> int ret = 0;
> const char *audit_cause = "ENOMEM";
> struct ima_template_entry *entry = NULL;
> struct integrity_iint_cache iint = {};
> + struct integrity_iint_cache digest_iint = {};
> struct ima_event_data event_data = {.iint = &iint,
> .filename = eventname,
> .buf = buf,
> @@ -752,7 +756,7 @@ int process_buffer_measurement(struct inode *inode, const void *buf, int size,
> struct {
> struct ima_digest_data hdr;
> char digest[IMA_MAX_DIGEST_SIZE];
> - } hash = {};
> + } hash = {}, digest_hash = {};
> int violation = 0;
> int action = 0;
> u32 secid;
> @@ -801,6 +805,24 @@ int process_buffer_measurement(struct inode *inode, const void *buf, int size,
> goto out;
> }
>
> + if (measure_buf_hash) {
> + digest_iint.ima_hash = &digest_hash.hdr;
> + digest_iint.ima_hash->algo = ima_hash_algo;
> + digest_iint.ima_hash->length = hash_digest_size[ima_hash_algo];
> +
> + ret = ima_calc_buffer_hash(hash.hdr.digest,
> + iint.ima_hash->length,
> + digest_iint.ima_hash);
> + if (ret < 0) {
> + audit_cause = "digest_hashing_error";
> + goto out;
> + }
> +
> + event_data.iint = &digest_iint;
> + event_data.buf = hash.hdr.digest;
> + event_data.buf_len = iint.ima_hash->length;
> + }
> +
There seems to be some code and variable duplication by doing it this
way. Copying the caluclated buffer data hash to a temporary buffer
might eliminate it.
> ret = ima_alloc_init_template(&event_data, &entry, template);
> if (ret < 0) {
> audit_cause = "alloc_entry";
^ permalink raw reply
* Re: [PATCH v8 2/3] Teach SELinux about anonymous inodes
From: Stephen Smalley @ 2020-08-31 18:05 UTC (permalink / raw)
To: Lokesh Gidra
Cc: Alexander Viro, James Morris, Casey Schaufler, Eric Biggers,
Serge E. Hallyn, Paul Moore, Eric Paris, Daniel Colascione,
Kees Cook, Eric W. Biederman, KP Singh, David Howells,
Thomas Cedeno, Anders Roxell, Sami Tolvanen, Matthew Garrett,
Aaron Goidel, Randy Dunlap, Joel Fernandes (Google), YueHaibing,
Christian Brauner, Alexei Starovoitov, Alexey Budankov,
Adrian Reber, Aleksa Sarai, Linux FS Devel, linux-kernel,
LSM List, SElinux list, kaleshsingh, calin, surenb,
Nick Kralevich, Jeffrey Vander Stoep, kernel-team,
Daniel Colascione, Andrew Morton
In-Reply-To: <20200827063522.2563293-3-lokeshgidra@google.com>
On Thu, Aug 27, 2020 at 2:35 AM Lokesh Gidra <lokeshgidra@google.com> wrote:
>
> From: Daniel Colascione <dancol@google.com>
>
> This change uses the anon_inodes and LSM infrastructure introduced in
> the previous patch to give SELinux the ability to control
> anonymous-inode files that are created using the new anon_inode_getfd_secure()
> function.
>
> A SELinux policy author detects and controls these anonymous inodes by
> adding a name-based type_transition rule that assigns a new security
> type to anonymous-inode files created in some domain. The name used
> for the name-based transition is the name associated with the
> anonymous inode for file listings --- e.g., "[userfaultfd]" or
> "[perf_event]".
>
> Example:
>
> type uffd_t;
> type_transition sysadm_t sysadm_t : anon_inode uffd_t "[userfaultfd]";
> allow sysadm_t uffd_t:anon_inode { create };
>
> (The next patch in this series is necessary for making userfaultfd
> support this new interface. The example above is just
> for exposition.)
>
> Signed-off-by: Daniel Colascione <dancol@google.com>
> Acked-by: Casey Schaufler <casey@schaufler-ca.com>
> Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> ---
> security/selinux/hooks.c | 53 +++++++++++++++++++++++++++++
> security/selinux/include/classmap.h | 2 ++
> 2 files changed, 55 insertions(+)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index a340986aa92e..b83f56e5ef40 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2926,6 +2926,58 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
> return 0;
> }
>
> +static int selinux_inode_init_security_anon(struct inode *inode,
> + const struct qstr *name,
> + const struct inode *context_inode)
> +{
> + const struct task_security_struct *tsec = selinux_cred(current_cred());
> + struct common_audit_data ad;
> + struct inode_security_struct *isec;
> + int rc;
> +
> + if (unlikely(!selinux_state.initialized))
This should use selinux_initialized(&selinux_state) instead.
^ permalink raw reply
* Re: [PATCH v3 4/6] IMA: add policy to measure critical data from kernel components
From: Mimi Zohar @ 2020-08-31 18:15 UTC (permalink / raw)
To: Tushar Sugandhi, stephen.smalley.work, casey, agk, snitzer,
gmazyland
Cc: tyhicks, sashal, jmorris, nramas, linux-integrity, selinux,
linux-security-module, linux-kernel, dm-devel
In-Reply-To: <20200828015704.6629-5-tusharsu@linux.microsoft.com>
On Thu, 2020-08-27 at 18:57 -0700, Tushar Sugandhi wrote:
> There would be several candidate kernel components suitable for IMA
> measurement. Not all of them would have support for IMA measurement.
> Also, system administrators may not want to measure data for all of
> them, even when they support IMA measurement. An IMA policy specific
> to various kernel components is needed to measure their respective
> critical data.
The base policy rules are wide, but may be constrained by specifying
different options. For example the builtin policy rules cannot be
written in terms LSM labels, which would constrain them. A policy rule
may measure all keyrings or may constrain which keyrings need to be
measured. Measuring critical data is not any different.
Please rewrite the above paragraph accordingly.
>
> Add a new IMA policy "critical_kernel_data_sources" to support measuring
> various critical kernel components. This policy would enable the
> system administrators to limit the measurement to the components,
> if the components support IMA measurement.
"critical_kernel_data_sources" is really wordy. Find a better, self
defining term for describing the type of data, one that isn't so wordy,
and reflect it in the code.
>
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> ---
> Documentation/ABI/testing/ima_policy | 3 +++
> security/integrity/ima/ima_policy.c | 29 +++++++++++++++++++++++++++-
> 2 files changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> index cd572912c593..7ccdc1964e29 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -48,6 +48,9 @@ Description:
> template:= name of a defined IMA template type
> (eg, ima-ng). Only valid when action is "measure".
> pcr:= decimal value
> + critical_kernel_data_sources:= list of kernel
> + components (eg, selinux|apparmor|dm-crypt) that
> + contain data critical to the security of the kernel.
This original policy definition, for the most part, is in Backus–Naur
format. The keyring names is an exception, because it is not limited
to pre-defined kernel objects. The critical data hook is measuring
things in kernel memory. As new calls to measure critical data are
added, new identifiers would be added here.
For example, if SELinux is the first example of measuring critical
data, then the SELinux critical data patch would include
"critical_data:= [selinux]". Each subsequent critical data being
measured would extend this list. At the same time, the list of known
"critical data" defined in patch 6/6 would be updated.
Normally a new feature and the first usage of that feature are included
in the same patch set. Separating them like this makes it difficult to
write, review and upstream.
Mimi
^ permalink raw reply
* Re: [PATCH v8 2/3] Teach SELinux about anonymous inodes
From: Lokesh Gidra @ 2020-08-31 18:21 UTC (permalink / raw)
To: Stephen Smalley
Cc: Alexander Viro, James Morris, Casey Schaufler, Eric Biggers,
Serge E. Hallyn, Paul Moore, Eric Paris, Daniel Colascione,
Kees Cook, Eric W. Biederman, KP Singh, David Howells,
Thomas Cedeno, Anders Roxell, Sami Tolvanen, Matthew Garrett,
Aaron Goidel, Randy Dunlap, Joel Fernandes (Google), YueHaibing,
Christian Brauner, Alexei Starovoitov, Alexey Budankov,
Adrian Reber, Aleksa Sarai, Linux FS Devel, linux-kernel,
LSM List, SElinux list, Kalesh Singh, Calin Juravle,
Suren Baghdasaryan, Nick Kralevich, Jeffrey Vander Stoep,
kernel-team, Daniel Colascione, Andrew Morton
In-Reply-To: <CAEjxPJ7rsyw=AkRUmnshF5gWygHEN1ahKi5uD9FtYovz0JRKCQ@mail.gmail.com>
On Mon, Aug 31, 2020 at 11:05 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Thu, Aug 27, 2020 at 2:35 AM Lokesh Gidra <lokeshgidra@google.com> wrote:
> >
> > From: Daniel Colascione <dancol@google.com>
> >
> > This change uses the anon_inodes and LSM infrastructure introduced in
> > the previous patch to give SELinux the ability to control
> > anonymous-inode files that are created using the new anon_inode_getfd_secure()
> > function.
> >
> > A SELinux policy author detects and controls these anonymous inodes by
> > adding a name-based type_transition rule that assigns a new security
> > type to anonymous-inode files created in some domain. The name used
> > for the name-based transition is the name associated with the
> > anonymous inode for file listings --- e.g., "[userfaultfd]" or
> > "[perf_event]".
> >
> > Example:
> >
> > type uffd_t;
> > type_transition sysadm_t sysadm_t : anon_inode uffd_t "[userfaultfd]";
> > allow sysadm_t uffd_t:anon_inode { create };
> >
> > (The next patch in this series is necessary for making userfaultfd
> > support this new interface. The example above is just
> > for exposition.)
> >
> > Signed-off-by: Daniel Colascione <dancol@google.com>
> > Acked-by: Casey Schaufler <casey@schaufler-ca.com>
> > Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > ---
> > security/selinux/hooks.c | 53 +++++++++++++++++++++++++++++
> > security/selinux/include/classmap.h | 2 ++
> > 2 files changed, 55 insertions(+)
> >
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index a340986aa92e..b83f56e5ef40 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -2926,6 +2926,58 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
> > return 0;
> > }
> >
> > +static int selinux_inode_init_security_anon(struct inode *inode,
> > + const struct qstr *name,
> > + const struct inode *context_inode)
> > +{
> > + const struct task_security_struct *tsec = selinux_cred(current_cred());
> > + struct common_audit_data ad;
> > + struct inode_security_struct *isec;
> > + int rc;
> > +
> > + if (unlikely(!selinux_state.initialized))
>
> This should use selinux_initialized(&selinux_state) instead.
Thanks for the review. I'll make the change in the next version.
Kindly have a look at other patches in the series as well.
^ permalink raw reply
* Re: [PATCH v3 5/6] IMA: add hook to measure critical data from kernel components
From: Mimi Zohar @ 2020-08-31 18:23 UTC (permalink / raw)
To: Tushar Sugandhi, stephen.smalley.work, casey, agk, snitzer,
gmazyland
Cc: tyhicks, sashal, jmorris, nramas, linux-integrity, selinux,
linux-security-module, linux-kernel, dm-devel
In-Reply-To: <20200828015704.6629-6-tusharsu@linux.microsoft.com>
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 52cbbc1f7ea2..a889bf40cb7e 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -869,6 +869,30 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
> fdput(f);
> }
>
> +/**
> + * ima_measure_critical_data - measure critical data
> + * @event_name: name for the given data
> + * @event_data_source: name of the event data source
> + * @buf: pointer to buffer containing data to measure
> + * @buf_len: length of buffer(in bytes)
> + * @measure_buf_hash: if set to true - will measure hash of the buf,
> + * instead of buf
> + *
> + * Buffers can only be measured, not appraised.
> + */
> +int ima_measure_critical_data(const char *event_name,
> + const char *event_data_source,
> + const void *buf, int buf_len,
> + bool measure_buf_hash)
> +{
> + if (!event_name || !event_data_source || !buf || !buf_len)
> + return -EINVAL;
> +
> + return process_buffer_measurement(NULL, buf, buf_len, event_name,
> + CRITICAL_DATA, 0, event_data_source,
> + measure_buf_hash);
This is exactly what I'm concerned about. Failure to measure data may
be audited, but should never fail.
Mimi
> +}
^ permalink raw reply
* Re: [PATCH 02/11] evm: Load EVM key in ima_load_x509() to avoid appraisal
From: Mimi Zohar @ 2020-08-31 19:26 UTC (permalink / raw)
To: Roberto Sassu, mjg59@google.com
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu
In-Reply-To: <a4bf0f73e0854cf18d942330a7543d9d@huawei.com>
On Mon, 2020-08-31 at 09:44 +0000, Roberto Sassu wrote:
>
> > As much as possible IMA and EVM should remain independent of each
> > other. Modifying integrity_load_x509() doesn't help. This looks like
> > a good reason for calling another EVM function from within IMA.
>
> Can I add your Reviewed-by?
Yes, that's fine.
Mimi
^ permalink raw reply
* Re: [PATCH 03/11] evm: Refuse EVM_ALLOW_METADATA_WRITES only if the HMAC key is loaded
From: Mimi Zohar @ 2020-08-31 21:31 UTC (permalink / raw)
To: Roberto Sassu, mjg59@google.com
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, stable@vger.kernel.org,
Silviu Vlasceanu
In-Reply-To: <0c1c8fb398c340d89531360be7e3418b@huawei.com>
On Mon, 2020-08-31 at 08:24 +0000, Roberto Sassu wrote:
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Friday, August 21, 2020 10:15 PM
> > Hi Roberto,
> >
> > On Thu, 2020-06-18 at 18:01 +0200, Roberto Sassu wrote:
> > > Granting metadata write is safe if the HMAC key is not loaded, as it won't
> > > let an attacker obtain a valid HMAC from corrupted xattrs.
> > evm_write_key()
> > > however does not allow it if any key is loaded, including a public key,
> > > which should not be a problem.
> > >
> >
> > Why is the existing hebavior a problem? What is the problem being
> > solved?
>
> Hi Mimi
>
> currently it is not possible to set EVM_ALLOW_METADATA_WRITES when
> only a public key is loaded and the HMAC key is not. The patch removes
> this limitation.
Yes, I understand. You're describing "what" the problem is, not "why"
this is a problem. Support for loading EVM HMAC and x509 certificates
isn't new. Please add a line or two prior to this paragraph providing
the context for why this is now a problem.
Is the problem related to previoulsy not beginning EVM verification
until after the EVM HMAC key was loaded? Or perhaps EVM signatures
were not that common since they weren't portable. Now, with portable
and immutable signatures loading x509 certificates is more common.
thanks,
Mimi
^ permalink raw reply
* RE: [PATCH 07/11] evm: Set IMA_CHANGE_XATTR/ATTR bit if EVM_ALLOW_METADATA_WRITES is set
From: Roberto Sassu @ 2020-09-01 9:08 UTC (permalink / raw)
To: Mimi Zohar, mjg59@google.com
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu,
stable@vger.kernel.org
In-Reply-To: <67cafcf63daf8e418871eb5302e7327ba851e253.camel@linux.ibm.com>
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Monday, August 24, 2020 2:18 PM
> On Thu, 2020-06-18 at 18:04 +0200, Roberto Sassu wrote:
> > When EVM_ALLOW_METADATA_WRITES is set, EVM allows any operation
> on
> > metadata. Its main purpose is to allow users to freely set metadata when
> > they are protected by a portable signature, until the HMAC key is loaded.
> >
> > However, IMA is not notified about metadata changes and, after the first
> > appraisal, always allows access to the files without checking metadata
> > again.
>
> ^after the first successful appraisal
> >
> > This patch checks in evm_reset_status() if EVM_ALLOW_METADATA
> WRITES is
> > enabled and if it is, sets the IMA_CHANGE_XATTR/ATTR bits depending on
> the
> > operation performed. At the next appraisal, metadata are revalidated.
>
> EVM modifying IMA bits crosses the boundary between EVM and IMA.
> There
> is already an IMA post_setattr hook. IMA could reset its own bit
> there. If necessary EVM could export as a function it's status info.
I wouldn't try to guess in IMA when EVM resets its status. We would have
to duplicate the logic to check if an EVM key is loaded, if the passed xattr
is a POSIX ACL, ...
I think it is better to set a flag, maybe a new one, directly in EVM, to notify
the integrity subsystem that iint->evm_status is no longer valid.
If the EVM flag is set, IMA would reset the appraisal flags, as it uses
iint->evm_status for appraisal. We can consider to reset also the measure
flags when we have a template that includes file metadata.
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
> Mimi
>
> >
> > This patch also adds a call to evm_reset_status() in
> > evm_inode_post_setattr() so that EVM won't return the cached status
> the
> > next time appraisal is performed.
> >
> > Cc: stable@vger.kernel.org # 4.16.x
> > Fixes: ae1ba1676b88e ("EVM: Allow userland to permit modification of
> EVM-protected metadata")
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> > security/integrity/evm/evm_main.c | 14 ++++++++++----
> > 1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/security/integrity/evm/evm_main.c
> b/security/integrity/evm/evm_main.c
> > index 41cc6a4aaaab..d4d918183094 100644
> > --- a/security/integrity/evm/evm_main.c
> > +++ b/security/integrity/evm/evm_main.c
> > @@ -478,13 +478,17 @@ int evm_inode_removexattr(struct dentry
> *dentry, const char *xattr_name)
> > return evm_protect_xattr(dentry, xattr_name, NULL, 0);
> > }
> >
> > -static void evm_reset_status(struct inode *inode)
> > +static void evm_reset_status(struct inode *inode, int bit)
> > {
> > struct integrity_iint_cache *iint;
> >
> > iint = integrity_iint_find(inode);
> > - if (iint)
> > + if (iint) {
> > + if (evm_initialized & EVM_ALLOW_METADATA_WRITES)
> > + set_bit(bit, &iint->atomic_flags);
> > +
> > iint->evm_status = INTEGRITY_UNKNOWN;
> > + }
> > }
> >
> > /**:q
> > @@ -507,7 +511,7 @@ void evm_inode_post_setxattr(struct dentry
> *dentry, const char *xattr_name,
> > && !posix_xattr_acl(xattr_name)))
> > return;
> >
> > - evm_reset_status(dentry->d_inode);
> > + evm_reset_status(dentry->d_inode, IMA_CHANGE_XATTR);
> >
> > evm_update_evmxattr(dentry, xattr_name, xattr_value,
> xattr_value_len);
> > }
> > @@ -527,7 +531,7 @@ void evm_inode_post_removexattr(struct dentry
> *dentry, const char *xattr_name)
> > if (!evm_key_loaded() || !evm_protected_xattr(xattr_name))
> > return;
> >
> > - evm_reset_status(dentry->d_inode);
> > + evm_reset_status(dentry->d_inode, IMA_CHANGE_XATTR);
> >
> > evm_update_evmxattr(dentry, xattr_name, NULL, 0);
> > }
> > @@ -600,6 +604,8 @@ void evm_inode_post_setattr(struct dentry
> *dentry, int ia_valid)
> > if (!evm_key_loaded())
> > return;
> >
> > + evm_reset_status(dentry->d_inode, IMA_CHANGE_ATTR);
> > +
> > if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
> > evm_update_evmxattr(dentry, NULL, NULL, 0);
> > }
>
^ permalink raw reply
* RE: [PATCH 07/11] evm: Set IMA_CHANGE_XATTR/ATTR bit if EVM_ALLOW_METADATA_WRITES is set
From: Roberto Sassu @ 2020-09-01 11:41 UTC (permalink / raw)
To: Mimi Zohar, mjg59@google.com
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu,
stable@vger.kernel.org
In-Reply-To: <ae06c113ec91442e293f2466cae3dd1b81f241eb.camel@linux.ibm.com>
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Tuesday, September 1, 2020 1:05 PM
> On Tue, 2020-09-01 at 09:08 +0000, Roberto Sassu wrote:
> > > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > > Sent: Monday, August 24, 2020 2:18 PM
> > > On Thu, 2020-06-18 at 18:04 +0200, Roberto Sassu wrote:
> > > > When EVM_ALLOW_METADATA_WRITES is set, EVM allows any
> operation
> > > on
> > > > metadata. Its main purpose is to allow users to freely set metadata
> when
> > > > they are protected by a portable signature, until the HMAC key is
> loaded.
> > > >
> > > > However, IMA is not notified about metadata changes and, after the
> first
> > > > appraisal, always allows access to the files without checking metadata
> > > > again.
> > >
> > > ^after the first successful appraisal
> > > >
> > > > This patch checks in evm_reset_status() if EVM_ALLOW_METADATA
> > > WRITES is
> > > > enabled and if it is, sets the IMA_CHANGE_XATTR/ATTR bits
> depending on
> > > the
> > > > operation performed. At the next appraisal, metadata are revalidated.
> > >
> > > EVM modifying IMA bits crosses the boundary between EVM and IMA.
> > > There
> > > is already an IMA post_setattr hook. IMA could reset its own bit
> > > there. If necessary EVM could export as a function it's status info.
> >
> > I wouldn't try to guess in IMA when EVM resets its status. We would have
> > to duplicate the logic to check if an EVM key is loaded, if the passed xattr
> > is a POSIX ACL, ...
>
> Agreed, but IMA could call an EVM function.
>
> >
> > I think it is better to set a flag, maybe a new one, directly in EVM, to notify
> > the integrity subsystem that iint->evm_status is no longer valid.
> >
> > If the EVM flag is set, IMA would reset the appraisal flags, as it uses
> > iint->evm_status for appraisal. We can consider to reset also the measure
> > flags when we have a template that includes file metadata.
>
> When would IMA read the EVM flag? Who would reset the flag? At what
> point would it be reset? Just as EVM shouldn't be resetting the IMA
> flag, IMA shouldn't be resetting the EVM flag.
IMA would read the flag in process_measurement() and behave similarly
to when it processes IMA_CHANGE_ATTR. The flag would be reset by
evm_verify_hmac().
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
^ 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