* [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
* [PATCH v3 3/6] IMA: update process_buffer_measurement to measure buffer hash
From: Tushar Sugandhi @ 2020-08-28 1:57 UTC (permalink / raw)
To: zohar, 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-1-tusharsu@linux.microsoft.com>
process_buffer_measurement() currently only measures the input buffer.
When the buffer being measured is too large, it may result in bloated
IMA logs.
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>
---
security/integrity/ima/ima.h | 3 +-
security/integrity/ima/ima_appraise.c | 2 +-
security/integrity/ima/ima_asymmetric_keys.c | 2 +-
security/integrity/ima/ima_main.c | 29 ++++++++++++++++++--
security/integrity/ima/ima_queue_keys.c | 3 +-
5 files changed, 32 insertions(+), 7 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 83ed57147e68..ba332de8ed0b 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -267,7 +267,8 @@ void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
struct ima_template_desc *template_desc);
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);
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_appraise.c b/security/integrity/ima/ima_appraise.c
index 372d16382960..20adffe5bf58 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -336,7 +336,7 @@ int ima_check_blacklist(struct integrity_iint_cache *iint,
if ((rc == -EPERM) && (iint->flags & IMA_MEASURE))
process_buffer_measurement(NULL, digest, digestsize,
"blacklisted-hash", NONE,
- pcr, NULL);
+ pcr, NULL, false);
}
return rc;
diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
index 1c68c500c26f..a74095793936 100644
--- a/security/integrity/ima/ima_asymmetric_keys.c
+++ b/security/integrity/ima/ima_asymmetric_keys.c
@@ -60,5 +60,5 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
*/
process_buffer_measurement(NULL, payload, payload_len,
keyring->description, KEY_CHECK, 0,
- keyring->description);
+ keyring->description, false);
}
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 0979a62a9257..52cbbc1f7ea2 100644
--- a/security/integrity/ima/ima_main.c
+++ 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;
+ }
+
ret = ima_alloc_init_template(&event_data, &entry, template);
if (ret < 0) {
audit_cause = "alloc_entry";
@@ -842,7 +864,8 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
return;
process_buffer_measurement(file_inode(f.file), buf, size,
- "kexec-cmdline", KEXEC_CMDLINE, 0, NULL);
+ "kexec-cmdline", KEXEC_CMDLINE, 0, NULL,
+ false);
fdput(f);
}
diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c
index 69a8626a35c0..c2f2ad34f9b7 100644
--- a/security/integrity/ima/ima_queue_keys.c
+++ b/security/integrity/ima/ima_queue_keys.c
@@ -162,7 +162,8 @@ void ima_process_queued_keys(void)
entry->payload_len,
entry->keyring_name,
KEY_CHECK, 0,
- entry->keyring_name);
+ entry->keyring_name,
+ false);
list_del(&entry->list);
ima_free_key_entry(entry);
}
--
2.17.1
^ permalink raw reply related
* [PATCH v3 2/6] IMA: change process_buffer_measurement return type from void to int
From: Tushar Sugandhi @ 2020-08-28 1:57 UTC (permalink / raw)
To: zohar, 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-1-tusharsu@linux.microsoft.com>
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.
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;
}
/**
--
2.17.1
^ permalink raw reply related
* [PATCH v3 4/6] IMA: add policy to measure critical data from kernel components
From: Tushar Sugandhi @ 2020-08-28 1:57 UTC (permalink / raw)
To: zohar, 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-1-tusharsu@linux.microsoft.com>
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.
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.
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.
default policy:
# PROC_SUPER_MAGIC
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 8866e84d0062..c8a044705347 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -33,6 +33,7 @@
#define IMA_PCR 0x0100
#define IMA_FSNAME 0x0200
#define IMA_KEYRINGS 0x0400
+#define IMA_DATA_SOURCES 0x0800
#define UNKNOWN 0
#define MEASURE 0x0001 /* same as IMA_MEASURE */
@@ -84,6 +85,7 @@ struct ima_rule_entry {
} lsm[MAX_LSM_RULES];
char *fsname;
struct ima_rule_opt_list *keyrings; /* Measure keys added to these keyrings */
+ struct ima_rule_opt_list *data_sources; /* Measure data from these sources */
struct ima_template_desc *template;
};
@@ -911,7 +913,7 @@ enum {
Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
Opt_appraise_type, Opt_appraise_flag,
Opt_permit_directio, Opt_pcr, Opt_template, Opt_keyrings,
- Opt_err
+ Opt_data_sources, Opt_err
};
static const match_table_t policy_tokens = {
@@ -948,6 +950,7 @@ static const match_table_t policy_tokens = {
{Opt_pcr, "pcr=%s"},
{Opt_template, "template=%s"},
{Opt_keyrings, "keyrings=%s"},
+ {Opt_data_sources, "critical_kernel_data_sources=%s"},
{Opt_err, NULL}
};
@@ -1312,6 +1315,24 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
entry->flags |= IMA_KEYRINGS;
break;
+ case Opt_data_sources:
+ ima_log_string(ab, "critical_kernel_data_sources",
+ args[0].from);
+
+ if (entry->data_sources) {
+ result = -EINVAL;
+ break;
+ }
+
+ entry->data_sources = ima_alloc_rule_opt_list(args);
+ if (IS_ERR(entry->data_sources)) {
+ result = PTR_ERR(entry->data_sources);
+ entry->data_sources = NULL;
+ break;
+ }
+
+ entry->flags |= IMA_DATA_SOURCES;
+ break;
case Opt_fsuuid:
ima_log_string(ab, "fsuuid", args[0].from);
@@ -1692,6 +1713,12 @@ int ima_policy_show(struct seq_file *m, void *v)
seq_puts(m, " ");
}
+ if (entry->flags & IMA_DATA_SOURCES) {
+ seq_puts(m, "critical_kernel_data_sources=");
+ ima_show_rule_opt_list(m, entry->data_sources);
+ seq_puts(m, " ");
+ }
+
if (entry->flags & IMA_PCR) {
snprintf(tbuf, sizeof(tbuf), "%d", entry->pcr);
seq_printf(m, pt(Opt_pcr), tbuf);
--
2.17.1
^ permalink raw reply related
* [PATCH v3 1/6] IMA: generalize keyring specific measurement constructs
From: Tushar Sugandhi @ 2020-08-28 1:56 UTC (permalink / raw)
To: zohar, 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-1-tusharsu@linux.microsoft.com>
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.
Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
security/integrity/ima/ima.h | 6 ++---
security/integrity/ima/ima_api.c | 6 ++---
security/integrity/ima/ima_main.c | 6 ++---
security/integrity/ima/ima_policy.c | 42 ++++++++++++++++-------------
4 files changed, 33 insertions(+), 27 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 38043074ce5e..8875085db689 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -255,7 +255,7 @@ static inline void ima_process_queued_keys(void) {}
int ima_get_action(struct inode *inode, const struct cred *cred, u32 secid,
int mask, enum ima_hooks func, int *pcr,
struct ima_template_desc **template_desc,
- const char *keyring);
+ const char *func_data);
int ima_must_measure(struct inode *inode, int mask, enum ima_hooks func);
int ima_collect_measurement(struct integrity_iint_cache *iint,
struct file *file, void *buf, loff_t size,
@@ -267,7 +267,7 @@ void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
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 *keyring);
+ 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,
@@ -283,7 +283,7 @@ const char *ima_d_path(const struct path *path, char **pathbuf, char *filename);
int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid,
enum ima_hooks func, int mask, int flags, int *pcr,
struct ima_template_desc **template_desc,
- const char *keyring);
+ const char *func_data);
void ima_init_policy(void);
void ima_update_policy(void);
void ima_update_policy_flag(void);
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 4f39fb93f278..af218babd198 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -170,7 +170,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
* @func: caller identifier
* @pcr: pointer filled in if matched measure policy sets pcr=
* @template_desc: pointer filled in if matched measure policy sets template=
- * @keyring: keyring name used to determine the action
+ * @func_data: private data specific to @func, can be NULL.
*
* The policy is defined in terms of keypairs:
* subj=, obj=, type=, func=, mask=, fsmagic=
@@ -186,14 +186,14 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
int ima_get_action(struct inode *inode, const struct cred *cred, u32 secid,
int mask, enum ima_hooks func, int *pcr,
struct ima_template_desc **template_desc,
- const char *keyring)
+ const char *func_data)
{
int flags = IMA_MEASURE | IMA_AUDIT | IMA_APPRAISE | IMA_HASH;
flags &= ima_policy_flag;
return ima_match_policy(inode, cred, secid, func, mask, flags, pcr,
- template_desc, keyring);
+ template_desc, func_data);
}
/*
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 8a91711ca79b..c870fd6d2f83 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -732,13 +732,13 @@ int ima_load_data(enum kernel_load_data_id id)
* @eventname: event name to be used for the buffer entry.
* @func: IMA hook
* @pcr: pcr to extend the measurement
- * @keyring: keyring name to determine the action to be performed
+ * @func_data: private data specific to @func, can be NULL.
*
* 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 *keyring)
+ int pcr, const char *func_data)
{
int ret = 0;
const char *audit_cause = "ENOMEM";
@@ -770,7 +770,7 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size,
if (func) {
security_task_getsecid(current, &secid);
action = ima_get_action(inode, current_cred(), secid, 0, func,
- &pcr, &template, keyring);
+ &pcr, &template, func_data);
if (!(action & IMA_MEASURE))
return;
}
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,
+ const char *func_data,
+ bool allow_empty_opt_list,
+ const struct cred *cred)
{
bool matched = false;
size_t i;
@@ -467,14 +473,14 @@ static bool ima_match_keyring(struct ima_rule_entry *rule,
if ((rule->flags & IMA_UID) && !rule->uid_op(cred->uid, rule->uid))
return false;
- if (!rule->keyrings)
- return true;
+ if (!opt_list)
+ return allow_empty_opt_list;
- if (!keyring)
+ if (!func_data)
return false;
- for (i = 0; i < rule->keyrings->count; i++) {
- if (!strcmp(rule->keyrings->items[i], keyring)) {
+ for (i = 0; i < opt_list->count; i++) {
+ if (!strcmp(opt_list->items[i], func_data)) {
matched = true;
break;
}
@@ -491,20 +497,21 @@ static bool ima_match_keyring(struct ima_rule_entry *rule,
* @secid: the secid of the task to be validated
* @func: LIM hook identifier
* @mask: requested action (MAY_READ | MAY_WRITE | MAY_APPEND | MAY_EXEC)
- * @keyring: keyring name to check in policy for KEY_CHECK func
+ * @func_data: private data specific to @func, can be NULL.
*
* Returns true on rule match, false on failure.
*/
static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
const struct cred *cred, u32 secid,
enum ima_hooks func, int mask,
- const char *keyring)
+ const char *func_data)
{
int i;
if (func == KEY_CHECK) {
return (rule->flags & IMA_FUNC) && (rule->func == func) &&
- ima_match_keyring(rule, keyring, cred);
+ ima_match_rule_data(rule, rule->keyrings, func_data,
+ true, cred);
}
if ((rule->flags & IMA_FUNC) &&
(rule->func != func && func != POST_SETATTR))
@@ -608,8 +615,7 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func)
* @mask: requested action (MAY_READ | MAY_WRITE | MAY_APPEND | MAY_EXEC)
* @pcr: set the pcr to extend
* @template_desc: the template that should be used for this rule
- * @keyring: the keyring name, if given, to be used to check in the policy.
- * keyring can be NULL if func is anything other than KEY_CHECK.
+ * @func_data: private data specific to @func, can be NULL.
*
* Measure decision based on func/mask/fsmagic and LSM(subj/obj/type)
* conditions.
@@ -621,7 +627,7 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func)
int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid,
enum ima_hooks func, int mask, int flags, int *pcr,
struct ima_template_desc **template_desc,
- const char *keyring)
+ const char *func_data)
{
struct ima_rule_entry *entry;
int action = 0, actmask = flags | (flags << 1);
@@ -636,7 +642,7 @@ int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid,
continue;
if (!ima_match_rules(entry, inode, cred, secid, func, mask,
- keyring))
+ func_data))
continue;
action |= entry->flags & IMA_ACTION_FLAGS;
--
2.17.1
^ permalink raw reply related
* [PATCH v3 5/6] IMA: add hook to measure critical data from kernel components
From: Tushar Sugandhi @ 2020-08-28 1:57 UTC (permalink / raw)
To: zohar, 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-1-tusharsu@linux.microsoft.com>
Currently, IMA does not provide a generic function for kernel components
to measure their data. A generic function provided by IMA would
enable various parts of the kernel with easier and faster on-boarding to
use IMA infrastructure, would avoid code duplication, and consistent
usage of IMA policy "critical_kernel_data_sources" across the kernel.
Add a new IMA func CRITICAL_DATA and a corresponding IMA hook
ima_measure_critical_data() to support measuring various critical kernel
components. Limit the measurement to the components that are specified
in the IMA policy - CRITICAL_DATA+critical_kernel_data_sources.
Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
Documentation/ABI/testing/ima_policy | 8 ++++++-
include/linux/ima.h | 11 +++++++++
security/integrity/ima/ima.h | 1 +
security/integrity/ima/ima_api.c | 2 +-
security/integrity/ima/ima_main.c | 24 ++++++++++++++++++++
security/integrity/ima/ima_policy.c | 34 ++++++++++++++++++++++++----
6 files changed, 73 insertions(+), 7 deletions(-)
diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index 7ccdc1964e29..36d9cee9704d 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -29,7 +29,7 @@ Description:
base: func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
[FIRMWARE_CHECK]
[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
- [KEXEC_CMDLINE] [KEY_CHECK]
+ [KEXEC_CMDLINE] [KEY_CHECK] [CRITICAL_DATA]
mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
[[^]MAY_EXEC]
fsmagic:= hex value
@@ -51,6 +51,8 @@ Description:
critical_kernel_data_sources:= list of kernel
components (eg, selinux|apparmor|dm-crypt) that
contain data critical to the security of the kernel.
+ Only valid when action is "measure" and func is
+ CRITICAL_DATA.
default policy:
# PROC_SUPER_MAGIC
@@ -128,3 +130,7 @@ Description:
keys added to .builtin_trusted_keys or .ima keyring:
measure func=KEY_CHECK keyrings=.builtin_trusted_keys|.ima
+
+ Example of measure rule using CRITICAL_DATA to measure critical data
+
+ measure func=CRITICAL_DATA critical_kernel_data_sources=selinux|apparmor|dm-crypt
diff --git a/include/linux/ima.h b/include/linux/ima.h
index d15100de6cdd..136fc02580db 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -26,6 +26,10 @@ extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
extern void ima_post_path_mknod(struct dentry *dentry);
extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
+extern int ima_measure_critical_data(const char *event_name,
+ const char *event_data_source,
+ const void *buf, int buf_len,
+ bool measure_buf_hash);
#ifdef CONFIG_IMA_KEXEC
extern void ima_add_kexec_buffer(struct kimage *image);
@@ -104,6 +108,13 @@ static inline int ima_file_hash(struct file *file, char *buf, size_t buf_size)
}
static inline void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) {}
+static inline int ima_measure_critical_data(const char *event_name,
+ const char *event_data_source,
+ const void *buf, int buf_len,
+ bool measure_buf_hash)
+{
+ return -EOPNOTSUPP;
+}
#endif /* CONFIG_IMA */
#ifndef CONFIG_IMA_KEXEC
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index ba332de8ed0b..00b84052c8f1 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -200,6 +200,7 @@ static inline unsigned int ima_hash_key(u8 *digest)
hook(POLICY_CHECK, policy) \
hook(KEXEC_CMDLINE, kexec_cmdline) \
hook(KEY_CHECK, key) \
+ hook(CRITICAL_DATA, critical_data) \
hook(MAX_CHECK, none)
#define __ima_hook_enumify(ENUM, str) ENUM,
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index af218babd198..9917e1730cb6 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -176,7 +176,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
* subj=, obj=, type=, func=, mask=, fsmagic=
* subj,obj, and type: are LSM specific.
* func: FILE_CHECK | BPRM_CHECK | CREDS_CHECK | MMAP_CHECK | MODULE_CHECK
- * | KEXEC_CMDLINE | KEY_CHECK
+ * | KEXEC_CMDLINE | KEY_CHECK | CRITICAL_DATA
* mask: contains the permission mask
* fsmagic: hex value
*
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);
+}
+
static int __init init_ima(void)
{
int error;
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index c8a044705347..0c5202c1f26e 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -510,14 +510,23 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
{
int i;
- if (func == KEY_CHECK) {
- return (rule->flags & IMA_FUNC) && (rule->func == func) &&
- ima_match_rule_data(rule, rule->keyrings, func_data,
- true, cred);
- }
if ((rule->flags & IMA_FUNC) &&
(rule->func != func && func != POST_SETATTR))
return false;
+
+ switch (func) {
+ case KEY_CHECK:
+ return ((rule->func == func) &&
+ ima_match_rule_data(rule, rule->keyrings,
+ func_data, true, cred));
+ case CRITICAL_DATA:
+ return ((rule->func == func) &&
+ ima_match_rule_data(rule, rule->data_sources,
+ func_data, false, cred));
+ default:
+ break;
+ }
+
if ((rule->flags & IMA_MASK) &&
(rule->mask != mask && func != POST_SETATTR))
return false;
@@ -1113,6 +1122,19 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
if (ima_rule_contains_lsm_cond(entry))
return false;
+ break;
+ case CRITICAL_DATA:
+ if (entry->action & ~(MEASURE | DONT_MEASURE))
+ return false;
+
+ if (!(entry->flags & IMA_DATA_SOURCES) ||
+ (entry->flags & ~(IMA_FUNC | IMA_UID | IMA_PCR |
+ IMA_DATA_SOURCES)))
+ return false;
+
+ if (ima_rule_contains_lsm_cond(entry))
+ return false;
+
break;
default:
return false;
@@ -1245,6 +1267,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
else if (IS_ENABLED(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS) &&
strcmp(args[0].from, "KEY_CHECK") == 0)
entry->func = KEY_CHECK;
+ else if (strcmp(args[0].from, "CRITICAL_DATA") == 0)
+ entry->func = CRITICAL_DATA;
else
result = -EINVAL;
if (!result)
--
2.17.1
^ permalink raw reply related
* [PATCH v3 6/6] IMA: validate supported kernel data sources before measurement
From: Tushar Sugandhi @ 2020-08-28 1:57 UTC (permalink / raw)
To: zohar, 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-1-tusharsu@linux.microsoft.com>
Currently, IMA does not restrict random data sources from measuring their
data using ima_measure_critical_data(). Any kernel data source can call
the function, and it's data will get measured as long as the input
event_data_source is part of the IMA policy -
CRITICAL_DATA+critical_kernel_data_sources. This may result in IMA log
getting bloated by random data sources. Supporting random data sources
at run-time may also impact the reliability of the system.
To ensure that only data from supported sources are measured, the kernel
component needs to be added to a compile-time list of supported sources
(an "allowed list of components") in ima.h. IMA then validates the input
parameter - event_data_source passed to ima_measure_critical_data()
against this allowed list at run-time.
Provide an infrastructure for kernel data sources to be added to
the supported data sources list at compile-time. Update
ima_measure_critical_data() to validate, at run-time, that the data
source is supported before measuring the data.
Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
security/integrity/ima/ima.h | 29 +++++++++++++++++++++++++++++
security/integrity/ima/ima_main.c | 3 +++
2 files changed, 32 insertions(+)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 00b84052c8f1..ecb0a1e7378f 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -228,6 +228,35 @@ extern const char *const func_tokens[];
struct modsig;
+#define __ima_supported_kernel_data_sources(source) \
+ source(MIN_SOURCE, min_source) \
+ source(MAX_SOURCE, max_source)
+
+#define __ima_enum_stringify(ENUM, str) (#str),
+
+enum ima_supported_kernel_data_sources {
+ __ima_supported_kernel_data_sources(__ima_hook_enumify)
+};
+
+static const char * const ima_supported_kernel_data_sources_str[] = {
+ __ima_supported_kernel_data_sources(__ima_enum_stringify)
+};
+
+static inline bool ima_kernel_data_source_is_supported(const char *source)
+{
+ int i;
+
+ if (!source)
+ return false;
+
+ for (i = MIN_SOURCE + 1; i < MAX_SOURCE; i++) {
+ if (!strcmp(ima_supported_kernel_data_sources_str[i], source))
+ return true;
+ }
+
+ return false;
+}
+
#ifdef CONFIG_IMA_QUEUE_EARLY_BOOT_KEYS
/*
* To track keys that need to be measured.
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index a889bf40cb7e..41be4d1d839e 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -888,6 +888,9 @@ int ima_measure_critical_data(const char *event_name,
if (!event_name || !event_data_source || !buf || !buf_len)
return -EINVAL;
+ if (!ima_kernel_data_source_is_supported(event_data_source))
+ return -EPERM;
+
return process_buffer_measurement(NULL, buf, buf_len, event_name,
CRITICAL_DATA, 0, event_data_source,
measure_buf_hash);
--
2.17.1
^ permalink raw reply related
* [PATCH v3 0/6] IMA: Infrastructure for measurement of critical kernel data
From: Tushar Sugandhi @ 2020-08-28 1:56 UTC (permalink / raw)
To: zohar, stephen.smalley.work, casey, agk, snitzer, gmazyland
Cc: tyhicks, sashal, jmorris, nramas, linux-integrity, selinux,
linux-security-module, linux-kernel, dm-devel
There are several kernel components that contain critical data which if
accidentally or maliciously altered, can compromise the security of the
kernel. Example of such components would include LSMs like SELinux, or
AppArmor; or device-mapper targets like dm-crypt, dm-verity etc.
Many of these components do not use the capabilities provided by kernel
integrity subsystem (IMA), and thus they don't use the benefits of
extended TPM PCR quotes and ultimately the benefits of remote attestation.
This series bridges this gap, so that potential kernel components that
contain data critical to the security of the kernel could take advantage
of IMA's measuring and quoting abilities - thus ultimately enabling
remote attestation for their specific data.
System administrators may want to pick and choose which kernel
components they would want to enable for measurements, quoting, and
remote attestation. To enable that, a new IMA policy is introduced.
And lastly, the functionality is exposed through a function
ima_measure_critical_data(). The functionality is generic enough to
measure the data of any kernel component at run-time. To ensure that only
data from supported sources are measured, the kernel component needs to
be added to a compile-time list of supported sources (an "allowed list
of components"). IMA validates the source passed to
ima_measure_critical_data() against this allowed list at run-time.
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 series also has a dependency on the following patch series:
https://patchwork.kernel.org/patch/11709527/
Change Log v3:
Incorporated feedback from Mimi on v2.
- Renamed the policy "data_sources" to
"critical_kernel_data_sources".
- Added "critical_kernel_data_sources" description in
Documentation/ima-policy.
- Split CRITICAL_DATA + critical_kernel_data_sources into two separate
patches.
- Merged hook ima_measure_critical_data() + CRITICAL_DATA into a single
patch.
- Added functionality to validate data sources before measurement.
Change Log v2:
- Reverted the unnecessary indentations in existing #define.
- Updated the description to replace the word 'enlightened' with
'supported'.
- Reverted the unnecessary rename of attribute size to buf_len.
- Introduced a boolean parameter measure_buf_hash as per community
feedback to support measuring hash of the buffer, instead of the
buffer itself.
Tushar Sugandhi (6):
IMA: generalize keyring specific measurement constructs
IMA: change process_buffer_measurement return type from void to int
IMA: update process_buffer_measurement to measure buffer hash
IMA: add policy to measure critical data from kernel components
IMA: add hook to measure critical data from kernel components
IMA: validate supported kernel data sources before measurement
Documentation/ABI/testing/ima_policy | 11 +-
include/linux/ima.h | 11 ++
security/integrity/ima/ima.h | 41 +++++++-
security/integrity/ima/ima_api.c | 8 +-
security/integrity/ima/ima_appraise.c | 2 +-
security/integrity/ima/ima_asymmetric_keys.c | 2 +-
security/integrity/ima/ima_main.c | 72 +++++++++++--
security/integrity/ima/ima_policy.c | 101 +++++++++++++++----
security/integrity/ima/ima_queue_keys.c | 3 +-
9 files changed, 205 insertions(+), 46 deletions(-)
--
2.17.1
^ permalink raw reply
* Re: [PATCH RFC] netlabel: remove unused param from audit_log_format()
From: Paul Moore @ 2020-08-27 20:10 UTC (permalink / raw)
To: Alex Dewar
Cc: David S. Miller, Jakub Kicinski, netdev, linux-security-module,
linux-kernel
In-Reply-To: <20200827172006.gudui4alfbbf2a2p@medion>
On Thu, Aug 27, 2020 at 1:20 PM Alex Dewar <alex.dewar90@gmail.com> wrote:
> On Thu, Aug 27, 2020 at 06:06:34PM +0100, Alex Dewar wrote:
> > On Thu, Aug 27, 2020 at 01:00:58PM -0400, Paul Moore wrote:
> > > On Thu, Aug 27, 2020 at 12:39 PM 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=0". So don't print res anymore.
> > > >
> > > > Addresses-Coverity: ("Dead code")
> > > > Fixes: d3b990b7f327 ("netlabel: fix problems with mapping removal")
> > > > Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
> > > > ---
> > > >
> > > > I wasn't sure whether it was intended that something other than ret_val
> > > > be printed in the log, so that's why I'm sending this as an RFC.
> > >
> > > It's intentional for a couple of reasons:
> > >
> > > * The people who care about audit logs like to see success/fail (e.g.
> > > "res=X") for audit events/records, so printing this out gives them the
> > > warm fuzzies.
> > >
> > > * For a lot of awful reasons that I won't bore you with, we really
> > > don't want to add/remove fields in the middle of an audit record so we
> > > pretty much need to keep the "res=0" there even if it seems a bit
> > > redundant.
> > >
> > > So NACK from me, but thanks for paying attention just the same :)
> >
> > Would you rather just have an explicit "res=0" in there, without looking
> > at ret_val? The thing is that ret_val will *always* be zero at this point in
> > the code, because, if not, the function will already have returned.
> > That's why Coverity flagged it up as a redundant check.
>
> Sorry, I meant "res=1". The code will always print res=1, because
> ret_val is always 0.
That's okay, I can't tell you how many times I've made that mistake
with "res=" :)
Anyway, yes at that point ret_val should always be 0, and "res=X"
should always be "res=1", so if you wanted to change it to a fixed
value so you could get rid of the ternary statement that would be
okay.
> > > > 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 f73a8382c275..526762b2f3a9 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",
> > > > + entry->domain ? entry->domain : "(default)");
> > > > audit_log_end(audit_buf);
> > > > }
> > > >
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH RFC] netlabel: remove unused param from audit_log_format()
From: Alex Dewar @ 2020-08-27 17:20 UTC (permalink / raw)
To: Alex Dewar
Cc: Paul Moore, David S. Miller, Jakub Kicinski, netdev,
linux-security-module, linux-kernel
In-Reply-To: <20200827170634.wogybzcxux7sgefb@medion>
On Thu, Aug 27, 2020 at 06:06:34PM +0100, Alex Dewar wrote:
> On Thu, Aug 27, 2020 at 01:00:58PM -0400, Paul Moore wrote:
> > On Thu, Aug 27, 2020 at 12:39 PM 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=0". So don't print res anymore.
> > >
> > > Addresses-Coverity: ("Dead code")
> > > Fixes: d3b990b7f327 ("netlabel: fix problems with mapping removal")
> > > Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
> > > ---
> > >
> > > I wasn't sure whether it was intended that something other than ret_val
> > > be printed in the log, so that's why I'm sending this as an RFC.
> >
> > It's intentional for a couple of reasons:
> >
> > * The people who care about audit logs like to see success/fail (e.g.
> > "res=X") for audit events/records, so printing this out gives them the
> > warm fuzzies.
> >
> > * For a lot of awful reasons that I won't bore you with, we really
> > don't want to add/remove fields in the middle of an audit record so we
> > pretty much need to keep the "res=0" there even if it seems a bit
> > redundant.
> >
> > So NACK from me, but thanks for paying attention just the same :)
>
> Would you rather just have an explicit "res=0" in there, without looking
> at ret_val? The thing is that ret_val will *always* be zero at this point in
> the code, because, if not, the function will already have returned.
> That's why Coverity flagged it up as a redundant check.
Sorry, I meant "res=1". The code will always print res=1, because
ret_val is always 0.
>
> >
> > > 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 f73a8382c275..526762b2f3a9 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",
> > > + entry->domain ? entry->domain : "(default)");
> > > audit_log_end(audit_buf);
> > > }
> > >
> >
> > --
> > paul moore
> > www.paul-moore.com
^ permalink raw reply
* Re: [PATCH RFC] netlabel: remove unused param from audit_log_format()
From: Alex Dewar @ 2020-08-27 17:06 UTC (permalink / raw)
To: Paul Moore
Cc: Alex Dewar, David S. Miller, Jakub Kicinski, netdev,
linux-security-module, linux-kernel
In-Reply-To: <CAHC9VhRgi54TXae1Wi+SSzkuy9BL7HH=pZCHL1p215M9ZXKEOA@mail.gmail.com>
On Thu, Aug 27, 2020 at 01:00:58PM -0400, Paul Moore wrote:
> On Thu, Aug 27, 2020 at 12:39 PM 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=0". So don't print res anymore.
> >
> > Addresses-Coverity: ("Dead code")
> > Fixes: d3b990b7f327 ("netlabel: fix problems with mapping removal")
> > Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
> > ---
> >
> > I wasn't sure whether it was intended that something other than ret_val
> > be printed in the log, so that's why I'm sending this as an RFC.
>
> It's intentional for a couple of reasons:
>
> * The people who care about audit logs like to see success/fail (e.g.
> "res=X") for audit events/records, so printing this out gives them the
> warm fuzzies.
>
> * For a lot of awful reasons that I won't bore you with, we really
> don't want to add/remove fields in the middle of an audit record so we
> pretty much need to keep the "res=0" there even if it seems a bit
> redundant.
>
> So NACK from me, but thanks for paying attention just the same :)
Would you rather just have an explicit "res=0" in there, without looking
at ret_val? The thing is that ret_val will *always* be zero at this point in
the code, because, if not, the function will already have returned.
That's why Coverity flagged it up as a redundant check.
>
> > 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 f73a8382c275..526762b2f3a9 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",
> > + entry->domain ? entry->domain : "(default)");
> > audit_log_end(audit_buf);
> > }
> >
>
> --
> paul moore
> www.paul-moore.com
^ permalink raw reply
* Re: [PATCH RFC] netlabel: remove unused param from audit_log_format()
From: Paul Moore @ 2020-08-27 17:00 UTC (permalink / raw)
To: Alex Dewar
Cc: David S. Miller, Jakub Kicinski, netdev, linux-security-module,
linux-kernel
In-Reply-To: <20200827163712.106303-1-alex.dewar90@gmail.com>
On Thu, Aug 27, 2020 at 12:39 PM 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=0". So don't print res anymore.
>
> Addresses-Coverity: ("Dead code")
> Fixes: d3b990b7f327 ("netlabel: fix problems with mapping removal")
> Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
> ---
>
> I wasn't sure whether it was intended that something other than ret_val
> be printed in the log, so that's why I'm sending this as an RFC.
It's intentional for a couple of reasons:
* The people who care about audit logs like to see success/fail (e.g.
"res=X") for audit events/records, so printing this out gives them the
warm fuzzies.
* For a lot of awful reasons that I won't bore you with, we really
don't want to add/remove fields in the middle of an audit record so we
pretty much need to keep the "res=0" there even if it seems a bit
redundant.
So NACK from me, but thanks for paying attention just the same :)
> 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 f73a8382c275..526762b2f3a9 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",
> + entry->domain ? entry->domain : "(default)");
> audit_log_end(audit_buf);
> }
>
--
paul moore
www.paul-moore.com
^ permalink raw reply
* [PATCH RFC] netlabel: remove unused param from audit_log_format()
From: Alex Dewar @ 2020-08-27 16:37 UTC (permalink / raw)
Cc: Alex Dewar, Paul Moore, David S. Miller, Jakub Kicinski, netdev,
linux-security-module, linux-kernel
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=0". So don't print res anymore.
Addresses-Coverity: ("Dead code")
Fixes: d3b990b7f327 ("netlabel: fix problems with mapping removal")
Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
---
I wasn't sure whether it was intended that something other than ret_val
be printed in the log, so that's why I'm sending this as an RFC.
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 f73a8382c275..526762b2f3a9 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",
+ entry->domain ? entry->domain : "(default)");
audit_log_end(audit_buf);
}
--
2.28.0
^ permalink raw reply related
* Re: [PATCH 02/18] fsinfo: Add fsinfo() syscall to query filesystem information [ver #21]
From: Michael Kerrisk (man-pages) @ 2020-08-27 11:27 UTC (permalink / raw)
To: David Howells
Cc: Alexander Viro, Linux API, Linus Torvalds, Ian Kent,
Miklos Szeredi, Christian Brauner, Jann Horn, Darrick J. Wong,
Karel Zak, Jeff Layton, linux-fsdevel@vger.kernel.org,
linux-security-module, lkml, linux-man
In-Reply-To: <159646180259.1784947.223853053048725752.stgit@warthog.procyon.org.uk>
Hello David,
On Mon, 3 Aug 2020 at 15:37, David Howells <dhowells@redhat.com> wrote:
>
> Add a system call to allow filesystem information to be queried. A request
> value can be given to indicate the desired attribute. Support is provided
> for enumerating multi-value attributes.
Do we have an up to date manual page for this system call?
Could you please (re)post to the same CC as this mail, plus linux-man@?
Thanks,
Michael
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
^ permalink raw reply
* Re: [PATCH] security/keys: use kvfree_sensitive()
From: Jarkko Sakkinen @ 2020-08-27 13:26 UTC (permalink / raw)
To: Denis Efremov
Cc: David Howells, keyrings, linux-security-module, linux-kernel
In-Reply-To: <20200827072923.14718-1-efremov@linux.com>
On Thu, Aug 27, 2020 at 10:29:23AM +0300, Denis Efremov wrote:
> Use kvfree_sensitive() instead of open-coding it.
>
> Signed-off-by: Denis Efremov <efremov@linux.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
/Jarkko
^ permalink raw reply
* [PATCH] security/keys: use kvfree_sensitive()
From: Denis Efremov @ 2020-08-27 7:29 UTC (permalink / raw)
To: David Howells
Cc: Denis Efremov, Jarkko Sakkinen, keyrings, linux-security-module,
linux-kernel
Use kvfree_sensitive() instead of open-coding it.
Signed-off-by: Denis Efremov <efremov@linux.com>
---
security/keys/big_key.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/security/keys/big_key.c b/security/keys/big_key.c
index 691347dea3c1..d17e5f09eeb8 100644
--- a/security/keys/big_key.c
+++ b/security/keys/big_key.c
@@ -121,8 +121,7 @@ int big_key_preparse(struct key_preparsed_payload *prep)
*path = file->f_path;
path_get(path);
fput(file);
- memzero_explicit(buf, enclen);
- kvfree(buf);
+ kvfree_sensitive(buf, enclen);
} else {
/* Just store the data in a buffer */
void *data = kmalloc(datalen, GFP_KERNEL);
@@ -140,8 +139,7 @@ int big_key_preparse(struct key_preparsed_payload *prep)
err_enckey:
kfree_sensitive(enckey);
error:
- memzero_explicit(buf, enclen);
- kvfree(buf);
+ kvfree_sensitive(buf, enclen);
return ret;
}
@@ -273,8 +271,7 @@ long big_key_read(const struct key *key, char *buffer, size_t buflen)
err_fput:
fput(file);
error:
- memzero_explicit(buf, enclen);
- kvfree(buf);
+ kvfree_sensitive(buf, enclen);
} else {
ret = datalen;
memcpy(buffer, key->payload.data[big_key_data], datalen);
--
2.26.2
^ permalink raw reply related
* [PATCH v8 1/3] Add a new LSM-supporting anonymous inode interface
From: Lokesh Gidra @ 2020-08-27 6:35 UTC (permalink / raw)
To: Alexander Viro, James Morris, Stephen Smalley, Casey Schaufler,
Eric Biggers
Cc: Serge E. Hallyn, Paul Moore, Eric Paris, Lokesh Gidra,
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-fsdevel, linux-kernel, linux-security-module, selinux,
kaleshsingh, calin, surenb, nnk, jeffv, kernel-team,
Daniel Colascione
In-Reply-To: <20200827063522.2563293-1-lokeshgidra@google.com>
From: Daniel Colascione <dancol@google.com>
This change adds a new function, anon_inode_getfd_secure, that creates
anonymous-node file with individual non-S_PRIVATE inode to which security
modules can apply policy. Existing callers continue using the original
singleton-inode kind of anonymous-inode file. We can transition anonymous
inode users to the new kind of anonymous inode in individual patches for
the sake of bisection and review.
The new function accepts an optional context_inode parameter that
callers can use to provide additional contextual information to
security modules for granting/denying permission to create an anon inode
of the same type.
For example, in case of userfaultfd, the created inode is a
'logical child' of the context_inode (userfaultfd inode of the
parent process) in the sense that it provides the security context
required during creation of the child process' userfaultfd inode.
Signed-off-by: Daniel Colascione <dancol@google.com>
[Fix comment documenting return values of inode_init_security_anon()]
[Add context_inode description in comments to anon_inode_getfd_secure()]
[Remove definition of anon_inode_getfile_secure() as there are no callers]
[Make _anon_inode_getfile() static]
[Use correct error cast in _anon_inode_getfile()]
[Fix error handling in _anon_inode_getfile()]
Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
---
fs/anon_inodes.c | 147 +++++++++++++++++++++++++---------
include/linux/anon_inodes.h | 8 ++
include/linux/lsm_hook_defs.h | 2 +
include/linux/lsm_hooks.h | 9 +++
include/linux/security.h | 10 +++
security/security.c | 8 ++
6 files changed, 144 insertions(+), 40 deletions(-)
diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
index 89714308c25b..c3f16deda211 100644
--- a/fs/anon_inodes.c
+++ b/fs/anon_inodes.c
@@ -55,61 +55,79 @@ static struct file_system_type anon_inode_fs_type = {
.kill_sb = kill_anon_super,
};
-/**
- * anon_inode_getfile - creates a new file instance by hooking it up to an
- * anonymous inode, and a dentry that describe the "class"
- * of the file
- *
- * @name: [in] name of the "class" of the new file
- * @fops: [in] file operations for the new file
- * @priv: [in] private data for the new file (will be file's private_data)
- * @flags: [in] flags
- *
- * Creates a new file by hooking it on a single inode. This is useful for files
- * that do not need to have a full-fledged inode in order to operate correctly.
- * All the files created with anon_inode_getfile() will share a single inode,
- * hence saving memory and avoiding code duplication for the file/inode/dentry
- * setup. Returns the newly created file* or an error pointer.
- */
-struct file *anon_inode_getfile(const char *name,
- const struct file_operations *fops,
- void *priv, int flags)
+static struct inode *anon_inode_make_secure_inode(
+ const char *name,
+ const struct inode *context_inode)
{
- struct file *file;
+ struct inode *inode;
+ const struct qstr qname = QSTR_INIT(name, strlen(name));
+ int error;
+
+ inode = alloc_anon_inode(anon_inode_mnt->mnt_sb);
+ if (IS_ERR(inode))
+ return inode;
+ inode->i_flags &= ~S_PRIVATE;
+ error = security_inode_init_security_anon(inode, &qname, context_inode);
+ if (error) {
+ iput(inode);
+ return ERR_PTR(error);
+ }
+ return inode;
+}
- if (IS_ERR(anon_inode_inode))
- return ERR_PTR(-ENODEV);
+static struct file *_anon_inode_getfile(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode,
+ bool secure)
+{
+ struct inode *inode;
+ struct file *file;
if (fops->owner && !try_module_get(fops->owner))
return ERR_PTR(-ENOENT);
- /*
- * We know the anon_inode inode count is always greater than zero,
- * so ihold() is safe.
- */
- ihold(anon_inode_inode);
- file = alloc_file_pseudo(anon_inode_inode, anon_inode_mnt, name,
+ if (secure) {
+ inode = anon_inode_make_secure_inode(name, context_inode);
+ if (IS_ERR(inode)) {
+ file = ERR_CAST(inode);
+ goto err;
+ }
+ } else {
+ inode = anon_inode_inode;
+ if (IS_ERR(inode)) {
+ file = ERR_PTR(-ENODEV);
+ goto err;
+ }
+ /*
+ * We know the anon_inode inode count is always
+ * greater than zero, so ihold() is safe.
+ */
+ ihold(inode);
+ }
+
+ file = alloc_file_pseudo(inode, anon_inode_mnt, name,
flags & (O_ACCMODE | O_NONBLOCK), fops);
if (IS_ERR(file))
- goto err;
+ goto err_iput;
- file->f_mapping = anon_inode_inode->i_mapping;
+ file->f_mapping = inode->i_mapping;
file->private_data = priv;
return file;
+err_iput:
+ iput(inode);
err:
- iput(anon_inode_inode);
module_put(fops->owner);
return file;
}
-EXPORT_SYMBOL_GPL(anon_inode_getfile);
/**
- * anon_inode_getfd - creates a new file instance by hooking it up to an
- * anonymous inode, and a dentry that describe the "class"
- * of the file
+ * anon_inode_getfile - creates a new file instance by hooking it up to an
+ * anonymous inode, and a dentry that describe the "class"
+ * of the file
*
* @name: [in] name of the "class" of the new file
* @fops: [in] file operations for the new file
@@ -118,12 +136,23 @@ EXPORT_SYMBOL_GPL(anon_inode_getfile);
*
* Creates a new file by hooking it on a single inode. This is useful for files
* that do not need to have a full-fledged inode in order to operate correctly.
- * All the files created with anon_inode_getfd() will share a single inode,
+ * All the files created with anon_inode_getfile() will share a single inode,
* hence saving memory and avoiding code duplication for the file/inode/dentry
- * setup. Returns new descriptor or an error code.
+ * setup. Returns the newly created file* or an error pointer.
*/
-int anon_inode_getfd(const char *name, const struct file_operations *fops,
- void *priv, int flags)
+struct file *anon_inode_getfile(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags)
+{
+ return _anon_inode_getfile(name, fops, priv, flags, NULL, false);
+}
+EXPORT_SYMBOL_GPL(anon_inode_getfile);
+
+static int _anon_inode_getfd(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode,
+ bool secure)
{
int error, fd;
struct file *file;
@@ -133,7 +162,8 @@ int anon_inode_getfd(const char *name, const struct file_operations *fops,
return error;
fd = error;
- file = anon_inode_getfile(name, fops, priv, flags);
+ file = _anon_inode_getfile(name, fops, priv, flags, context_inode,
+ secure);
if (IS_ERR(file)) {
error = PTR_ERR(file);
goto err_put_unused_fd;
@@ -146,8 +176,45 @@ int anon_inode_getfd(const char *name, const struct file_operations *fops,
put_unused_fd(fd);
return error;
}
+
+/**
+ * anon_inode_getfd - creates a new file instance by hooking it up to
+ * an anonymous inode and a dentry that describe
+ * the "class" of the file
+ *
+ * @name: [in] name of the "class" of the new file
+ * @fops: [in] file operations for the new file
+ * @priv: [in] private data for the new file (will be file's private_data)
+ * @flags: [in] flags
+ *
+ * Creates a new file by hooking it on a single inode. This is
+ * useful for files that do not need to have a full-fledged inode in
+ * order to operate correctly. All the files created with
+ * anon_inode_getfd() will use the same singleton inode, reducing
+ * memory use and avoiding code duplication for the file/inode/dentry
+ * setup. Returns a newly created file descriptor or an error code.
+ */
+int anon_inode_getfd(const char *name, const struct file_operations *fops,
+ void *priv, int flags)
+{
+ return _anon_inode_getfd(name, fops, priv, flags, NULL, false);
+}
EXPORT_SYMBOL_GPL(anon_inode_getfd);
+/**
+ * Like anon_inode_getfd(), but adds the @context_inode argument to
+ * allow security modules to control creation of the new file. Once the
+ * security module makes the decision, this inode is no longer needed
+ * and hence reference to it is not held.
+ */
+int anon_inode_getfd_secure(const char *name, const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode)
+{
+ return _anon_inode_getfd(name, fops, priv, flags, context_inode, true);
+}
+EXPORT_SYMBOL_GPL(anon_inode_getfd_secure);
+
static int __init anon_inode_init(void)
{
anon_inode_mnt = kern_mount(&anon_inode_fs_type);
diff --git a/include/linux/anon_inodes.h b/include/linux/anon_inodes.h
index d0d7d96261ad..6ab840ee93bc 100644
--- a/include/linux/anon_inodes.h
+++ b/include/linux/anon_inodes.h
@@ -10,12 +10,20 @@
#define _LINUX_ANON_INODES_H
struct file_operations;
+struct inode;
struct file *anon_inode_getfile(const char *name,
const struct file_operations *fops,
void *priv, int flags);
+
+int anon_inode_getfd_secure(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode);
+
int anon_inode_getfd(const char *name, const struct file_operations *fops,
void *priv, int flags);
+
#endif /* _LINUX_ANON_INODES_H */
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 2a8c74d99015..35ff75c43de4 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -113,6 +113,8 @@ LSM_HOOK(void, LSM_RET_VOID, inode_free_security, struct inode *inode)
LSM_HOOK(int, 0, inode_init_security, struct inode *inode,
struct inode *dir, const struct qstr *qstr, const char **name,
void **value, size_t *len)
+LSM_HOOK(int, 0, inode_init_security_anon, struct inode *inode,
+ const struct qstr *name, const struct inode *context_inode)
LSM_HOOK(int, 0, inode_create, struct inode *dir, struct dentry *dentry,
umode_t mode)
LSM_HOOK(int, 0, inode_link, struct dentry *old_dentry, struct inode *dir,
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 9e2e3e63719d..586186f1184b 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -233,6 +233,15 @@
* Returns 0 if @name and @value have been successfully set,
* -EOPNOTSUPP if no security attribute is needed, or
* -ENOMEM on memory allocation failure.
+ * @inode_init_security_anon:
+ * Set up the incore security field for the new anonymous inode
+ * and return whether the inode creation is permitted by the security
+ * module or not.
+ * @inode contains the inode structure
+ * @name name of the anonymous inode class
+ * @context_inode optional related inode
+ * Returns 0 on success, -EACCESS if the security module denies the
+ * creation of this inode, or another -errno upon other errors.
* @inode_create:
* Check permission to create a regular file.
* @dir contains inode structure of the parent of the new file.
diff --git a/include/linux/security.h b/include/linux/security.h
index 0a0a03b36a3b..7c6b3dcf4721 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -322,6 +322,9 @@ void security_inode_free(struct inode *inode);
int security_inode_init_security(struct inode *inode, struct inode *dir,
const struct qstr *qstr,
initxattrs initxattrs, void *fs_data);
+int security_inode_init_security_anon(struct inode *inode,
+ const struct qstr *name,
+ const struct inode *context_inode);
int security_old_inode_init_security(struct inode *inode, struct inode *dir,
const struct qstr *qstr, const char **name,
void **value, size_t *len);
@@ -732,6 +735,13 @@ static inline int security_inode_init_security(struct inode *inode,
return 0;
}
+static inline int security_inode_init_security_anon(struct inode *inode,
+ const struct qstr *name,
+ const struct inode *context_inode)
+{
+ return 0;
+}
+
static inline int security_old_inode_init_security(struct inode *inode,
struct inode *dir,
const struct qstr *qstr,
diff --git a/security/security.c b/security/security.c
index 70a7ad357bc6..2c4b121a01b9 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1057,6 +1057,14 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
}
EXPORT_SYMBOL(security_inode_init_security);
+int security_inode_init_security_anon(struct inode *inode,
+ const struct qstr *name,
+ const struct inode *context_inode)
+{
+ return call_int_hook(inode_init_security_anon, 0, inode, name,
+ context_inode);
+}
+
int security_old_inode_init_security(struct inode *inode, struct inode *dir,
const struct qstr *qstr, const char **name,
void **value, size_t *len)
--
2.28.0.297.g1956fa8f8d-goog
^ permalink raw reply related
* [PATCH v8 0/3] SELinux support for anonymous inodes and UFFD
From: Lokesh Gidra @ 2020-08-27 6:35 UTC (permalink / raw)
To: Alexander Viro, James Morris, Stephen Smalley, Casey Schaufler,
Eric Biggers
Cc: Serge E. Hallyn, Paul Moore, Eric Paris, Lokesh Gidra,
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-fsdevel, linux-kernel, linux-security-module, selinux,
kaleshsingh, calin, surenb, nnk, jeffv, kernel-team
Userfaultfd in unprivileged contexts could be potentially very
useful. We'd like to harden userfaultfd to make such unprivileged use
less risky. This patch series allows SELinux to manage userfaultfd
file descriptors and in the future, other kinds of
anonymous-inode-based file descriptor. SELinux policy authors can
apply policy types to anonymous inodes by providing name-based
transition rules keyed off the anonymous inode internal name (
"[userfaultfd]" in the case of userfaultfd(2) file descriptors) and
applying policy to the new SIDs thus produced.
With SELinux managed userfaultfd, an admin can control creation and
movement of the file descriptors. In particular, handling of
a userfaultfd descriptor by a different process is essentially a
ptrace access into the process, without any of the corresponding
security_ptrace_access_check() checks. For privacy, the admin may
want to deny such accesses, which is possible with SELinux support.
Inside the kernel, a new anon_inode interface, anon_inode_getfd_secure,
allows callers to opt into this SELinux management. In this new "secure"
mode, anon_inodes create new ephemeral inodes for anonymous file objects
instead of reusing the normal anon_inodes singleton dummy inode. A new
LSM hook gives security modules an opportunity to configure and veto
these ephemeral inodes.
This patch series is one of two fork of [1] and is an
alternative to [2].
The primary difference between the two patch series is that this
partch series creates a unique inode for each "secure" anonymous
inode, while the other patch series ([2]) continues using the
singleton dummy anonymous inode and adds a way to attach SELinux
security information directly to file objects.
I prefer the approach in this patch series because 1) it's a smaller
patch than [2], and 2) it produces a more regular security
architecture: in this patch series, secure anonymous inodes aren't
S_PRIVATE and they maintain the SELinux property that the label for a
file is in its inode. We do need an additional inode per anonymous
file, but per-struct-file inode creation doesn't seem to be a problem
for pipes and sockets.
The previous version of this feature ([1]) created a new SELinux
security class for userfaultfd file descriptors. This version adopts
the generic transition-based approach of [2].
This patch series also differs from [2] in that it doesn't affect all
anonymous inodes right away --- instead requiring anon_inodes callers
to opt in --- but this difference isn't one of basic approach. The
important question to resolve is whether we should be creating new
inodes or enhancing per-file data.
Changes from the first version of the patch:
- Removed some error checks
- Defined a new anon_inode SELinux class to resolve the
ambiguity in [3]
- Inherit sclass as well as descriptor from context inode
Changes from the second version of the patch:
- Fixed example policy in the commit message to reflect the use of
the new anon_inode class.
Changes from the third version of the patch:
- Dropped the fops parameter to the LSM hook
- Documented hook parameters
- Fixed incorrect class used for SELinux transition
- Removed stray UFFD changed early in the series
- Removed a redundant ERR_PTR(PTR_ERR())
Changes from the fourth version of the patch:
- Removed an unused parameter from an internal function
- Fixed function documentation
Changes from the fifth version of the patch:
- Fixed function documentation in fs/anon_inodes.c and
include/linux/lsm_hooks.h
- Used anon_inode_getfd_secure() in userfaultfd() syscall and removed
owner from userfaultfd_ctx.
Changed from the sixth version of the patch:
- Removed definition of anon_inode_getfile_secure() as there are no
callers.
- Simplified function description of anon_inode_getfd_secure().
- Elaborated more on the purpose of 'context_inode' in commit message.
Changed from the seventh version of the patch:
- Fixed error handling in _anon_inode_getfile().
- Fixed minor comment and indentation related issues.
[1] https://lore.kernel.org/lkml/20200211225547.235083-1-dancol@google.com/
[2] https://lore.kernel.org/linux-fsdevel/20200213194157.5877-1-sds@tycho.nsa.gov/
[3] https://lore.kernel.org/lkml/23f725ca-5b5a-5938-fcc8-5bbbfc9ba9bc@tycho.nsa.gov/
Daniel Colascione (3):
Add a new LSM-supporting anonymous inode interface
Teach SELinux about anonymous inodes
Wire UFFD up to SELinux
fs/anon_inodes.c | 147 ++++++++++++++++++++--------
fs/userfaultfd.c | 19 ++--
include/linux/anon_inodes.h | 8 ++
include/linux/lsm_hook_defs.h | 2 +
include/linux/lsm_hooks.h | 9 ++
include/linux/security.h | 10 ++
security/security.c | 8 ++
security/selinux/hooks.c | 53 ++++++++++
security/selinux/include/classmap.h | 2 +
9 files changed, 209 insertions(+), 49 deletions(-)
--
2.28.0.297.g1956fa8f8d-goog
^ permalink raw reply
* [PATCH v8 3/3] Wire UFFD up to SELinux
From: Lokesh Gidra @ 2020-08-27 6:35 UTC (permalink / raw)
To: Alexander Viro, James Morris, Stephen Smalley, Casey Schaufler,
Eric Biggers
Cc: Serge E. Hallyn, Paul Moore, Eric Paris, Lokesh Gidra,
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-fsdevel, linux-kernel, linux-security-module, selinux,
kaleshsingh, calin, surenb, nnk, jeffv, kernel-team,
Daniel Colascione
In-Reply-To: <20200827063522.2563293-1-lokeshgidra@google.com>
From: Daniel Colascione <dancol@google.com>
This change gives userfaultfd file descriptors a real security
context, allowing policy to act on them.
Signed-off-by: Daniel Colascione <dancol@google.com>
[Remove owner inode from userfaultfd_ctx]
[Use anon_inode_getfd_secure() instead of anon_inode_getfile_secure()
in userfaultfd syscall]
[Use inode of file in userfaultfd_read() in resolve_userfault_fork()]
Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
---
fs/userfaultfd.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 0e4a3837da52..918535b49475 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -978,14 +978,14 @@ static __poll_t userfaultfd_poll(struct file *file, poll_table *wait)
static const struct file_operations userfaultfd_fops;
-static int resolve_userfault_fork(struct userfaultfd_ctx *ctx,
- struct userfaultfd_ctx *new,
+static int resolve_userfault_fork(struct userfaultfd_ctx *new,
+ struct inode *inode,
struct uffd_msg *msg)
{
int fd;
- fd = anon_inode_getfd("[userfaultfd]", &userfaultfd_fops, new,
- O_RDWR | (new->flags & UFFD_SHARED_FCNTL_FLAGS));
+ fd = anon_inode_getfd_secure("[userfaultfd]", &userfaultfd_fops, new,
+ O_RDWR | (new->flags & UFFD_SHARED_FCNTL_FLAGS), inode);
if (fd < 0)
return fd;
@@ -995,7 +995,7 @@ static int resolve_userfault_fork(struct userfaultfd_ctx *ctx,
}
static ssize_t userfaultfd_ctx_read(struct userfaultfd_ctx *ctx, int no_wait,
- struct uffd_msg *msg)
+ struct uffd_msg *msg, struct inode *inode)
{
ssize_t ret;
DECLARE_WAITQUEUE(wait, current);
@@ -1106,7 +1106,7 @@ static ssize_t userfaultfd_ctx_read(struct userfaultfd_ctx *ctx, int no_wait,
spin_unlock_irq(&ctx->fd_wqh.lock);
if (!ret && msg->event == UFFD_EVENT_FORK) {
- ret = resolve_userfault_fork(ctx, fork_nctx, msg);
+ ret = resolve_userfault_fork(fork_nctx, inode, msg);
spin_lock_irq(&ctx->event_wqh.lock);
if (!list_empty(&fork_event)) {
/*
@@ -1166,6 +1166,7 @@ static ssize_t userfaultfd_read(struct file *file, char __user *buf,
ssize_t _ret, ret = 0;
struct uffd_msg msg;
int no_wait = file->f_flags & O_NONBLOCK;
+ struct inode *inode = file_inode(file);
if (ctx->state == UFFD_STATE_WAIT_API)
return -EINVAL;
@@ -1173,7 +1174,7 @@ static ssize_t userfaultfd_read(struct file *file, char __user *buf,
for (;;) {
if (count < sizeof(msg))
return ret ? ret : -EINVAL;
- _ret = userfaultfd_ctx_read(ctx, no_wait, &msg);
+ _ret = userfaultfd_ctx_read(ctx, no_wait, &msg, inode);
if (_ret < 0)
return ret ? ret : _ret;
if (copy_to_user((__u64 __user *) buf, &msg, sizeof(msg)))
@@ -1995,8 +1996,8 @@ SYSCALL_DEFINE1(userfaultfd, int, flags)
/* prevent the mm struct to be freed */
mmgrab(ctx->mm);
- fd = anon_inode_getfd("[userfaultfd]", &userfaultfd_fops, ctx,
- O_RDWR | (flags & UFFD_SHARED_FCNTL_FLAGS));
+ fd = anon_inode_getfd_secure("[userfaultfd]", &userfaultfd_fops, ctx,
+ O_RDWR | (flags & UFFD_SHARED_FCNTL_FLAGS), NULL);
if (fd < 0) {
mmdrop(ctx->mm);
kmem_cache_free(userfaultfd_ctx_cachep, ctx);
--
2.28.0.297.g1956fa8f8d-goog
^ permalink raw reply related
* [PATCH v8 2/3] Teach SELinux about anonymous inodes
From: Lokesh Gidra @ 2020-08-27 6:35 UTC (permalink / raw)
To: Alexander Viro, James Morris, Stephen Smalley, Casey Schaufler,
Eric Biggers
Cc: Serge E. Hallyn, Paul Moore, Eric Paris, Lokesh Gidra,
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-fsdevel, linux-kernel, linux-security-module, selinux,
kaleshsingh, calin, surenb, nnk, jeffv, kernel-team,
Daniel Colascione, Andrew Morton
In-Reply-To: <20200827063522.2563293-1-lokeshgidra@google.com>
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))
+ return 0;
+
+ isec = selinux_inode(inode);
+
+ /*
+ * We only get here once per ephemeral inode. The inode has
+ * been initialized via inode_alloc_security but is otherwise
+ * untouched.
+ */
+
+ if (context_inode) {
+ struct inode_security_struct *context_isec =
+ selinux_inode(context_inode);
+ isec->sclass = context_isec->sclass;
+ isec->sid = context_isec->sid;
+ } else {
+ isec->sclass = SECCLASS_ANON_INODE;
+ rc = security_transition_sid(
+ &selinux_state, tsec->sid, tsec->sid,
+ isec->sclass, name, &isec->sid);
+ if (rc)
+ return rc;
+ }
+
+ isec->initialized = LABEL_INITIALIZED;
+
+ /*
+ * Now that we've initialized security, check whether we're
+ * allowed to actually create this type of anonymous inode.
+ */
+
+ ad.type = LSM_AUDIT_DATA_INODE;
+ ad.u.inode = inode;
+
+ return avc_has_perm(&selinux_state,
+ tsec->sid,
+ isec->sid,
+ isec->sclass,
+ FILE__CREATE,
+ &ad);
+}
+
static int selinux_inode_create(struct inode *dir, struct dentry *dentry, umode_t mode)
{
return may_create(dir, dentry, SECCLASS_FILE);
@@ -6987,6 +7039,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(inode_free_security, selinux_inode_free_security),
LSM_HOOK_INIT(inode_init_security, selinux_inode_init_security),
+ LSM_HOOK_INIT(inode_init_security_anon, selinux_inode_init_security_anon),
LSM_HOOK_INIT(inode_create, selinux_inode_create),
LSM_HOOK_INIT(inode_link, selinux_inode_link),
LSM_HOOK_INIT(inode_unlink, selinux_inode_unlink),
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 40cebde62856..ba2e01a6955c 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -249,6 +249,8 @@ struct security_class_mapping secclass_map[] = {
{"open", "cpu", "kernel", "tracepoint", "read", "write"} },
{ "lockdown",
{ "integrity", "confidentiality", NULL } },
+ { "anon_inode",
+ { COMMON_FILE_PERMS, NULL } },
{ NULL }
};
--
2.28.0.297.g1956fa8f8d-goog
^ permalink raw reply related
* Re: [PATCH v20 22/23] LSM: Add /proc attr entry for full LSM context
From: Randy Dunlap @ 2020-08-26 18:02 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: linux-audit, keescook, john.johansen, penguin-kernel, paul, sds,
linux-api
In-Reply-To: <20200826145247.10029-23-casey@schaufler-ca.com>
Hi,
On 8/26/20 7:52 AM, Casey Schaufler wrote:
> diff --git a/Documentation/security/lsm.rst b/Documentation/security/lsm.rst
> index 6a2a2e973080..fd4c87358d54 100644
> --- a/Documentation/security/lsm.rst
> +++ b/Documentation/security/lsm.rst
> @@ -129,3 +129,31 @@ to identify it as the first security module to be registered.
> The capabilities security module does not use the general security
> blobs, unlike other modules. The reasons are historical and are
> based on overhead, complexity and performance concerns.
> +
> +LSM External Interfaces
> +=======================
> +
> +The LSM infrastructure does not generally provide external interfaces.
> +The individual security modules provide what external interfaces they
> +require.
> +
> +The file ``/sys/kernel/security/lsm`` provides a comma
> +separated list of the active security modules.
> +
> +The file ``/proc/pid/attr/display`` contains the name of the security
> +module for which the ``/proc/pid/attr/current`` interface will
> +apply. This interface can be written to.
> +
> +The infrastructure does provide an interface for the special
> +case where multiple security modules provide a process context.
> +This is provided in compound context format.
> +
> +- `lsm\0value\0lsm\0value\0`
> +
> +The `lsm` and `value` fields are nul terminated bytestrings.
Preferably NUL-terminated
> +Each field may contain whitespace or non-printable characters.
> +The nul bytes are included in the size of a compound context.
NUL
> +The context ``Bell\0Secret\0Biba\0Loose\0`` has a size of 23.
> +
> +The file ``/proc/pid/attr/context`` provides the security
> +context of the identified process.
thanks.
--
~Randy
Reported-by: Randy Dunlap <rdunlap@infradead.org>
^ permalink raw reply
* Re: [PATCH v2 0/2] ima: Fix keyrings race condition and other key related bugs
From: Nayna @ 2020-08-26 15:57 UTC (permalink / raw)
To: Tyler Hicks, Mimi Zohar
Cc: Dmitry Kasatkin, James Morris, Serge E . Hallyn,
Lakshmi Ramasubramanian, Tushar Sugandhi, Nayna Jain,
linux-kernel, linux-integrity, linux-security-module
In-Reply-To: <20200811192621.281675-1-tyhicks@linux.microsoft.com>
On 8/11/20 3:26 PM, Tyler Hicks wrote:
> v2:
> - Always return an ERR_PTR from ima_alloc_rule_opt_list() (Nayna)
> - Add Lakshmi's Reviewed-by to both patches
> - Rebased on commit 3db0d0c276a7 ("integrity: remove redundant
> initialization of variable ret") of next-integrity
> v1: https://lore.kernel.org/lkml/20200727140831.64251-1-tyhicks@linux.microsoft.com/
>
> Nayna pointed out that the "keyrings=" option in an IMA policy rule
> should only be accepted when CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS is
> enabled:
>
> https://lore.kernel.org/linux-integrity/336cc947-1f70-0286-6506-6df3d1d23a1d@linux.vnet.ibm.com/
>
> While fixing this, the compiler warned me about the potential for the
> ima_keyrings pointer to be NULL despite it being used, without a check
> for NULL, as the destination address for the strcpy() in
> ima_match_keyring().
>
> It also became apparent that there was not adequate locking around the
> use of the pre-allocated buffer that ima_keyrings points to. The kernel
> keyring has a lock (.sem member of struct key) that ensures only one key
> can be added to a given keyring at a time but there's no protection
> against adding multiple keys to different keyrings at the same time.
>
> The first patch in this series fixes both ima_keyrings related issues by
> parsing the list of keyrings in a KEY_CHECK rule at policy load time
> rather than deferring the parsing to policy check time. Once that fix is
> in place, the second patch can enforce that
> CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS must be enabled in order to use
> "func=KEY_CHECK" or "keyrings=" options in IMA policy.
>
> The new "keyrings=" value handling is done in a generic manner that can
> be reused by other options in the future. This seems to make sense as
> "appraise_type=" has similar style (though it doesn't need to be fully
> parsed at this time) and using "|" as an alternation delimiter is
> becoming the norm in IMA policy.
>
> This series is based on commit 311aa6aafea4 ("ima: move
> APPRAISE_BOOTPARAM dependency on ARCH_POLICY to runtime") in
> next-integrity.
>
> Tyler
>
> Tyler Hicks (2):
> ima: Pre-parse the list of keyrings in a KEY_CHECK rule
> ima: Fail rule parsing when asymmetric key measurement isn't
> supportable
>
> security/integrity/ima/ima_policy.c | 142 +++++++++++++++++++---------
> 1 file changed, 96 insertions(+), 46 deletions(-)
>
Sorry for delay in responding.
The patches look good. Feel free to add my tag
Reviewed-by: Nayna Jain <nayna@linux.ibm.com>
Thanks & Regards,
- Nayna
^ permalink raw reply
* Re: [PATCH v20 00/23] LSM: Module stacking for AppArmor
From: Casey Schaufler @ 2020-08-26 15:27 UTC (permalink / raw)
To: casey.schaufler, jmorris, linux-security-module, selinux
Cc: linux-audit, keescook, john.johansen, penguin-kernel, paul, sds,
Casey Schaufler
In-Reply-To: <20200826145247.10029-1-casey@schaufler-ca.com>
On 8/26/2020 7:52 AM, Casey Schaufler wrote:
> This patchset provides the changes required for
> the AppArmor security module to stack safely with any other.
>
> v20: Rebase to 5.9-rc1
> Change the BPF security module to use the lsmblob data. (patch 0002)
> Repair length logic in subject label processing (patch 0015)
> Handle -EINVAL from the empty BPF setprocattr hook (patch 0020)
> Correct length processing in append_ctx() (patch 0022)
> ...
>
> https://github.com/cschaufler/lsm-stacking.git#stack-5.8-rc6-a-v19
https://github.com/cschaufler/lsm-stacking.git#stack-5.9-rc1-v20
Sorry about the old URL.
^ permalink raw reply
* [PATCH v20 23/23] AppArmor: Remove the exclusive flag
From: Casey Schaufler @ 2020-08-26 14:52 UTC (permalink / raw)
To: casey.schaufler, jmorris, linux-security-module, selinux
Cc: casey, linux-audit, keescook, john.johansen, penguin-kernel, paul,
sds
In-Reply-To: <20200826145247.10029-1-casey@schaufler-ca.com>
With the inclusion of the "display" process attribute
mechanism AppArmor no longer needs to be treated as an
"exclusive" security module. Remove the flag that indicates
it is exclusive. Remove the stub getpeersec_dgram AppArmor
hook as it has no effect in the single LSM case and
interferes in the multiple LSM case.
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: John Johansen <john.johansen@canonical.com>
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
security/apparmor/lsm.c | 20 +-------------------
1 file changed, 1 insertion(+), 19 deletions(-)
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 7ce570b0f491..4b7cbe9bb1be 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -1129,22 +1129,6 @@ static int apparmor_socket_getpeersec_stream(struct socket *sock,
return error;
}
-/**
- * apparmor_socket_getpeersec_dgram - get security label of packet
- * @sock: the peer socket
- * @skb: packet data
- * @secid: pointer to where to put the secid of the packet
- *
- * Sets the netlabel socket state on sk from parent
- */
-static int apparmor_socket_getpeersec_dgram(struct socket *sock,
- struct sk_buff *skb, u32 *secid)
-
-{
- /* TODO: requires secid support */
- return -ENOPROTOOPT;
-}
-
/**
* apparmor_sock_graft - Initialize newly created socket
* @sk: child sock
@@ -1248,8 +1232,6 @@ static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
#endif
LSM_HOOK_INIT(socket_getpeersec_stream,
apparmor_socket_getpeersec_stream),
- LSM_HOOK_INIT(socket_getpeersec_dgram,
- apparmor_socket_getpeersec_dgram),
LSM_HOOK_INIT(sock_graft, apparmor_sock_graft),
#ifdef CONFIG_NETWORK_SECMARK
LSM_HOOK_INIT(inet_conn_request, apparmor_inet_conn_request),
@@ -1918,7 +1900,7 @@ static int __init apparmor_init(void)
DEFINE_LSM(apparmor) = {
.name = "apparmor",
- .flags = LSM_FLAG_LEGACY_MAJOR | LSM_FLAG_EXCLUSIVE,
+ .flags = LSM_FLAG_LEGACY_MAJOR,
.enabled = &apparmor_enabled,
.blobs = &apparmor_blob_sizes,
.init = apparmor_init,
--
2.24.1
^ permalink raw reply related
* [PATCH v20 22/23] LSM: Add /proc attr entry for full LSM context
From: Casey Schaufler @ 2020-08-26 14:52 UTC (permalink / raw)
To: casey.schaufler, jmorris, linux-security-module, selinux
Cc: casey, linux-audit, keescook, john.johansen, penguin-kernel, paul,
sds, linux-api
In-Reply-To: <20200826145247.10029-1-casey@schaufler-ca.com>
Add an entry /proc/.../attr/context which displays the full
process security "context" in compound format:
lsm1\0value\0lsm2\0value\0...
This entry is not writable.
A security module may decide that its policy does not allow
this information to be displayed. In this case none of the
information will be displayed.
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Cc: linux-api@vger.kernel.org
---
Documentation/security/lsm.rst | 28 ++++++++++
fs/proc/base.c | 1 +
include/linux/lsm_hooks.h | 6 +++
security/apparmor/include/procattr.h | 2 +-
security/apparmor/lsm.c | 8 ++-
security/apparmor/procattr.c | 22 ++++----
security/security.c | 79 ++++++++++++++++++++++++++++
security/selinux/hooks.c | 2 +-
security/smack/smack_lsm.c | 2 +-
9 files changed, 135 insertions(+), 15 deletions(-)
diff --git a/Documentation/security/lsm.rst b/Documentation/security/lsm.rst
index 6a2a2e973080..fd4c87358d54 100644
--- a/Documentation/security/lsm.rst
+++ b/Documentation/security/lsm.rst
@@ -129,3 +129,31 @@ to identify it as the first security module to be registered.
The capabilities security module does not use the general security
blobs, unlike other modules. The reasons are historical and are
based on overhead, complexity and performance concerns.
+
+LSM External Interfaces
+=======================
+
+The LSM infrastructure does not generally provide external interfaces.
+The individual security modules provide what external interfaces they
+require.
+
+The file ``/sys/kernel/security/lsm`` provides a comma
+separated list of the active security modules.
+
+The file ``/proc/pid/attr/display`` contains the name of the security
+module for which the ``/proc/pid/attr/current`` interface will
+apply. This interface can be written to.
+
+The infrastructure does provide an interface for the special
+case where multiple security modules provide a process context.
+This is provided in compound context format.
+
+- `lsm\0value\0lsm\0value\0`
+
+The `lsm` and `value` fields are nul terminated bytestrings.
+Each field may contain whitespace or non-printable characters.
+The nul bytes are included in the size of a compound context.
+The context ``Bell\0Secret\0Biba\0Loose\0`` has a size of 23.
+
+The file ``/proc/pid/attr/context`` provides the security
+context of the identified process.
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 2edb51d4c725..a6802573c238 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2804,6 +2804,7 @@ static const struct pid_entry attr_dir_stuff[] = {
ATTR(NULL, "keycreate", 0666),
ATTR(NULL, "sockcreate", 0666),
ATTR(NULL, "display", 0666),
+ ATTR(NULL, "context", 0444),
#ifdef CONFIG_SECURITY_SMACK
DIR("smack", 0555,
proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops),
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index e559df1df169..b99d51c6771c 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1359,6 +1359,12 @@
* @pages contains the number of pages.
* Return 0 if permission is granted.
*
+ * @getprocattr:
+ * Provide the named process attribute for display in special files in
+ * the /proc/.../attr directory. Attribute naming and the data displayed
+ * is at the discretion of the security modules. The exception is the
+ * "context" attribute, which will contain the security context of the
+ * task as a nul terminated text string without trailing whitespace.
* @ismaclabel:
* Check if the extended attribute specified by @name
* represents a MAC label. Returns 1 if name is a MAC
diff --git a/security/apparmor/include/procattr.h b/security/apparmor/include/procattr.h
index 31689437e0e1..03dbfdb2f2c0 100644
--- a/security/apparmor/include/procattr.h
+++ b/security/apparmor/include/procattr.h
@@ -11,7 +11,7 @@
#ifndef __AA_PROCATTR_H
#define __AA_PROCATTR_H
-int aa_getprocattr(struct aa_label *label, char **string);
+int aa_getprocattr(struct aa_label *label, char **string, bool newline);
int aa_setprocattr_changehat(char *args, size_t size, int flags);
#endif /* __AA_PROCATTR_H */
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 31a6f11890f1..7ce570b0f491 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -593,6 +593,7 @@ static int apparmor_getprocattr(struct task_struct *task, char *name,
const struct cred *cred = get_task_cred(task);
struct aa_task_ctx *ctx = task_ctx(current);
struct aa_label *label = NULL;
+ bool newline = true;
if (strcmp(name, "current") == 0)
label = aa_get_newest_label(cred_label(cred));
@@ -600,11 +601,14 @@ static int apparmor_getprocattr(struct task_struct *task, char *name,
label = aa_get_newest_label(ctx->previous);
else if (strcmp(name, "exec") == 0 && ctx->onexec)
label = aa_get_newest_label(ctx->onexec);
- else
+ else if (strcmp(name, "context") == 0) {
+ label = aa_get_newest_label(cred_label(cred));
+ newline = false;
+ } else
error = -EINVAL;
if (label)
- error = aa_getprocattr(label, value);
+ error = aa_getprocattr(label, value, newline);
aa_put_label(label);
put_cred(cred);
diff --git a/security/apparmor/procattr.c b/security/apparmor/procattr.c
index c929bf4a3df1..be3b083d9b74 100644
--- a/security/apparmor/procattr.c
+++ b/security/apparmor/procattr.c
@@ -20,6 +20,7 @@
* aa_getprocattr - Return the profile information for @profile
* @profile: the profile to print profile info about (NOT NULL)
* @string: Returns - string containing the profile info (NOT NULL)
+ * @newline: Should a newline be added to @string.
*
* Returns: length of @string on success else error on failure
*
@@ -30,20 +31,21 @@
*
* Returns: size of string placed in @string else error code on failure
*/
-int aa_getprocattr(struct aa_label *label, char **string)
+int aa_getprocattr(struct aa_label *label, char **string, bool newline)
{
struct aa_ns *ns = labels_ns(label);
struct aa_ns *current_ns = aa_get_current_ns();
+ int flags = FLAG_VIEW_SUBNS | FLAG_HIDDEN_UNCONFINED;
int len;
if (!aa_ns_visible(current_ns, ns, true)) {
aa_put_ns(current_ns);
return -EACCES;
}
+ if (newline)
+ flags |= FLAG_SHOW_MODE;
- len = aa_label_snxprint(NULL, 0, current_ns, label,
- FLAG_SHOW_MODE | FLAG_VIEW_SUBNS |
- FLAG_HIDDEN_UNCONFINED);
+ len = aa_label_snxprint(NULL, 0, current_ns, label, flags);
AA_BUG(len < 0);
*string = kmalloc(len + 2, GFP_KERNEL);
@@ -52,19 +54,19 @@ int aa_getprocattr(struct aa_label *label, char **string)
return -ENOMEM;
}
- len = aa_label_snxprint(*string, len + 2, current_ns, label,
- FLAG_SHOW_MODE | FLAG_VIEW_SUBNS |
- FLAG_HIDDEN_UNCONFINED);
+ len = aa_label_snxprint(*string, len + 2, current_ns, label, flags);
if (len < 0) {
aa_put_ns(current_ns);
return len;
}
- (*string)[len] = '\n';
- (*string)[len + 1] = 0;
+ if (newline) {
+ (*string)[len] = '\n';
+ (*string)[++len] = 0;
+ }
aa_put_ns(current_ns);
- return len + 1;
+ return len;
}
/**
diff --git a/security/security.c b/security/security.c
index 4752291376bf..537df0839404 100644
--- a/security/security.c
+++ b/security/security.c
@@ -754,6 +754,57 @@ static void __init lsm_early_task(struct task_struct *task)
panic("%s: Early task alloc failed.\n", __func__);
}
+/**
+ * append_ctx - append a lsm/context pair to a compound context
+ * @ctx: the existing compound context
+ * @ctxlen: size of the old context, including terminating nul byte
+ * @lsm: new lsm name, nul terminated
+ * @new: new context, possibly nul terminated
+ * @newlen: maximum size of @new
+ *
+ * replace @ctx with a new compound context, appending @newlsm and @new
+ * to @ctx. On exit the new data replaces the old, which is freed.
+ * @ctxlen is set to the new size, which includes a trailing nul byte.
+ *
+ * Returns 0 on success, -ENOMEM if no memory is available.
+ */
+static int append_ctx(char **ctx, int *ctxlen, const char *lsm, char *new,
+ int newlen)
+{
+ char *final;
+ size_t llen;
+ size_t nlen;
+ size_t flen;
+
+ llen = strlen(lsm) + 1;
+ /*
+ * A security module may or may not provide a trailing nul on
+ * when returning a security context. There is no definition
+ * of which it should be, and there are modules that do it
+ * each way.
+ */
+ nlen = strnlen(new, newlen);
+
+ flen = *ctxlen + llen + nlen + 1;
+ final = kzalloc(flen, GFP_KERNEL);
+
+ if (final == NULL)
+ return -ENOMEM;
+
+ if (*ctxlen)
+ memcpy(final, *ctx, *ctxlen);
+
+ memcpy(final + *ctxlen, lsm, llen);
+ memcpy(final + *ctxlen + llen, new, nlen);
+
+ kfree(*ctx);
+
+ *ctx = final;
+ *ctxlen = flen;
+
+ return 0;
+}
+
/*
* The default value of the LSM hook is defined in linux/lsm_hook_defs.h and
* can be accessed with:
@@ -2124,6 +2175,10 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
char **value)
{
struct security_hook_list *hp;
+ char *final = NULL;
+ char *cp;
+ int rc = 0;
+ int finallen = 0;
int display = lsm_task_display(current);
int slot = 0;
@@ -2151,6 +2206,30 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
return -ENOMEM;
}
+ if (!strcmp(name, "context")) {
+ hlist_for_each_entry(hp, &security_hook_heads.getprocattr,
+ list) {
+ rc = hp->hook.getprocattr(p, "context", &cp);
+ if (rc == -EINVAL)
+ continue;
+ if (rc < 0) {
+ kfree(final);
+ return rc;
+ }
+ rc = append_ctx(&final, &finallen, hp->lsmid->lsm,
+ cp, rc);
+ kfree(cp);
+ if (rc < 0) {
+ kfree(final);
+ return rc;
+ }
+ }
+ if (final == NULL)
+ return -EINVAL;
+ *value = final;
+ return finallen;
+ }
+
hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
continue;
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 89c22769506b..09379ea04fc4 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6293,7 +6293,7 @@ static int selinux_getprocattr(struct task_struct *p,
goto bad;
}
- if (!strcmp(name, "current"))
+ if (!strcmp(name, "current") || !strcmp(name, "context"))
sid = __tsec->sid;
else if (!strcmp(name, "prev"))
sid = __tsec->osid;
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 6f0cdb40addc..d7bb6442f192 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3463,7 +3463,7 @@ static int smack_getprocattr(struct task_struct *p, char *name, char **value)
char *cp;
int slen;
- if (strcmp(name, "current") != 0)
+ if (strcmp(name, "current") != 0 && strcmp(name, "context") != 0)
return -EINVAL;
cp = kstrdup(skp->smk_known, GFP_KERNEL);
--
2.24.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox