* Re: [PATCH] IMA: Handle early boot data measurement
From: Lakshmi Ramasubramanian @ 2020-08-25 17:55 UTC (permalink / raw)
To: Mimi Zohar, stephen.smalley.work, casey
Cc: tyhicks, tusharsu, sashal, jmorris, linux-integrity, selinux,
linux-security-module, linux-kernel
In-Reply-To: <49f8a616d80344c539b512f8b83590ea281ee54d.camel@linux.ibm.com>
On 8/25/20 10:42 AM, Mimi Zohar wrote:
>>>
>>> Please limit the changes in this patch to renaming the functions and/or
>>> files. For example, adding "measure_payload_hash" should be a separate
>>> patch, not hidden here.
>>>
>>
>> Thanks for the feedback Mimi.
>>
>> I'll split this into 2 patches:
>>
>> PATCH 1: Rename files + rename CONFIG
>> PATCH 2: Update IMA hook to utilize early boot data measurement.
>
> I'm referring to introducing the "measure_payload_hash" flag. I assume
> this is to indicate whether the buffer should be hashed or not.
>
> Example 1: ima_alloc_key_entry() and ima_alloc_data_entry(0 comparison
>> -static struct ima_key_entry *ima_alloc_key_entry(struct key *keyring,
>> - const void *payload,
>> - size_t payload_len)
>> -{
>
>
>> +static struct ima_data_entry *ima_alloc_data_entry(const char *event_name,
>> + const void *payload,
>> + size_t payload_len,
>> + const char *event_data,
>> + enum ima_hooks func,
>> + bool measure_payload_hash) <====
>> +{
>
> Example 2:
> diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
> index a74095793936..65423754765f 100644
> --- a/security/integrity/ima/ima_asymmetric_keys.c
> +++ b/security/integrity/ima/ima_asymmetric_keys.c
> @@ -37,9 +37,10 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
> if (!payload || (payload_len == 0))
> return;
>
> - if (ima_should_queue_key())
> - queued = ima_queue_key(keyring, payload, payload_len);
> -
> + if (ima_should_queue_data())
> + queued = ima_queue_data(keyring->description, payload,
> + payload_len, keyring->description,
> + KEY_CHECK, false); <===
> if (queued)
> return;
>
> But in general, as much as possible function and file name changes
> should be done independently of other changes.
>
> thanks,
I agree - but in this case, Tushar's patch series on adding support for
"Critical Data" measurement has already introduced
"measure_payload_hash" flag. His patch updates
"process_buffer_measurement()" to take this new flag and measure hash of
the given data.
My patches extend that to queuing the early boot requests and processing
them after a custom IMA policy is loaded.
If you still think "measure_payload_hash" flag should be introduced in
the queuing change as a separate patch I'll split the patches further.
Please let me know.
thanks,
-lakshmi
^ permalink raw reply
* Re: [PATCH] IMA: Handle early boot data measurement
From: Mimi Zohar @ 2020-08-25 17:42 UTC (permalink / raw)
To: Lakshmi Ramasubramanian, stephen.smalley.work, casey
Cc: tyhicks, tusharsu, sashal, jmorris, linux-integrity, selinux,
linux-security-module, linux-kernel
In-Reply-To: <307617de-b42d-ac52-6e9e-9e0d16bbc20e@linux.microsoft.com>
On Tue, 2020-08-25 at 08:46 -0700, Lakshmi Ramasubramanian wrote:
> On 8/25/20 8:40 AM, Mimi Zohar wrote:
> > On Fri, 2020-08-21 at 16:12 -0700, Lakshmi Ramasubramanian wrote:
> > > The current implementation of early boot measurement in
> > > the IMA subsystem is very specific to asymmetric keys. It does not
> > > handle early boot measurement of data from other subsystems such as
> > > Linux Security Module (LSM), Device-Mapper, etc. As a result data,
> > > provided by these subsystems during system boot are not measured by IMA.
> > >
> > > Update the early boot key measurement to handle any early boot data.
> > > Refactor the code from ima_queue_keys.c to a new file ima_queue_data.c.
> > > Rename the kernel configuration CONFIG_IMA_QUEUE_EARLY_BOOT_KEYS to
> > > CONFIG_IMA_QUEUE_EARLY_BOOT_DATA so it can be used for enabling any
> > > early boot data measurement. Since measurement of asymmetric keys is
> > > the first consumer of early boot measurement, this kernel configuration
> > > is enabled if IMA_MEASURE_ASYMMETRIC_KEYS and SYSTEM_TRUSTED_KEYRING are
> > > both enabled.
> > >
> > > Update the IMA hook ima_measure_critical_data() to utilize early boot
> > > measurement support.
> >
> > Please limit the changes in this patch to renaming the functions and/or
> > files. For example, adding "measure_payload_hash" should be a separate
> > patch, not hidden here.
> >
>
> Thanks for the feedback Mimi.
>
> I'll split this into 2 patches:
>
> PATCH 1: Rename files + rename CONFIG
> PATCH 2: Update IMA hook to utilize early boot data measurement.
I'm referring to introducing the "measure_payload_hash" flag. I assume
this is to indicate whether the buffer should be hashed or not.
Example 1: ima_alloc_key_entry() and ima_alloc_data_entry(0 comparison
> -static struct ima_key_entry *ima_alloc_key_entry(struct key *keyring,
> - const void *payload,
> - size_t payload_len)
> -{
> +static struct ima_data_entry *ima_alloc_data_entry(const char *event_name,
> + const void *payload,
> + size_t payload_len,
> + const char *event_data,
> + enum ima_hooks func,
> + bool measure_payload_hash) <====
> +{
Example 2:
diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
index a74095793936..65423754765f 100644
--- a/security/integrity/ima/ima_asymmetric_keys.c
+++ b/security/integrity/ima/ima_asymmetric_keys.c
@@ -37,9 +37,10 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
if (!payload || (payload_len == 0))
return;
- if (ima_should_queue_key())
- queued = ima_queue_key(keyring, payload, payload_len);
-
+ if (ima_should_queue_data())
+ queued = ima_queue_data(keyring->description, payload,
+ payload_len, keyring->description,
+ KEY_CHECK, false); <===
if (queued)
return;
But in general, as much as possible function and file name changes
should be done independently of other changes.
thanks,
Mimi
^ permalink raw reply related
* Re: [PATCH bpf-next v9 5/7] bpf: Implement bpf_local_storage for inodes
From: Martin KaFai Lau @ 2020-08-25 17:39 UTC (permalink / raw)
To: KP Singh
Cc: linux-kernel, bpf, linux-security-module, Alexei Starovoitov,
Daniel Borkmann, Paul Turner, Jann Horn, Florent Revest
In-Reply-To: <8d188285-96f5-3b17-126f-5e842702e339@chromium.org>
On Tue, Aug 25, 2020 at 04:10:26PM +0200, KP Singh wrote:
>
>
> On 8/25/20 2:52 AM, Martin KaFai Lau wrote:
> > On Sun, Aug 23, 2020 at 06:56:10PM +0200, KP Singh wrote:
> >> From: KP Singh <kpsingh@google.com>
> >>
> >> Similar to bpf_local_storage for sockets, add local storage for inodes.
> >> The life-cycle of storage is managed with the life-cycle of the inode.
> >> i.e. the storage is destroyed along with the owning inode.
> >>
> >> The BPF LSM allocates an __rcu pointer to the bpf_local_storage in the
> >> security blob which are now stackable and can co-exist with other LSMs.
> >>
> > [ ... ]
> >
> >> diff --git a/kernel/bpf/bpf_inode_storage.c b/kernel/bpf/bpf_inode_storage.c
> >> new file mode 100644
> >> index 000000000000..b0b283c224c1
> >> --- /dev/null
> >> +++ b/kernel/bpf/bpf_inode_storage.c
>
> [...]
>
> >> +
> >> +DEFINE_BPF_STORAGE_CACHE(inode_cache);
> >> +
> >> +static struct bpf_local_storage __rcu **
> >> +inode_storage_ptr(void *owner)
> >> +{
> >> + struct inode *inode = owner;
> >> + struct bpf_storage_blob *bsb;
> >> +
> >> + bsb = bpf_inode(inode);
> >> + if (!bsb)
> >> + return NULL;
> > just noticed this one. NULL could be returned here. When will it happen?
>
> This can happen if CONFIG_BPF_LSM is enabled but "bpf" is not in the list of
> active LSMs.
>
> >
> >> + return &bsb->storage;
> >> +}
> >> +
> >> +static struct bpf_local_storage_data *inode_storage_lookup(struct inode *inode,
> >> + struct bpf_map *map,
> >> + bool cacheit_lockit)
> >> +{
>
> [...]
>
> > path first before calling the bpf_local_storage_update() since
> > this case is specific to inode local storage.
> >
> > Same for the other bpf_local_storage_update() cases.
>
> If you're okay with this I can send a new series with the following updates.
>
> diff --git a/kernel/bpf/bpf_inode_storage.c b/kernel/bpf/bpf_inode_storage.c
> index b0b283c224c1..74546cee814d 100644
> --- a/kernel/bpf/bpf_inode_storage.c
> +++ b/kernel/bpf/bpf_inode_storage.c
> @@ -125,7 +125,7 @@ static int bpf_fd_inode_storage_update_elem(struct bpf_map *map, void *key,
>
> fd = *(int *)key;
> f = fget_raw(fd);
> - if (!f)
> + if (!f || !inode_storage_ptr(f->f_inode))
> return -EBADF;
>
> sdata = bpf_local_storage_update(f->f_inode,
> @@ -171,6 +171,14 @@ BPF_CALL_4(bpf_inode_storage_get, struct bpf_map *, map, struct inode *, inode,
> if (flags & ~(BPF_LOCAL_STORAGE_GET_F_CREATE))
> return (unsigned long)NULL;
>
> + /* explicitly check that the inode_storage_ptr is not
> + * NULL as inode_storage_lookup returns NULL in this case and
> + * and bpf_local_storage_update expects the owner to have a
> + * valid storage pointer.
> + */
> + if (!inode_storage_ptr(inode))
> + return (unsigned long)NULL;
LGTM
^ permalink raw reply
* Re: [PATCH v2 2/3] IMA: add policy to support measuring critical data from kernel components
From: Tushar Sugandhi @ 2020-08-25 17:32 UTC (permalink / raw)
To: Mimi 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: <d82c5cdab170d3dcc513b38632801c3aa14ca389.camel@linux.ibm.com>
On 2020-08-24 3:46 p.m., Mimi Zohar wrote:
> On Fri, 2020-08-21 at 11:21 -0700, Tushar Sugandhi wrote:
>> There would be several candidate kernel components suitable for IMA
>> measurement. Not all of them would have support for IMA measurement.
>> Also, system administrators may not want to measure data for all of
>> them, even when they support IMA measurement. An IMA policy specific
>> to various kernel components is needed to measure their respective
>> critical data.
>>
>> Add a new IMA policy CRITICAL_DATA+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 | 6 ++-
>> security/integrity/ima/ima.h | 1 +
>> security/integrity/ima/ima_api.c | 2 +-
>> security/integrity/ima/ima_policy.c | 62 +++++++++++++++++++++++++---
>> 4 files changed, 63 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
>> index cd572912c593..a0dd0f108555 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
>> @@ -125,3 +125,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 data_sources=selinux|apparmor|dm-crypt
>
> This example uses "data_sources" without first defining it in the
> "option:" section. Defining two new options is an indication that this
Thanks. I will define "data_sources" first in "option:" section.
> patch should be split up. One which defines the "CRITICAL_DATA" and
> another one which defines the new key value pair. The term
I intentionally kept the "CRITICAL_DATA" and "data_sources" in the same
patch.
CRITICAL_DATA is different than KEY_CHECK because in case of KEY_CHECK,
"keyrings=" is optional. If "keyrings=" is not specified, then we
measure all keyrings.
Where for CRITICAL_DATA, "data_sources=" is mandatory.
Because the data sources would be diverse and orthogonal to each other,
(unlike "keyrings=") - not specifying "data_sources=" shouldn't result
in IMA blindly measuring all data sources.
Since CRITICAL_DATA, and "data_sources=" go hand in hand, I wanted them
to be part of the same patch.
> "data_sources" is pretty generic. Perhaps constrain it a bit by re-
> naming it "critical_data=". Or was such using a generic name
> intentional?
>
We intentionally kept the name generic because the data to be measured
could be coming from any kernel component with any granularity (from a
single bool to megabytes of data). The kernel component is also loosely
defined here. It could be an LSM (like SELinux), or a broader base layer
(like device-mapper), or a specific module (like dm-crypt), or it could
be different parts of a single module.
Also, we didn't want to name "data_sources" as "critical_data" to avoid
confusion with func "CRITICAL_DATA".
> Normally "CRITICAL_DATA" would be defined with the critical data hook,
> but that seems to be defined in patch 3/3 "IMA: define IMA hook to
> measure critical data from kernel components".
>
I can make the "CRITICAL_DATA" and the hook as part of the same patch.
That would mean combining patch 2 and 3 into a single one.
Does it sound ok?
>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>> index 8875085db689..0f4209a92bfb 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_policy.c b/security/integrity/ima/ima_policy.c
>> index 8866e84d0062..7b649095ac7a 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;
>> };
>>
>> @@ -508,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;
>> @@ -911,7 +922,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 +959,7 @@ static const match_table_t policy_tokens = {
>> {Opt_pcr, "pcr=%s"},
>> {Opt_template, "template=%s"},
>> {Opt_keyrings, "keyrings=%s"},
>> + {Opt_data_sources, "data_sources=%s"},
>> {Opt_err, NULL}
>> };
>>
>> @@ -1110,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;
>
> Requiring IMA_FUNC and IMA_DATA_SOURCES makes sense, but why are
> IMA_UID and IMA_PCR required?
>
Since the data to be measured could be for any scenario, I didn't want
to restrict the kernel components from choosing UID to measure the data
for, or restrict them from choosing PCR to store the measurements in.
But as the consumers are kernel components, perhaps support for IMA_UID
is not required. But we should still support IMA_PCR.
Please let me know what do you think, and I can update the logic
accordingly.
>> +
>> + if (ima_rule_contains_lsm_cond(entry))
>> + return false;
>> +
>> break;
>> default:
>> return false;
>> @@ -1242,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)
>> @@ -1312,6 +1339,23 @@ 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, "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;
>> + }
>> +
>
> "keyrings=" isn't bounded because keyrings can be created by userspace.
> Perhaps keyring names has a minimum/maximum length. IMA isn't
> measuring userspace construsts. Shouldn't the list of critical data
> being measured be bounded and verified?
The comment is not entirely clear.
Do you mean there should be some sort of allow_list in IMA, against
which the values in "data_sources=" should be vetted? And if the
value is present in the IMA allow_list, then only the measurements for
that data source are allowed?
Or do you mean something else?
~Tushar
>
> Mimi
>
>> + entry->flags |= IMA_DATA_SOURCES;
>> + break;
>> case Opt_fsuuid:
>> ima_log_string(ab, "fsuuid", args[0].from);
>>
>> @@ -1692,6 +1736,12 @@ int ima_policy_show(struct seq_file *m, void *v)
>> seq_puts(m, " ");
>> }
>>
>> + if (entry->flags & IMA_DATA_SOURCES) {
>> + seq_puts(m, "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);
>
^ permalink raw reply
* Re: [PATCH v36 11/24] x86/sgx: Add SGX enclave driver
From: Borislav Petkov @ 2020-08-25 16:44 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: x86, linux-sgx, linux-kernel, linux-security-module, linux-mm,
Andrew Morton, Matthew Wilcox, Jethro Beekman, Haitao Huang,
Chunyang Hui, Jordan Hand, Nathaniel McCallum, Seth Moore,
Sean Christopherson, Suresh Siddha, andriy.shevchenko, asapek,
cedric.xing, chenalexchen, conradparker, cyhanish, dave.hansen,
haitao.huang, josh, kai.huang, kai.svahn, kmoy, ludloff, luto,
nhorman, puiterwijk, rientjes, tglx, yaozhangx
In-Reply-To: <20200716135303.276442-12-jarkko.sakkinen@linux.intel.com>
On Thu, Jul 16, 2020 at 04:52:50PM +0300, Jarkko Sakkinen wrote:
Just minor things below - I'm not even going to pretend I fully
understand what's going on but FWICT, it looks non-threateningly ok to
me.
> diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c
> new file mode 100644
> index 000000000000..b52520407f5b
> --- /dev/null
> +++ b/arch/x86/kernel/cpu/sgx/driver.c
> @@ -0,0 +1,177 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
> +// Copyright(c) 2016-18 Intel Corporation.
> +
> +#include <linux/acpi.h>
> +#include <linux/miscdevice.h>
> +#include <linux/mman.h>
> +#include <linux/security.h>
> +#include <linux/suspend.h>
> +#include <asm/traps.h>
> +#include "driver.h"
> +#include "encl.h"
> +
> +MODULE_DESCRIPTION("Intel SGX Enclave Driver");
> +MODULE_AUTHOR("Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>");
> +MODULE_LICENSE("Dual BSD/GPL");
That boilerplate stuff usually goes to the end of the file.
...
> +static struct sgx_encl_page *sgx_encl_load_page(struct sgx_encl *encl,
> + unsigned long addr)
> +{
> + struct sgx_encl_page *entry;
> + unsigned int flags;
> +
> + /* If process was forked, VMA is still there but vm_private_data is set
> + * to NULL.
> + */
> + if (!encl)
> + return ERR_PTR(-EFAULT);
> +
> + flags = atomic_read(&encl->flags);
> +
^ Superfluous newline.
> + if ((flags & SGX_ENCL_DEAD) || !(flags & SGX_ENCL_INITIALIZED))
> + return ERR_PTR(-EFAULT);
> +
> + entry = xa_load(&encl->page_array, PFN_DOWN(addr));
> + if (!entry)
> + return ERR_PTR(-EFAULT);
> +
> + /* Page is already resident in the EPC. */
> + if (entry->epc_page)
> + return entry;
> +
> + return ERR_PTR(-EFAULT);
> +}
> +
> +static void sgx_mmu_notifier_release(struct mmu_notifier *mn,
> + struct mm_struct *mm)
> +{
> + struct sgx_encl_mm *encl_mm =
> + container_of(mn, struct sgx_encl_mm, mmu_notifier);
Just let it stick out.
> + struct sgx_encl_mm *tmp = NULL;
> +
> + /*
> + * The enclave itself can remove encl_mm. Note, objects can't be moved
> + * off an RCU protected list, but deletion is ok.
> + */
> + spin_lock(&encl_mm->encl->mm_lock);
> + list_for_each_entry(tmp, &encl_mm->encl->mm_list, list) {
> + if (tmp == encl_mm) {
> + list_del_rcu(&encl_mm->list);
> + break;
> + }
> + }
> + spin_unlock(&encl_mm->encl->mm_lock);
> +
> + if (tmp == encl_mm) {
> + synchronize_srcu(&encl_mm->encl->srcu);
> + mmu_notifier_put(mn);
> + }
> +}
> +
> +static void sgx_mmu_notifier_free(struct mmu_notifier *mn)
> +{
> + struct sgx_encl_mm *encl_mm =
> + container_of(mn, struct sgx_encl_mm, mmu_notifier);
Ditto.
...
> +/**
> + * sgx_encl_may_map() - Check if a requested VMA mapping is allowed
> + * @encl: an enclave
> + * @start: lower bound of the address range, inclusive
> + * @end: upper bound of the address range, exclusive
> + * @vm_prot_bits: requested protections of the address range
> + *
> + * Iterate through the enclave pages contained within [@start, @end) to verify
> + * the permissions requested by @vm_prot_bits do not exceed that of any enclave
> + * page to be mapped.
> + *
> + * Return:
> + * 0 on success,
> + * -EACCES if VMA permissions exceed enclave page permissions
> + */
> +int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start,
> + unsigned long end, unsigned long vm_flags)
> +{
> + unsigned long vm_prot_bits = vm_flags & (VM_READ | VM_WRITE | VM_EXEC);
> + unsigned long idx_start = PFN_DOWN(start);
> + unsigned long idx_end = PFN_DOWN(end - 1);
> + struct sgx_encl_page *page;
> + XA_STATE(xas, &encl->page_array, idx_start);
> +
> + /*
> + * Disallow RIE tasks as their VMA permissions might conflict with the
"RIE", hmm what is that?
/me looks at the test
Aaah, READ_IMPLIES_EXEC. Is "RIE" some widely accepted acronym I'm not
aware of?
> + * enclave page permissions.
> + */
> + if (!!(current->personality & READ_IMPLIES_EXEC))
The "!!" is not really needed - you're in boolean context.
...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
* Re: [PATCH] IMA: Handle early boot data measurement
From: Lakshmi Ramasubramanian @ 2020-08-25 15:46 UTC (permalink / raw)
To: Mimi Zohar, stephen.smalley.work, casey
Cc: tyhicks, tusharsu, sashal, jmorris, linux-integrity, selinux,
linux-security-module, linux-kernel
In-Reply-To: <a7ea2da1f895ee3db4697c00804160acb6db656e.camel@linux.ibm.com>
On 8/25/20 8:40 AM, Mimi Zohar wrote:
> On Fri, 2020-08-21 at 16:12 -0700, Lakshmi Ramasubramanian wrote:
>> The current implementation of early boot measurement in
>> the IMA subsystem is very specific to asymmetric keys. It does not
>> handle early boot measurement of data from other subsystems such as
>> Linux Security Module (LSM), Device-Mapper, etc. As a result data,
>> provided by these subsystems during system boot are not measured by IMA.
>>
>> Update the early boot key measurement to handle any early boot data.
>> Refactor the code from ima_queue_keys.c to a new file ima_queue_data.c.
>> Rename the kernel configuration CONFIG_IMA_QUEUE_EARLY_BOOT_KEYS to
>> CONFIG_IMA_QUEUE_EARLY_BOOT_DATA so it can be used for enabling any
>> early boot data measurement. Since measurement of asymmetric keys is
>> the first consumer of early boot measurement, this kernel configuration
>> is enabled if IMA_MEASURE_ASYMMETRIC_KEYS and SYSTEM_TRUSTED_KEYRING are
>> both enabled.
>>
>> Update the IMA hook ima_measure_critical_data() to utilize early boot
>> measurement support.
>
> Please limit the changes in this patch to renaming the functions and/or
> files. For example, adding "measure_payload_hash" should be a separate
> patch, not hidden here.
>
Thanks for the feedback Mimi.
I'll split this into 2 patches:
PATCH 1: Rename files + rename CONFIG
PATCH 2: Update IMA hook to utilize early boot data measurement.
-lakshmi
^ permalink raw reply
* Re: [PATCH] IMA: Handle early boot data measurement
From: Mimi Zohar @ 2020-08-25 15:40 UTC (permalink / raw)
To: Lakshmi Ramasubramanian, stephen.smalley.work, casey
Cc: tyhicks, tusharsu, sashal, jmorris, linux-integrity, selinux,
linux-security-module, linux-kernel
In-Reply-To: <20200821231230.20212-1-nramas@linux.microsoft.com>
On Fri, 2020-08-21 at 16:12 -0700, Lakshmi Ramasubramanian wrote:
> The current implementation of early boot measurement in
> the IMA subsystem is very specific to asymmetric keys. It does not
> handle early boot measurement of data from other subsystems such as
> Linux Security Module (LSM), Device-Mapper, etc. As a result data,
> provided by these subsystems during system boot are not measured by IMA.
>
> Update the early boot key measurement to handle any early boot data.
> Refactor the code from ima_queue_keys.c to a new file ima_queue_data.c.
> Rename the kernel configuration CONFIG_IMA_QUEUE_EARLY_BOOT_KEYS to
> CONFIG_IMA_QUEUE_EARLY_BOOT_DATA so it can be used for enabling any
> early boot data measurement. Since measurement of asymmetric keys is
> the first consumer of early boot measurement, this kernel configuration
> is enabled if IMA_MEASURE_ASYMMETRIC_KEYS and SYSTEM_TRUSTED_KEYRING are
> both enabled.
>
> Update the IMA hook ima_measure_critical_data() to utilize early boot
> measurement support.
Please limit the changes in this patch to renaming the functions and/or
files. For example, adding "measure_payload_hash" should be a separate
patch, not hidden here.
Mimi
^ permalink raw reply
* Re: [PATCH bpf-next v9 5/7] bpf: Implement bpf_local_storage for inodes
From: KP Singh @ 2020-08-25 14:10 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: linux-kernel, bpf, linux-security-module, Alexei Starovoitov,
Daniel Borkmann, Paul Turner, Jann Horn, Florent Revest
In-Reply-To: <20200825005249.tu4c54fg36jt3rh4@kafai-mbp.dhcp.thefacebook.com>
On 8/25/20 2:52 AM, Martin KaFai Lau wrote:
> On Sun, Aug 23, 2020 at 06:56:10PM +0200, KP Singh wrote:
>> From: KP Singh <kpsingh@google.com>
>>
>> Similar to bpf_local_storage for sockets, add local storage for inodes.
>> The life-cycle of storage is managed with the life-cycle of the inode.
>> i.e. the storage is destroyed along with the owning inode.
>>
>> The BPF LSM allocates an __rcu pointer to the bpf_local_storage in the
>> security blob which are now stackable and can co-exist with other LSMs.
>>
> [ ... ]
>
>> diff --git a/kernel/bpf/bpf_inode_storage.c b/kernel/bpf/bpf_inode_storage.c
>> new file mode 100644
>> index 000000000000..b0b283c224c1
>> --- /dev/null
>> +++ b/kernel/bpf/bpf_inode_storage.c
[...]
>> +
>> +DEFINE_BPF_STORAGE_CACHE(inode_cache);
>> +
>> +static struct bpf_local_storage __rcu **
>> +inode_storage_ptr(void *owner)
>> +{
>> + struct inode *inode = owner;
>> + struct bpf_storage_blob *bsb;
>> +
>> + bsb = bpf_inode(inode);
>> + if (!bsb)
>> + return NULL;
> just noticed this one. NULL could be returned here. When will it happen?
This can happen if CONFIG_BPF_LSM is enabled but "bpf" is not in the list of
active LSMs.
>
>> + return &bsb->storage;
>> +}
>> +
>> +static struct bpf_local_storage_data *inode_storage_lookup(struct inode *inode,
>> + struct bpf_map *map,
>> + bool cacheit_lockit)
>> +{
[...]
> path first before calling the bpf_local_storage_update() since
> this case is specific to inode local storage.
>
> Same for the other bpf_local_storage_update() cases.
If you're okay with this I can send a new series with the following updates.
diff --git a/kernel/bpf/bpf_inode_storage.c b/kernel/bpf/bpf_inode_storage.c
index b0b283c224c1..74546cee814d 100644
--- a/kernel/bpf/bpf_inode_storage.c
+++ b/kernel/bpf/bpf_inode_storage.c
@@ -125,7 +125,7 @@ static int bpf_fd_inode_storage_update_elem(struct bpf_map *map, void *key,
fd = *(int *)key;
f = fget_raw(fd);
- if (!f)
+ if (!f || !inode_storage_ptr(f->f_inode))
return -EBADF;
sdata = bpf_local_storage_update(f->f_inode,
@@ -171,6 +171,14 @@ BPF_CALL_4(bpf_inode_storage_get, struct bpf_map *, map, struct inode *, inode,
if (flags & ~(BPF_LOCAL_STORAGE_GET_F_CREATE))
return (unsigned long)NULL;
+ /* explicitly check that the inode_storage_ptr is not
+ * NULL as inode_storage_lookup returns NULL in this case and
+ * and bpf_local_storage_update expects the owner to have a
+ * valid storage pointer.
+ */
+ if (!inode_storage_ptr(inode))
+ return (unsigned long)NULL;
+
sdata = inode_storage_lookup(inode, map, true);
if (sdata)
return (unsigned long)sdata->data;
>
>> + (struct bpf_local_storage_map *)map,
>> + value, map_flags);
>> + fput(f);
>> + return PTR_ERR_OR_ZERO(sdata);
>> +}
>> +
>
[...]
>> + return (unsigned long)NULL;
>> +}
>> +
>
>> diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
>> index 32d32d485451..35f9b19259e5 100644
>> --- a/security/bpf/hooks.c
>> +++ b/security/bpf/hooks.c
>> @@ -3,6 +3,7 @@
>> /*
>> * Copyright (C) 2020 Google LLC.
>> */
>> +#include <linux/bpf_local_storage.h>
> Is it needed?
No. Removed. Thanks!
>
>> #include <linux/lsm_hooks.h>
>> #include <linux/bpf_lsm.h>
>>
>> @@ -11,6 +12,7 @@ static struct security_hook_list bpf_lsm_hooks[] __lsm_ro_after_init = {
>> LSM_HOOK_INIT(NAME, bpf_lsm_##NAME),
[...]
>> + .blobs = &bpf_lsm_blob_sizes
>> };
^ permalink raw reply related
* Re: [PATCH v7 1/3] Add a new LSM-supporting anonymous inode interface
From: Eric Biggers @ 2020-08-25 3:50 UTC (permalink / raw)
To: Lokesh Gidra
Cc: Alexander Viro, James Morris, Stephen Smalley, Casey Schaufler,
Serge E. Hallyn, Paul Moore, Eric Paris, Daniel Colascione,
Kees Cook, Eric W. Biederman, KP Singh, David Howells,
Thomas Cedeno, Anders Roxell, Sami Tolvanen, Matthew Garrett,
Aaron Goidel, Randy Dunlap, Joel Fernandes (Google), YueHaibing,
Christian Brauner, Alexei Starovoitov, Alexey Budankov,
Adrian Reber, Aleksa Sarai, linux-fsdevel, linux-kernel,
linux-security-module, selinux, kaleshsingh, calin, surenb, nnk,
jeffv, kernel-team, Daniel Colascione
In-Reply-To: <20200821185645.801971-2-lokeshgidra@google.com>
On Fri, Aug 21, 2020 at 11:56:43AM -0700, Lokesh Gidra wrote:
> 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()]
>
> Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
> ---
> fs/anon_inodes.c | 148 ++++++++++++++++++++++++----------
> include/linux/anon_inodes.h | 13 +++
> include/linux/lsm_hook_defs.h | 2 +
> include/linux/lsm_hooks.h | 7 ++
> include/linux/security.h | 3 +
> security/security.c | 9 +++
> 6 files changed, 141 insertions(+), 41 deletions(-)
>
> diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
> index 89714308c25b..2aa8b57be895 100644
> --- a/fs/anon_inodes.c
> +++ b/fs/anon_inodes.c
> @@ -55,61 +55,78 @@ 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 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);
Weird indentation here. The call to security_inode_init_security_anon() fits on
one line.
> + if (error) {
> + iput(inode);
> + return ERR_PTR(error);
> + }
> + return inode;
> +}
> +
> +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 (IS_ERR(anon_inode_inode))
> - return ERR_PTR(-ENODEV);
> + if (secure) {
> + inode = anon_inode_make_secure_inode(
> + name, context_inode);
Likewise here. The call to anon_inode_make_secure_inode() fits on one line.
> + if (IS_ERR(inode))
> + return ERR_CAST(inode);
> + } else {
> + inode = anon_inode_inode;
> + if (IS_ERR(inode))
> + return ERR_PTR(-ENODEV);
> + /*
> + * We know the anon_inode inode count is always
> + * greater than zero, so ihold() is safe.
> + */
> + ihold(inode);
> + }
>
> - if (fops->owner && !try_module_get(fops->owner))
> - return ERR_PTR(-ENOENT);
> + if (fops->owner && !try_module_get(fops->owner)) {
> + file = ERR_PTR(-ENOENT);
> + goto err;
> + }
The error path here does module_put(fops->owner), even though a reference wasn't
acquired.
> +
> +/**
> + * 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_getfile() will use the same singleton inode, reducing
This should say anon_inode_getfd(), not anon_inode_getfile().
> +/**
> + * 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);
> +}
Weird indentation here again. The call to _anon_inode_getfd() fits on one line.
> @@ -162,4 +229,3 @@ static int __init anon_inode_init(void)
> }
>
> fs_initcall(anon_inode_init);
> -
Unnecessary whitespace change.
> diff --git a/include/linux/anon_inodes.h b/include/linux/anon_inodes.h
> index d0d7d96261ad..67bd85d92dca 100644
> --- a/include/linux/anon_inodes.h
> +++ b/include/linux/anon_inodes.h
> @@ -10,12 +10,25 @@
> #define _LINUX_ANON_INODES_H
>
> struct file_operations;
> +struct inode;
> +
> +struct file *anon_inode_getfile_secure(const char *name,
> + const struct file_operations *fops,
> + void *priv, int flags,
> + const struct inode *context_inode);
This function isn't defined anywhere.
> + * @inode_init_security_anon:
> + * Set up a secure anonymous inode.
> + * @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.
Is there a better name for this than "secure anonymous inode"?
(What is meant by "secure"?)
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 0a0a03b36a3b..95c133a8f8bb 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);
This patch doesn't compile when !CONFIG_SECURITY because this file is missing a
!CONFIG_SECURITY stub for security_inode_init_security_anon().
> diff --git a/security/security.c b/security/security.c
> index 70a7ad357bc6..149b3f024e2d 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1057,6 +1057,15 @@ 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);
> +}
Nit: everything else in this file has 'int' on the same line as the function
name.
- Eric
^ permalink raw reply
* Re: [PATCH bpf-next v9 4/7] bpf: Split bpf_local_storage to bpf_sk_storage
From: Martin KaFai Lau @ 2020-08-25 0:54 UTC (permalink / raw)
To: KP Singh
Cc: linux-kernel, bpf, linux-security-module, Alexei Starovoitov,
Daniel Borkmann, Paul Turner, Jann Horn, Florent Revest
In-Reply-To: <20200823165612.404892-5-kpsingh@chromium.org>
On Sun, Aug 23, 2020 at 06:56:09PM +0200, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
>
> A purely mechanical change:
>
> bpf_sk_storage.c = bpf_sk_storage.c + bpf_local_storage.c
> bpf_sk_storage.h = bpf_sk_storage.h + bpf_local_storage.h
>
> Signed-off-by: KP Singh <kpsingh@google.com>
Acked-by: Martin KaFai Lau <kafai@fb.com>
^ permalink raw reply
* Re: [PATCH bpf-next v9 3/7] bpf: Generalize bpf_sk_storage
From: Martin KaFai Lau @ 2020-08-25 0:53 UTC (permalink / raw)
To: KP Singh
Cc: linux-kernel, bpf, linux-security-module, Alexei Starovoitov,
Daniel Borkmann, Paul Turner, Jann Horn, Florent Revest
In-Reply-To: <20200823165612.404892-4-kpsingh@chromium.org>
On Sun, Aug 23, 2020 at 06:56:08PM +0200, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
>
> Refactor the functionality in bpf_sk_storage.c so that concept of
> storage linked to kernel objects can be extended to other objects like
> inode, task_struct etc.
>
> Each new local storage will still be a separate map and provide its own
> set of helpers. This allows for future object specific extensions and
> still share a lot of the underlying implementation.
>
> This includes the changes suggested by Martin in:
>
> https://lore.kernel.org/bpf/20200725013047.4006241-1-kafai@fb.com/
>
> adding new map operations to support bpf_local_storage maps:
>
> * storages for different kernel objects to optionally have different
> memory charging strategy (map_local_storage_charge,
> map_local_storage_uncharge)
> * Functionality to extract the storage pointer from a pointer to the
> owning object (map_owner_storage_ptr)
>
> Co-developed-by: Martin KaFai Lau <kafai@fb.com>
> Signed-off-by: KP Singh <kpsingh@google.com>
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
^ permalink raw reply
* Re: [PATCH bpf-next v9 5/7] bpf: Implement bpf_local_storage for inodes
From: Martin KaFai Lau @ 2020-08-25 0:52 UTC (permalink / raw)
To: KP Singh
Cc: linux-kernel, bpf, linux-security-module, Alexei Starovoitov,
Daniel Borkmann, Paul Turner, Jann Horn, Florent Revest
In-Reply-To: <20200823165612.404892-6-kpsingh@chromium.org>
On Sun, Aug 23, 2020 at 06:56:10PM +0200, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
>
> Similar to bpf_local_storage for sockets, add local storage for inodes.
> The life-cycle of storage is managed with the life-cycle of the inode.
> i.e. the storage is destroyed along with the owning inode.
>
> The BPF LSM allocates an __rcu pointer to the bpf_local_storage in the
> security blob which are now stackable and can co-exist with other LSMs.
>
[ ... ]
> diff --git a/kernel/bpf/bpf_inode_storage.c b/kernel/bpf/bpf_inode_storage.c
> new file mode 100644
> index 000000000000..b0b283c224c1
> --- /dev/null
> +++ b/kernel/bpf/bpf_inode_storage.c
> @@ -0,0 +1,265 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019 Facebook
> + * Copyright 2020 Google LLC.
> + */
> +
> +#include <linux/rculist.h>
> +#include <linux/list.h>
> +#include <linux/hash.h>
> +#include <linux/types.h>
> +#include <linux/spinlock.h>
> +#include <linux/bpf.h>
> +#include <linux/bpf_local_storage.h>
> +#include <net/sock.h>
> +#include <uapi/linux/sock_diag.h>
> +#include <uapi/linux/btf.h>
> +#include <linux/bpf_lsm.h>
> +#include <linux/btf_ids.h>
> +#include <linux/fdtable.h>
> +
> +DEFINE_BPF_STORAGE_CACHE(inode_cache);
> +
> +static struct bpf_local_storage __rcu **
> +inode_storage_ptr(void *owner)
> +{
> + struct inode *inode = owner;
> + struct bpf_storage_blob *bsb;
> +
> + bsb = bpf_inode(inode);
> + if (!bsb)
> + return NULL;
just noticed this one. NULL could be returned here. When will it happen?
> + return &bsb->storage;
> +}
> +
> +static struct bpf_local_storage_data *inode_storage_lookup(struct inode *inode,
> + struct bpf_map *map,
> + bool cacheit_lockit)
> +{
> + struct bpf_local_storage *inode_storage;
> + struct bpf_local_storage_map *smap;
> + struct bpf_storage_blob *bsb;
> +
> + bsb = bpf_inode(inode);
> + if (!bsb)
> + return NULL;
lookup is fine since NULL is checked here.
> +
> + inode_storage = rcu_dereference(bsb->storage);
> + if (!inode_storage)
> + return NULL;
> +
> + smap = (struct bpf_local_storage_map *)map;
> + return bpf_local_storage_lookup(inode_storage, smap, cacheit_lockit);
> +}
> +
[ ... ]
> +static int bpf_fd_inode_storage_update_elem(struct bpf_map *map, void *key,
> + void *value, u64 map_flags)
> +{
> + struct bpf_local_storage_data *sdata;
> + struct file *f;
> + int fd;
> +
> + fd = *(int *)key;
> + f = fget_raw(fd);
> + if (!f)
> + return -EBADF;
> +
> + sdata = bpf_local_storage_update(f->f_inode,
This will be an issue. bpf_local_storage_update() will not check NULL
returned by inode_storage_ptr(). It should be checked here in the inode code
path first before calling the bpf_local_storage_update() since
this case is specific to inode local storage.
Same for the other bpf_local_storage_update() cases.
> + (struct bpf_local_storage_map *)map,
> + value, map_flags);
> + fput(f);
> + return PTR_ERR_OR_ZERO(sdata);
> +}
> +
[ ... ]
> +BPF_CALL_4(bpf_inode_storage_get, struct bpf_map *, map, struct inode *, inode,
> + void *, value, u64, flags)
> +{
> + struct bpf_local_storage_data *sdata;
> +
> + if (flags & ~(BPF_LOCAL_STORAGE_GET_F_CREATE))
> + return (unsigned long)NULL;
> +
> + sdata = inode_storage_lookup(inode, map, true);
> + if (sdata)
> + return (unsigned long)sdata->data;
> +
> + /* This helper must only called from where the inode is gurranteed
> + * to have a refcount and cannot be freed.
> + */
> + if (flags & BPF_LOCAL_STORAGE_GET_F_CREATE) {
> + sdata = bpf_local_storage_update(
> + inode, (struct bpf_local_storage_map *)map, value,
> + BPF_NOEXIST);
> + return IS_ERR(sdata) ? (unsigned long)NULL :
> + (unsigned long)sdata->data;
> + }
> +
> + return (unsigned long)NULL;
> +}
> +
> diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
> index 32d32d485451..35f9b19259e5 100644
> --- a/security/bpf/hooks.c
> +++ b/security/bpf/hooks.c
> @@ -3,6 +3,7 @@
> /*
> * Copyright (C) 2020 Google LLC.
> */
> +#include <linux/bpf_local_storage.h>
Is it needed?
> #include <linux/lsm_hooks.h>
> #include <linux/bpf_lsm.h>
>
> @@ -11,6 +12,7 @@ static struct security_hook_list bpf_lsm_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(NAME, bpf_lsm_##NAME),
> #include <linux/lsm_hook_defs.h>
> #undef LSM_HOOK
> + LSM_HOOK_INIT(inode_free_security, bpf_inode_storage_free),
> };
>
> static int __init bpf_lsm_init(void)
> @@ -20,7 +22,12 @@ static int __init bpf_lsm_init(void)
> return 0;
> }
>
> +struct lsm_blob_sizes bpf_lsm_blob_sizes __lsm_ro_after_init = {
> + .lbs_inode = sizeof(struct bpf_storage_blob),
> +};
> +
> DEFINE_LSM(bpf) = {
> .name = "bpf",
> .init = bpf_lsm_init,
> + .blobs = &bpf_lsm_blob_sizes
> };
^ permalink raw reply
* Re: general protection fault in security_inode_getattr
From: Yonghong Song @ 2020-08-25 0:48 UTC (permalink / raw)
To: syzbot, andriin, ast, bpf, daniel, jmorris, john.fastabend, kafai,
kpsingh, linux-fsdevel, linux-kernel, linux-security-module,
linux-unionfs, miklos, netdev, omosnace, serge, songliubraving,
syzkaller-bugs
In-Reply-To: <00000000000020e49105ada8d598@google.com>
On 8/24/20 5:32 PM, syzbot wrote:
> syzbot has bisected this issue to:
>
> commit 35697c12d7ffd31a56d3c9604066a166b75d0169
> Author: Yonghong Song <yhs@fb.com>
> Date: Thu Jan 16 17:40:04 2020 +0000
>
> selftests/bpf: Fix test_progs send_signal flakiness with nmi mode
The above patch changed file:
tools/testing/selftests/bpf/prog_tests/send_signal.c
It is unlikely it caused the issue.
>
> bisection log: https://urldefense.proofpoint.com/v2/url?u=https-3A__syzkaller.appspot.com_x_bisect.txt-3Fx-3D13032139900000&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=854z77F3pbTpgGUCFUwM1OSEUj4NbiNIZsaad0Xg4qc&s=Le6D_jfzkec28_qNhbUwMesaUeBGaKEG6RLN3ZCzE1s&e=
> start commit: d012a719 Linux 5.9-rc2
> git tree: upstream
> final oops: https://urldefense.proofpoint.com/v2/url?u=https-3A__syzkaller.appspot.com_x_report.txt-3Fx-3D10832139900000&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=854z77F3pbTpgGUCFUwM1OSEUj4NbiNIZsaad0Xg4qc&s=VbLXb22TgxAtiPQTEq0t5r0WgNDiFwstWKnme0tWBLE&e=
> console output: https://urldefense.proofpoint.com/v2/url?u=https-3A__syzkaller.appspot.com_x_log.txt-3Fx-3D17032139900000&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=854z77F3pbTpgGUCFUwM1OSEUj4NbiNIZsaad0Xg4qc&s=yu72HnqjHzOTtJ5dyD7q0QW9sOwEO6pPHjeYTTutKdc&e=
> kernel config: https://urldefense.proofpoint.com/v2/url?u=https-3A__syzkaller.appspot.com_x_.config-3Fx-3D891ca5711a9f1650&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=854z77F3pbTpgGUCFUwM1OSEUj4NbiNIZsaad0Xg4qc&s=6cylIRBZjHQmgkJQoofuLN2XSifb-13qrrj58PQpBYs&e=
> dashboard link: https://urldefense.proofpoint.com/v2/url?u=https-3A__syzkaller.appspot.com_bug-3Fextid-3Df07cc9be8d1d226947ed&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=854z77F3pbTpgGUCFUwM1OSEUj4NbiNIZsaad0Xg4qc&s=siCrm2aO-fzR3Nym_zaPQQnmxMlJo0bRj87zgm7o5mY&e=
> syz repro: https://urldefense.proofpoint.com/v2/url?u=https-3A__syzkaller.appspot.com_x_repro.syz-3Fx-3D104a650e900000&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=854z77F3pbTpgGUCFUwM1OSEUj4NbiNIZsaad0Xg4qc&s=yNEvyRe2bO4AKgq2UuJc32katp4k4UGKwMUDEBlhM7E&e=
>
> Reported-by: syzbot+f07cc9be8d1d226947ed@syzkaller.appspotmail.com
> Fixes: 35697c12d7ff ("selftests/bpf: Fix test_progs send_signal flakiness with nmi mode")
>
> For information about bisection process see: https://urldefense.proofpoint.com/v2/url?u=https-3A__goo.gl_tpsmEJ-23bisection&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=854z77F3pbTpgGUCFUwM1OSEUj4NbiNIZsaad0Xg4qc&s=K4KdZK8rBgxDv4M9uwEndkCB4mCatTH-opwefN-0b-M&e=
>
^ permalink raw reply
* Re: general protection fault in security_inode_getattr
From: syzbot @ 2020-08-25 0:32 UTC (permalink / raw)
To: andriin, ast, bpf, daniel, jmorris, john.fastabend, kafai,
kpsingh, linux-fsdevel, linux-kernel, linux-security-module,
linux-unionfs, miklos, netdev, omosnace, serge, songliubraving,
syzkaller-bugs, yhs
In-Reply-To: <0000000000008caae305ab9a5318@google.com>
syzbot has bisected this issue to:
commit 35697c12d7ffd31a56d3c9604066a166b75d0169
Author: Yonghong Song <yhs@fb.com>
Date: Thu Jan 16 17:40:04 2020 +0000
selftests/bpf: Fix test_progs send_signal flakiness with nmi mode
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=13032139900000
start commit: d012a719 Linux 5.9-rc2
git tree: upstream
final oops: https://syzkaller.appspot.com/x/report.txt?x=10832139900000
console output: https://syzkaller.appspot.com/x/log.txt?x=17032139900000
kernel config: https://syzkaller.appspot.com/x/.config?x=891ca5711a9f1650
dashboard link: https://syzkaller.appspot.com/bug?extid=f07cc9be8d1d226947ed
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=104a650e900000
Reported-by: syzbot+f07cc9be8d1d226947ed@syzkaller.appspotmail.com
Fixes: 35697c12d7ff ("selftests/bpf: Fix test_progs send_signal flakiness with nmi mode")
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
^ permalink raw reply
* Re: [PATCH v2] block: grant IOPRIO_CLASS_RT to CAP_SYS_NICE
From: Serge E. Hallyn @ 2020-08-24 23:51 UTC (permalink / raw)
To: Khazhismel Kumykov
Cc: Jens Axboe, Serge Hallyn, Paolo Valente, Bart Van Assche,
linux-block, linux-kernel, linux-security-module
In-Reply-To: <20200824221034.2170308-1-khazhy@google.com>
On Mon, Aug 24, 2020 at 03:10:34PM -0700, Khazhismel Kumykov wrote:
> CAP_SYS_ADMIN is too broad, and ionice fits into CAP_SYS_NICE's grouping.
>
> Retain CAP_SYS_ADMIN permission for backwards compatibility.
>
> Signed-off-by: Khazhismel Kumykov <khazhy@google.com>
Acked-by: Serge Hallyn <serge@hallyn.com>
> ---
> block/ioprio.c | 2 +-
> include/uapi/linux/capability.h | 2 ++
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> v2: fix embarrassing logic mistake
> diff --git a/block/ioprio.c b/block/ioprio.c
> index 77bcab11dce5..276496246fe9 100644
> --- a/block/ioprio.c
> +++ b/block/ioprio.c
> @@ -69,7 +69,7 @@ int ioprio_check_cap(int ioprio)
>
> switch (class) {
> case IOPRIO_CLASS_RT:
> - if (!capable(CAP_SYS_ADMIN))
> + if (!capable(CAP_SYS_NICE) && !capable(CAP_SYS_ADMIN))
> return -EPERM;
> /* fall through */
> /* rt has prio field too */
> diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
> index 395dd0df8d08..c6ca33034147 100644
> --- a/include/uapi/linux/capability.h
> +++ b/include/uapi/linux/capability.h
> @@ -288,6 +288,8 @@ struct vfs_ns_cap_data {
> processes and setting the scheduling algorithm used by another
> process. */
> /* Allow setting cpu affinity on other processes */
> +/* Allow setting realtime ioprio class */
> +/* Allow setting ioprio class on other processes */
>
> #define CAP_SYS_NICE 23
>
> --
> 2.28.0.297.g1956fa8f8d-goog
^ permalink raw reply
* Re: [net PATCH] netlabel: fix problems with mapping removal
From: David Miller @ 2020-08-24 23:08 UTC (permalink / raw)
To: paul; +Cc: netdev, linux-security-module, selinux, stephen.smalley.work
In-Reply-To: <159804209207.16190.14955035148979265114.stgit@sifl>
From: Paul Moore <paul@paul-moore.com>
Date: Fri, 21 Aug 2020 16:34:52 -0400
> This patch fixes two main problems seen when removing NetLabel
> mappings: memory leaks and potentially extra audit noise.
>
> The memory leaks are caused by not properly free'ing the mapping's
> address selector struct when free'ing the entire entry as well as
> not properly cleaning up a temporary mapping entry when adding new
> address selectors to an existing entry. This patch fixes both these
> problems such that kmemleak reports no NetLabel associated leaks
> after running the SELinux test suite.
>
> The potentially extra audit noise was caused by the auditing code in
> netlbl_domhsh_remove_entry() being called regardless of the entry's
> validity. If another thread had already marked the entry as invalid,
> but not removed/free'd it from the list of mappings, then it was
> possible that an additional mapping removal audit record would be
> generated. This patch fixes this by returning early from the removal
> function when the entry was previously marked invalid. This change
> also had the side benefit of improving the code by decreasing the
> indentation level of large chunk of code by one (accounting for most
> of the diffstat).
>
> Fixes: 63c416887437 ("netlabel: Add network address selectors to the NetLabel/LSM domain mapping")
> Reported-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
Applied and queued up for -stable, thanks Paul.
^ permalink raw reply
* Re: [PATCH v2 2/3] IMA: add policy to support measuring critical data from kernel components
From: Mimi Zohar @ 2020-08-24 22:46 UTC (permalink / raw)
To: Tushar Sugandhi, stephen.smalley.work, casey, agk, snitzer,
gmazyland
Cc: tyhicks, sashal, jmorris, nramas, linux-integrity, selinux,
linux-security-module, linux-kernel, dm-devel
In-Reply-To: <20200821182107.5328-3-tusharsu@linux.microsoft.com>
On Fri, 2020-08-21 at 11:21 -0700, Tushar Sugandhi wrote:
> There would be several candidate kernel components suitable for IMA
> measurement. Not all of them would have support for IMA measurement.
> Also, system administrators may not want to measure data for all of
> them, even when they support IMA measurement. An IMA policy specific
> to various kernel components is needed to measure their respective
> critical data.
>
> Add a new IMA policy CRITICAL_DATA+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 | 6 ++-
> security/integrity/ima/ima.h | 1 +
> security/integrity/ima/ima_api.c | 2 +-
> security/integrity/ima/ima_policy.c | 62 +++++++++++++++++++++++++---
> 4 files changed, 63 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> index cd572912c593..a0dd0f108555 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
> @@ -125,3 +125,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 data_sources=selinux|apparmor|dm-crypt
This example uses "data_sources" without first defining it in the
"option:" section. Defining two new options is an indication that this
patch should be split up. One which defines the "CRITICAL_DATA" and
another one which defines the new key value pair. The term
"data_sources" is pretty generic. Perhaps constrain it a bit by re-
naming it "critical_data=". Or was such using a generic name
intentional?
Normally "CRITICAL_DATA" would be defined with the critical data hook,
but that seems to be defined in patch 3/3 "IMA: define IMA hook to
measure critical data from kernel components".
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 8875085db689..0f4209a92bfb 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_policy.c b/security/integrity/ima/ima_policy.c
> index 8866e84d0062..7b649095ac7a 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;
> };
>
> @@ -508,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;
> @@ -911,7 +922,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 +959,7 @@ static const match_table_t policy_tokens = {
> {Opt_pcr, "pcr=%s"},
> {Opt_template, "template=%s"},
> {Opt_keyrings, "keyrings=%s"},
> + {Opt_data_sources, "data_sources=%s"},
> {Opt_err, NULL}
> };
>
> @@ -1110,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;
Requiring IMA_FUNC and IMA_DATA_SOURCES makes sense, but why are
IMA_UID and IMA_PCR required?
> +
> + if (ima_rule_contains_lsm_cond(entry))
> + return false;
> +
> break;
> default:
> return false;
> @@ -1242,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)
> @@ -1312,6 +1339,23 @@ 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, "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;
> + }
> +
"keyrings=" isn't bounded because keyrings can be created by userspace.
Perhaps keyring names has a minimum/maximum length. IMA isn't
measuring userspace construsts. Shouldn't the list of critical data
being measured be bounded and verified?
Mimi
> + entry->flags |= IMA_DATA_SOURCES;
> + break;
> case Opt_fsuuid:
> ima_log_string(ab, "fsuuid", args[0].from);
>
> @@ -1692,6 +1736,12 @@ int ima_policy_show(struct seq_file *m, void *v)
> seq_puts(m, " ");
> }
>
> + if (entry->flags & IMA_DATA_SOURCES) {
> + seq_puts(m, "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);
^ permalink raw reply
* Re: [PATCH] SELinux: Measure state and hash of policy using IMA
From: Paul Moore @ 2020-08-24 22:18 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: Ondrej Mosnacek, Stephen Smalley, Mimi Zohar, Casey Schaufler,
Tyler Hicks, tusharsu, Sasha Levin, James Morris, linux-integrity,
SElinux list, LSM List, linux-kernel
In-Reply-To: <f041e8ee-3955-9551-b72d-d4d7fa6e636d@linux.microsoft.com>
On Mon, Aug 24, 2020 at 5:29 PM Lakshmi Ramasubramanian
<nramas@linux.microsoft.com> wrote:
> On 8/24/20 1:01 PM, Ondrej Mosnacek wrote:
> > On Mon, Aug 24, 2020 at 9:30 PM Stephen Smalley
> > <stephen.smalley.work@gmail.com> wrote:
> >> On Mon, Aug 24, 2020 at 2:13 PM Lakshmi Ramasubramanian
> >> <nramas@linux.microsoft.com> wrote:
> >>> On 8/24/20 7:00 AM, Stephen Smalley wrote:
...
> >>> Is Ondrej's re-try approach I need to use to workaround policy reload issue?
> >>
> >> No, I think perhaps we should move the mutex to selinux_state instead
> >> of selinux_fs_info. selinux_fs_info has a pointer to selinux_state so
> >> it can then use it indirectly. Note that your patches are going to
> >> conflict with other ongoing work in the selinux next branch that is
> >> refactoring policy load and converting the policy rwlock to RCU.
> >
> > Yeah, and I'm experimenting with a patch on top of Stephen's RCU work
> > that would allow you to do this in a straightforward way without even
> > messing with the fsi->mutex. My patch may or may not be eventually
> > committed, but either way I'd recommend holding off on this for a
> > while until the dust settles around the RCU conversion.
>
> I can make the SELinux\IMA changes in "selinux next branch" taking
> dependencies on Stephen's patches + relevant IMA patches.
I know it can be frustrating to hear what I'm about to say, but the
best option is probably just to wait a little to let things settle in
the SELinux -next branch. There is a lot of stuff going on right now
with patches flooding in (at least "flooding" from a SELinux kernel
development perspective) and we/I've haven't gotten through all of
them yet.
> Could you please let me know the URL to the "selinux next branch"?
git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git next
--
paul moore
www.paul-moore.com
^ permalink raw reply
* [PATCH v2] block: grant IOPRIO_CLASS_RT to CAP_SYS_NICE
From: Khazhismel Kumykov @ 2020-08-24 22:10 UTC (permalink / raw)
To: Jens Axboe
Cc: Serge Hallyn, Paolo Valente, Bart Van Assche, linux-block,
linux-kernel, linux-security-module, Khazhismel Kumykov
CAP_SYS_ADMIN is too broad, and ionice fits into CAP_SYS_NICE's grouping.
Retain CAP_SYS_ADMIN permission for backwards compatibility.
Signed-off-by: Khazhismel Kumykov <khazhy@google.com>
---
block/ioprio.c | 2 +-
include/uapi/linux/capability.h | 2 ++
2 files changed, 3 insertions(+), 1 deletion(-)
v2: fix embarrassing logic mistake
diff --git a/block/ioprio.c b/block/ioprio.c
index 77bcab11dce5..276496246fe9 100644
--- a/block/ioprio.c
+++ b/block/ioprio.c
@@ -69,7 +69,7 @@ int ioprio_check_cap(int ioprio)
switch (class) {
case IOPRIO_CLASS_RT:
- if (!capable(CAP_SYS_ADMIN))
+ if (!capable(CAP_SYS_NICE) && !capable(CAP_SYS_ADMIN))
return -EPERM;
/* fall through */
/* rt has prio field too */
diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
index 395dd0df8d08..c6ca33034147 100644
--- a/include/uapi/linux/capability.h
+++ b/include/uapi/linux/capability.h
@@ -288,6 +288,8 @@ struct vfs_ns_cap_data {
processes and setting the scheduling algorithm used by another
process. */
/* Allow setting cpu affinity on other processes */
+/* Allow setting realtime ioprio class */
+/* Allow setting ioprio class on other processes */
#define CAP_SYS_NICE 23
--
2.28.0.297.g1956fa8f8d-goog
^ permalink raw reply related
* Re: [PATCH] SELinux: Measure state and hash of policy using IMA
From: Lakshmi Ramasubramanian @ 2020-08-24 21:29 UTC (permalink / raw)
To: Ondrej Mosnacek, Stephen Smalley
Cc: Mimi Zohar, Casey Schaufler, Tyler Hicks, tusharsu, Sasha Levin,
James Morris, linux-integrity, SElinux list, LSM List,
linux-kernel
In-Reply-To: <CAFqZXNvVQ5U6Ea3gT32Z0hfWbu7GPR-mTF2z6-JZZJT57Heuuw@mail.gmail.com>
On 8/24/20 1:01 PM, Ondrej Mosnacek wrote:
> On Mon, Aug 24, 2020 at 9:30 PM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
>> On Mon, Aug 24, 2020 at 2:13 PM Lakshmi Ramasubramanian
>> <nramas@linux.microsoft.com> wrote:
>>>
>>> On 8/24/20 7:00 AM, Stephen Smalley wrote:
>>>
>>>>> +int security_read_policy_kernel(struct selinux_state *state,
>>>>> + void **data, size_t *len)
>>>>> +{
>>>>> + int rc;
>>>>> +
>>>>> + rc = security_read_policy_len(state, len);
>>>>> + if (rc)
>>>>> + return rc;
>>>>> +
>>>>> + *data = vmalloc(*len);
>>>>> + if (!*data)
>>>>> + return -ENOMEM;
>>>>>
>>>>> + return security_read_selinux_policy(state, data, len);
>>>>> }
>>>>
>>>> See the discussion here:
>>>> https://lore.kernel.org/selinux/20200824113015.1375857-1-omosnace@redhat.com/T/#t
>>>>
>>>> In order for this to be safe, you need to ensure that all callers of
>>>> security_read_policy_kernel() have taken fsi->mutex in selinuxfs and
>>>> any use of security_read_policy_len() occurs while holding the mutex.
>>>> Otherwise, the length can change between security_read_policy_len()
>>>> and security_read_selinux_policy() if policy is reloaded.
>>>>
>>>
>>> "struct selinux_fs_info" is available when calling
>>> security_read_policy_kernel() - currently in measure.c.
>>> Only "struct selinux_state" is.
>>>
>>> Is Ondrej's re-try approach I need to use to workaround policy reload issue?
>>
>> No, I think perhaps we should move the mutex to selinux_state instead
>> of selinux_fs_info. selinux_fs_info has a pointer to selinux_state so
>> it can then use it indirectly. Note that your patches are going to
>> conflict with other ongoing work in the selinux next branch that is
>> refactoring policy load and converting the policy rwlock to RCU.
>
> Yeah, and I'm experimenting with a patch on top of Stephen's RCU work
> that would allow you to do this in a straightforward way without even
> messing with the fsi->mutex. My patch may or may not be eventually
> committed, but either way I'd recommend holding off on this for a
> while until the dust settles around the RCU conversion.
>
I can make the SELinux\IMA changes in "selinux next branch" taking
dependencies on Stephen's patches + relevant IMA patches.
Could you please let me know the URL to the "selinux next branch"?
thanks,
-lakshmi
^ permalink raw reply
* [PATCH] ima: use kmemdup() rather than kmalloc+memcpy
From: Alex Dewar @ 2020-08-24 21:21 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin, James Morris, Serge E. Hallyn,
linux-integrity, linux-security-module, linux-kernel
Cc: Alex Dewar
Issue identified with Coccinelle.
Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
---
security/integrity/ima/ima_policy.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index b4de33074b37..ea917a787d9d 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -284,7 +284,7 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
struct ima_rule_entry *nentry;
int i;
- nentry = kmalloc(sizeof(*nentry), GFP_KERNEL);
+ nentry = kmemdup(entry, sizeof(*nentry), GFP_KERNEL);
if (!nentry)
return NULL;
@@ -292,7 +292,6 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
* Immutable elements are copied over as pointers and data; only
* lsm rules can change
*/
- memcpy(nentry, entry, sizeof(*nentry));
memset(nentry->lsm, 0, sizeof_field(struct ima_rule_entry, lsm));
for (i = 0; i < MAX_LSM_RULES; i++) {
--
2.28.0
^ permalink raw reply related
* Re: general protection fault in security_inode_getattr
From: Ondrej Mosnacek @ 2020-08-24 21:00 UTC (permalink / raw)
To: syzbot
Cc: andriin, Alexei Starovoitov, bpf, Daniel Borkmann, James Morris,
john.fastabend, kafai, KP Singh, Linux kernel mailing list,
Linux Security Module list, network dev, Serge E. Hallyn,
songliubraving, syzkaller-bugs, yhs, linux-fsdevel,
Miklos Szeredi, linux-unionfs
In-Reply-To: <000000000000a726a405ada4b6cf@google.com>
On Mon, Aug 24, 2020 at 9:37 PM syzbot
<syzbot+f07cc9be8d1d226947ed@syzkaller.appspotmail.com> wrote:
> syzbot has found a reproducer for the following issue on:
Looping in fsdevel and OverlayFS maintainers, as this seems to be
FS/OverlayFS related...
See also original report against 5.8-rc7:
https://lore.kernel.org/linux-security-module/0000000000008caae305ab9a5318@google.com/T/
>
> HEAD commit: d012a719 Linux 5.9-rc2
> git tree: upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=14aa130e900000
> kernel config: https://syzkaller.appspot.com/x/.config?x=891ca5711a9f1650
> dashboard link: https://syzkaller.appspot.com/bug?extid=f07cc9be8d1d226947ed
> compiler: clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=104a650e900000
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+f07cc9be8d1d226947ed@syzkaller.appspotmail.com
>
> general protection fault, probably for non-canonical address 0xdffffc000000000d: 0000 [#1] PREEMPT SMP KASAN
> KASAN: null-ptr-deref in range [0x0000000000000068-0x000000000000006f]
> CPU: 1 PID: 32288 Comm: syz-executor.3 Not tainted 5.9.0-rc2-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> RIP: 0010:d_backing_inode include/linux/dcache.h:549 [inline]
> RIP: 0010:security_inode_getattr+0x42/0x140 security/security.c:1276
> Code: 1b fe 49 8d 5e 08 48 89 d8 48 c1 e8 03 42 80 3c 38 00 74 08 48 89 df e8 bc b4 5b fe 48 8b 1b 48 83 c3 68 48 89 d8 48 c1 e8 03 <42> 80 3c 38 00 74 08 48 89 df e8 9f b4 5b fe 48 8b 1b 48 83 c3 0c
> RSP: 0018:ffffc9000a017750 EFLAGS: 00010202
> RAX: 000000000000000d RBX: 0000000000000068 RCX: ffff888093ec6180
> RDX: 0000000000000000 RSI: ffffc9000a017860 RDI: ffffc9000a017850
> RBP: ffffc9000a017850 R08: dffffc0000000000 R09: ffffc9000a017850
> R10: fffff52001402f0c R11: 0000000000000000 R12: ffffc9000a017860
> R13: 0000000000008401 R14: ffffc9000a017850 R15: dffffc0000000000
> FS: 00007f292d4ef700(0000) GS:ffff8880ae900000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000001b30920074 CR3: 00000000937fd000 CR4: 00000000001506e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> vfs_getattr+0x21/0x60 fs/stat.c:121
> ovl_copy_up_one fs/overlayfs/copy_up.c:850 [inline]
> ovl_copy_up_flags+0x2ef/0x2a00 fs/overlayfs/copy_up.c:931
> ovl_maybe_copy_up+0x154/0x180 fs/overlayfs/copy_up.c:963
> ovl_open+0xa2/0x200 fs/overlayfs/file.c:147
> do_dentry_open+0x7c8/0x1010 fs/open.c:817
> do_open fs/namei.c:3251 [inline]
> path_openat+0x2794/0x3840 fs/namei.c:3368
> do_filp_open+0x191/0x3a0 fs/namei.c:3395
> file_open_name+0x321/0x430 fs/open.c:1113
> acct_on kernel/acct.c:207 [inline]
> __do_sys_acct kernel/acct.c:286 [inline]
> __se_sys_acct+0x122/0x6f0 kernel/acct.c:273
> do_syscall_64+0x31/0x70 arch/x86/entry/common.c:46
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
> RIP: 0033:0x45d579
> Code: 5d b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 2b b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00
> RSP: 002b:00007f292d4eec78 EFLAGS: 00000246 ORIG_RAX: 00000000000000a3
> RAX: ffffffffffffffda RBX: 0000000000000700 RCX: 000000000045d579
> RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000020000040
> RBP: 000000000118cf70 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 000000000118cf4c
> R13: 00007ffc8e04bc4f R14: 00007f292d4ef9c0 R15: 000000000118cf4c
> Modules linked in:
> ---[ end trace 7e4f1041b188e411 ]---
> RIP: 0010:d_backing_inode include/linux/dcache.h:549 [inline]
> RIP: 0010:security_inode_getattr+0x42/0x140 security/security.c:1276
> Code: 1b fe 49 8d 5e 08 48 89 d8 48 c1 e8 03 42 80 3c 38 00 74 08 48 89 df e8 bc b4 5b fe 48 8b 1b 48 83 c3 68 48 89 d8 48 c1 e8 03 <42> 80 3c 38 00 74 08 48 89 df e8 9f b4 5b fe 48 8b 1b 48 83 c3 0c
> RSP: 0018:ffffc9000a017750 EFLAGS: 00010202
> RAX: 000000000000000d RBX: 0000000000000068 RCX: ffff888093ec6180
> RDX: 0000000000000000 RSI: ffffc9000a017860 RDI: ffffc9000a017850
> RBP: ffffc9000a017850 R08: dffffc0000000000 R09: ffffc9000a017850
> R10: fffff52001402f0c R11: 0000000000000000 R12: ffffc9000a017860
> R13: 0000000000008401 R14: ffffc9000a017850 R15: dffffc0000000000
> FS: 00007f292d4ef700(0000) GS:ffff8880ae900000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007fef820e7000 CR3: 00000000937fd000 CR4: 00000000001506e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>
--
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel
Red Hat, Inc.
^ permalink raw reply
* Re: [PATCH] block: grant IOPRIO_CLASS_RT to CAP_SYS_NICE
From: Khazhismel Kumykov @ 2020-08-24 20:48 UTC (permalink / raw)
To: Jens Axboe
Cc: Serge Hallyn, Paolo Valente, Bart Van Assche, linux-block,
Linux Kernel Mailing List, linux-security-module
In-Reply-To: <20200824204501.1707957-1-khazhy@google.com>
[-- Attachment #1: Type: text/plain, Size: 1557 bytes --]
On Mon, Aug 24, 2020 at 1:45 PM Khazhismel Kumykov <khazhy@google.com> wrote:
>
> CAP_SYS_ADMIN is too broad, and ionice fits into CAP_SYS_NICE's grouping.
>
> Retain CAP_SYS_ADMIN permission for backwards compatibility.
>
> Signed-off-by: Khazhismel Kumykov <khazhy@google.com>
> ---
> block/ioprio.c | 2 +-
> include/uapi/linux/capability.h | 2 ++
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/block/ioprio.c b/block/ioprio.c
> index 77bcab11dce5..4572456430f9 100644
> --- a/block/ioprio.c
> +++ b/block/ioprio.c
> @@ -69,7 +69,7 @@ int ioprio_check_cap(int ioprio)
>
> switch (class) {
> case IOPRIO_CLASS_RT:
> - if (!capable(CAP_SYS_ADMIN))
> + if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_NICE))
yikes, sorry for the spam
> return -EPERM;
> /* fall through */
> /* rt has prio field too */
> diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
> index 395dd0df8d08..c6ca33034147 100644
> --- a/include/uapi/linux/capability.h
> +++ b/include/uapi/linux/capability.h
> @@ -288,6 +288,8 @@ struct vfs_ns_cap_data {
> processes and setting the scheduling algorithm used by another
> process. */
> /* Allow setting cpu affinity on other processes */
> +/* Allow setting realtime ioprio class */
> +/* Allow setting ioprio class on other processes */
>
> #define CAP_SYS_NICE 23
>
> --
> 2.28.0.297.g1956fa8f8d-goog
>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 3850 bytes --]
^ permalink raw reply
* [PATCH] block: grant IOPRIO_CLASS_RT to CAP_SYS_NICE
From: Khazhismel Kumykov @ 2020-08-24 20:45 UTC (permalink / raw)
To: Jens Axboe
Cc: Serge Hallyn, Paolo Valente, Bart Van Assche, linux-block,
linux-kernel, linux-security-module, Khazhismel Kumykov
CAP_SYS_ADMIN is too broad, and ionice fits into CAP_SYS_NICE's grouping.
Retain CAP_SYS_ADMIN permission for backwards compatibility.
Signed-off-by: Khazhismel Kumykov <khazhy@google.com>
---
block/ioprio.c | 2 +-
include/uapi/linux/capability.h | 2 ++
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/block/ioprio.c b/block/ioprio.c
index 77bcab11dce5..4572456430f9 100644
--- a/block/ioprio.c
+++ b/block/ioprio.c
@@ -69,7 +69,7 @@ int ioprio_check_cap(int ioprio)
switch (class) {
case IOPRIO_CLASS_RT:
- if (!capable(CAP_SYS_ADMIN))
+ if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_NICE))
return -EPERM;
/* fall through */
/* rt has prio field too */
diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
index 395dd0df8d08..c6ca33034147 100644
--- a/include/uapi/linux/capability.h
+++ b/include/uapi/linux/capability.h
@@ -288,6 +288,8 @@ struct vfs_ns_cap_data {
processes and setting the scheduling algorithm used by another
process. */
/* Allow setting cpu affinity on other processes */
+/* Allow setting realtime ioprio class */
+/* Allow setting ioprio class on other processes */
#define CAP_SYS_NICE 23
--
2.28.0.297.g1956fa8f8d-goog
^ permalink raw reply related
* Re: [PATCH] integrity: Use current_uid() in integrity_audit_message()
From: Mimi Zohar @ 2020-08-24 20:33 UTC (permalink / raw)
To: Denis Efremov
Cc: James Morris, Serge E. Hallyn, Lakshmi Ramasubramanian,
linux-security-module, linux-kernel
In-Reply-To: <20200824125435.487194-1-efremov@linux.com>
On Mon, 2020-08-24 at 15:54 +0300, Denis Efremov wrote:
> Modify integrity_audit_message() to use current_uid().
>
> Signed-off-by: Denis Efremov <efremov@linux.com>
Thank you. This patch is now queued in next-integrity-testing.
Mimi
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox