* [PATCH v2 4/6] selftests/bpf: Add test for bpf_ima_file_hash()
From: Roberto Sassu @ 2022-02-15 12:40 UTC (permalink / raw)
To: zohar, shuah, ast, daniel, andrii, kpsingh, revest
Cc: linux-integrity, linux-security-module, linux-kselftest, netdev,
bpf, linux-kernel, Roberto Sassu
In-Reply-To: <20220215124042.186506-1-roberto.sassu@huawei.com>
Modify the existing IMA test to call bpf_ima_file_hash() and update the
expected result accordingly.
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
.../selftests/bpf/prog_tests/test_ima.c | 29 ++++++++++++++++---
tools/testing/selftests/bpf/progs/ima.c | 10 +++++--
2 files changed, 33 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/test_ima.c b/tools/testing/selftests/bpf/prog_tests/test_ima.c
index 97d8a6f84f4a..62bf0e830453 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_ima.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_ima.c
@@ -13,9 +13,10 @@
#include "ima.skel.h"
-static int run_measured_process(const char *measured_dir, u32 *monitored_pid)
+static int run_measured_process(const char *measured_dir, u32 *monitored_pid,
+ bool *use_ima_file_hash)
{
- int child_pid, child_status;
+ int err, child_pid, child_status;
child_pid = fork();
if (child_pid == 0) {
@@ -24,6 +25,21 @@ static int run_measured_process(const char *measured_dir, u32 *monitored_pid)
NULL);
exit(errno);
+ } else if (child_pid > 0) {
+ waitpid(child_pid, &child_status, 0);
+ err = WEXITSTATUS(child_status);
+ if (err)
+ return err;
+ }
+
+ child_pid = fork();
+ if (child_pid == 0) {
+ *monitored_pid = getpid();
+ *use_ima_file_hash = true;
+ execlp("./ima_setup.sh", "./ima_setup.sh", "run", measured_dir,
+ NULL);
+ exit(errno);
+
} else if (child_pid > 0) {
waitpid(child_pid, &child_status, 0);
return WEXITSTATUS(child_status);
@@ -72,12 +88,17 @@ void test_test_ima(void)
if (CHECK(err, "failed to run command", "%s, errno = %d\n", cmd, errno))
goto close_clean;
- err = run_measured_process(measured_dir, &skel->bss->monitored_pid);
+ err = run_measured_process(measured_dir, &skel->bss->monitored_pid,
+ &skel->bss->use_ima_file_hash);
if (CHECK(err, "run_measured_process", "err = %d\n", err))
goto close_clean;
err = ring_buffer__consume(ringbuf);
- ASSERT_EQ(err, 1, "num_samples_or_err");
+ /*
+ * 1 sample with use_ima_file_hash = false
+ * 2 samples with use_ima_file_hash = true (./ima_setup.sh, /bin/true)
+ */
+ ASSERT_EQ(err, 3, "num_samples_or_err");
ASSERT_NEQ(ima_hash_from_bpf, 0, "ima_hash");
close_clean:
diff --git a/tools/testing/selftests/bpf/progs/ima.c b/tools/testing/selftests/bpf/progs/ima.c
index 96060ff4ffc6..9bb63f96cfc0 100644
--- a/tools/testing/selftests/bpf/progs/ima.c
+++ b/tools/testing/selftests/bpf/progs/ima.c
@@ -18,6 +18,8 @@ struct {
char _license[] SEC("license") = "GPL";
+bool use_ima_file_hash;
+
SEC("lsm.s/bprm_committed_creds")
void BPF_PROG(ima, struct linux_binprm *bprm)
{
@@ -28,8 +30,12 @@ void BPF_PROG(ima, struct linux_binprm *bprm)
pid = bpf_get_current_pid_tgid() >> 32;
if (pid == monitored_pid) {
- ret = bpf_ima_inode_hash(bprm->file->f_inode, &ima_hash,
- sizeof(ima_hash));
+ if (!use_ima_file_hash)
+ ret = bpf_ima_inode_hash(bprm->file->f_inode, &ima_hash,
+ sizeof(ima_hash));
+ else
+ ret = bpf_ima_file_hash(bprm->file, &ima_hash,
+ sizeof(ima_hash));
if (ret < 0 || ima_hash == 0)
return;
--
2.32.0
^ permalink raw reply related
* [PATCH v2 2/6] ima: Always return a file measurement in ima_file_hash()
From: Roberto Sassu @ 2022-02-15 12:40 UTC (permalink / raw)
To: zohar, shuah, ast, daniel, andrii, kpsingh, revest
Cc: linux-integrity, linux-security-module, linux-kselftest, netdev,
bpf, linux-kernel, Roberto Sassu
In-Reply-To: <20220215124042.186506-1-roberto.sassu@huawei.com>
__ima_inode_hash() checks if a digest has been already calculated by
looking for the integrity_iint_cache structure associated to the passed
inode.
Users of ima_file_hash() (e.g. eBPF) might be interested in obtaining the
information without having to setup an IMA policy so that the digest is
always available at the time they call this function.
Call ima_collect_measurement() in __ima_inode_hash(), if the file
descriptor is available (passed by ima_file_hash()), and store the file
measurement in a temporary integrity_iint_cache structure.
This change does not cause memory usage increase, due to using the
temporary integrity_iint_cache structure, and due to freeing the
ima_digest_data structure inside integrity_iint_cache before exiting from
__ima_inode_hash().
For compatibility reasons, the behavior of ima_inode_hash() remains
unchanged.
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
security/integrity/ima/ima_main.c | 36 +++++++++++++++++++++----------
1 file changed, 25 insertions(+), 11 deletions(-)
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 946ba8a12eab..3562a212a5ba 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -520,15 +520,27 @@ int ima_file_check(struct file *file, int mask)
}
EXPORT_SYMBOL_GPL(ima_file_check);
-static int __ima_inode_hash(struct inode *inode, char *buf, size_t buf_size)
+static int __ima_inode_hash(struct inode *inode, struct file *file, char *buf,
+ size_t buf_size)
{
- struct integrity_iint_cache *iint;
- int hash_algo;
+ struct integrity_iint_cache *iint = NULL, tmp_iint;
+ int rc, hash_algo;
- if (!ima_policy_flag)
- return -EOPNOTSUPP;
+ if (ima_policy_flag)
+ iint = integrity_iint_find(inode);
+
+ if (!iint && file) {
+ memset(&tmp_iint, 0, sizeof(tmp_iint));
+ tmp_iint.inode = inode;
+
+ rc = ima_collect_measurement(&tmp_iint, file, NULL, 0,
+ ima_hash_algo, NULL);
+ if (rc < 0)
+ return -EOPNOTSUPP;
+
+ iint = &tmp_iint;
+ }
- iint = integrity_iint_find(inode);
if (!iint)
return -EOPNOTSUPP;
@@ -552,12 +564,14 @@ static int __ima_inode_hash(struct inode *inode, char *buf, size_t buf_size)
hash_algo = iint->ima_hash->algo;
mutex_unlock(&iint->mutex);
+ if (iint == &tmp_iint)
+ kfree(iint->ima_hash);
+
return hash_algo;
}
/**
- * ima_file_hash - return the stored measurement if a file has been hashed and
- * is in the iint cache.
+ * ima_file_hash - return a measurement of the file
* @file: pointer to the file
* @buf: buffer in which to store the hash
* @buf_size: length of the buffer
@@ -570,7 +584,7 @@ static int __ima_inode_hash(struct inode *inode, char *buf, size_t buf_size)
* The file hash returned is based on the entire file, including the appended
* signature.
*
- * If IMA is disabled or if no measurement is available, return -EOPNOTSUPP.
+ * If the measurement cannot be performed, return -EOPNOTSUPP.
* If the parameters are incorrect, return -EINVAL.
*/
int ima_file_hash(struct file *file, char *buf, size_t buf_size)
@@ -578,7 +592,7 @@ int ima_file_hash(struct file *file, char *buf, size_t buf_size)
if (!file)
return -EINVAL;
- return __ima_inode_hash(file_inode(file), buf, buf_size);
+ return __ima_inode_hash(file_inode(file), file, buf, buf_size);
}
EXPORT_SYMBOL_GPL(ima_file_hash);
@@ -605,7 +619,7 @@ int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size)
if (!inode)
return -EINVAL;
- return __ima_inode_hash(inode, buf, buf_size);
+ return __ima_inode_hash(inode, NULL, buf, buf_size);
}
EXPORT_SYMBOL_GPL(ima_inode_hash);
--
2.32.0
^ permalink raw reply related
* [PATCH v2 3/6] bpf-lsm: Introduce new helper bpf_ima_file_hash()
From: Roberto Sassu @ 2022-02-15 12:40 UTC (permalink / raw)
To: zohar, shuah, ast, daniel, andrii, kpsingh, revest
Cc: linux-integrity, linux-security-module, linux-kselftest, netdev,
bpf, linux-kernel, Roberto Sassu
In-Reply-To: <20220215124042.186506-1-roberto.sassu@huawei.com>
ima_file_hash() has been modified to calculate the measurement of a file on
demand, if it has not been already performed by IMA. For compatibility
reasons, ima_inode_hash() remains unchanged.
Keep the same approach in eBPF and introduce the new helper
bpf_ima_file_hash() to take advantage of the modified behavior of
ima_file_hash().
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
include/uapi/linux/bpf.h | 11 +++++++++++
kernel/bpf/bpf_lsm.c | 20 ++++++++++++++++++++
tools/include/uapi/linux/bpf.h | 11 +++++++++++
3 files changed, 42 insertions(+)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index b0383d371b9a..ba33d5718d6b 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -4648,6 +4648,16 @@ union bpf_attr {
* **-EOPNOTSUP** if IMA is disabled or **-EINVAL** if
* invalid arguments are passed.
*
+ * long bpf_ima_file_hash(struct file *file, void *dst, u32 size)
+ * Description
+ * Returns a calculated IMA hash of the *file*.
+ * If the hash is larger than *size*, then only *size*
+ * bytes will be copied to *dst*
+ * Return
+ * The **hash_algo** is returned on success,
+ * **-EOPNOTSUP** if the hash calculation failed or **-EINVAL** if
+ * invalid arguments are passed.
+ *
* struct socket *bpf_sock_from_file(struct file *file)
* Description
* If the given file represents a socket, returns the associated
@@ -5182,6 +5192,7 @@ union bpf_attr {
FN(bprm_opts_set), \
FN(ktime_get_coarse_ns), \
FN(ima_inode_hash), \
+ FN(ima_file_hash), \
FN(sock_from_file), \
FN(check_mtu), \
FN(for_each_map_elem), \
diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
index 9e4ecc990647..e8d27af5bbcc 100644
--- a/kernel/bpf/bpf_lsm.c
+++ b/kernel/bpf/bpf_lsm.c
@@ -99,6 +99,24 @@ static const struct bpf_func_proto bpf_ima_inode_hash_proto = {
.allowed = bpf_ima_inode_hash_allowed,
};
+BPF_CALL_3(bpf_ima_file_hash, struct file *, file, void *, dst, u32, size)
+{
+ return ima_file_hash(file, dst, size);
+}
+
+BTF_ID_LIST_SINGLE(bpf_ima_file_hash_btf_ids, struct, file)
+
+static const struct bpf_func_proto bpf_ima_file_hash_proto = {
+ .func = bpf_ima_file_hash,
+ .gpl_only = false,
+ .ret_type = RET_INTEGER,
+ .arg1_type = ARG_PTR_TO_BTF_ID,
+ .arg1_btf_id = &bpf_ima_file_hash_btf_ids[0],
+ .arg2_type = ARG_PTR_TO_UNINIT_MEM,
+ .arg3_type = ARG_CONST_SIZE,
+ .allowed = bpf_ima_inode_hash_allowed,
+};
+
static const struct bpf_func_proto *
bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
{
@@ -121,6 +139,8 @@ bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_bprm_opts_set_proto;
case BPF_FUNC_ima_inode_hash:
return prog->aux->sleepable ? &bpf_ima_inode_hash_proto : NULL;
+ case BPF_FUNC_ima_file_hash:
+ return prog->aux->sleepable ? &bpf_ima_file_hash_proto : NULL;
default:
return tracing_prog_func_proto(func_id, prog);
}
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index b0383d371b9a..ba33d5718d6b 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -4648,6 +4648,16 @@ union bpf_attr {
* **-EOPNOTSUP** if IMA is disabled or **-EINVAL** if
* invalid arguments are passed.
*
+ * long bpf_ima_file_hash(struct file *file, void *dst, u32 size)
+ * Description
+ * Returns a calculated IMA hash of the *file*.
+ * If the hash is larger than *size*, then only *size*
+ * bytes will be copied to *dst*
+ * Return
+ * The **hash_algo** is returned on success,
+ * **-EOPNOTSUP** if the hash calculation failed or **-EINVAL** if
+ * invalid arguments are passed.
+ *
* struct socket *bpf_sock_from_file(struct file *file)
* Description
* If the given file represents a socket, returns the associated
@@ -5182,6 +5192,7 @@ union bpf_attr {
FN(bprm_opts_set), \
FN(ktime_get_coarse_ns), \
FN(ima_inode_hash), \
+ FN(ima_file_hash), \
FN(sock_from_file), \
FN(check_mtu), \
FN(for_each_map_elem), \
--
2.32.0
^ permalink raw reply related
* [PATCH v2 0/6] bpf-lsm: Extend interoperability with IMA
From: Roberto Sassu @ 2022-02-15 12:40 UTC (permalink / raw)
To: zohar, shuah, ast, daniel, andrii, kpsingh, revest
Cc: linux-integrity, linux-security-module, linux-kselftest, netdev,
bpf, linux-kernel, Roberto Sassu
Extend the interoperability with IMA, to give wider flexibility for the
implementation of integrity-focused LSMs based on eBPF.
Patch 1 fixes some style issues.
Patches 2-4 gives the ability to eBPF-based LSMs to take advantage of the
measurement capability of IMA without needing to setup a policy in IMA
(those LSMs might implement the policy capability themselves).
Patches 5-6 allows eBPF-based LSMs to evaluate files read by the kernel.
Changelog
v1:
- Modify ima_file_hash() only and allow the usage of the function with the
modified behavior by eBPF-based LSMs through the new function
bpf_ima_file_hash() (suggested by Mimi)
- Make bpf_lsm_kernel_read_file() sleepable so that bpf_ima_inode_hash()
and bpf_ima_file_hash() can be called inside the implementation of
eBPF-based LSMs for this hook
Roberto Sassu (6):
ima: Fix documentation-related warnings in ima_main.c
ima: Always return a file measurement in ima_file_hash()
bpf-lsm: Introduce new helper bpf_ima_file_hash()
selftests/bpf: Add test for bpf_ima_file_hash()
bpf-lsm: Make bpf_lsm_kernel_read_file() as sleepable
selftests/bpf: Add test for bpf_lsm_kernel_read_file()
include/uapi/linux/bpf.h | 11 +++++
kernel/bpf/bpf_lsm.c | 21 +++++++++
security/integrity/ima/ima_main.c | 47 ++++++++++++-------
tools/include/uapi/linux/bpf.h | 11 +++++
tools/testing/selftests/bpf/ima_setup.sh | 2 +
.../selftests/bpf/prog_tests/test_ima.c | 30 ++++++++++--
tools/testing/selftests/bpf/progs/ima.c | 34 ++++++++++++--
7 files changed, 132 insertions(+), 24 deletions(-)
--
2.32.0
^ permalink raw reply
* [PATCH v2 1/6] ima: Fix documentation-related warnings in ima_main.c
From: Roberto Sassu @ 2022-02-15 12:40 UTC (permalink / raw)
To: zohar, shuah, ast, daniel, andrii, kpsingh, revest
Cc: linux-integrity, linux-security-module, linux-kselftest, netdev,
bpf, linux-kernel, Roberto Sassu
In-Reply-To: <20220215124042.186506-1-roberto.sassu@huawei.com>
Fix some warnings in ima_main.c, displayed with W=n make argument.
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
security/integrity/ima/ima_main.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 8c6e4514d494..946ba8a12eab 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -418,6 +418,7 @@ int ima_file_mmap(struct file *file, unsigned long prot)
/**
* ima_file_mprotect - based on policy, limit mprotect change
+ * @vma: vm_area_struct protection is set to
* @prot: contains the protection that will be applied by the kernel.
*
* Files can be mmap'ed read/write and later changed to execute to circumvent
@@ -610,8 +611,8 @@ EXPORT_SYMBOL_GPL(ima_inode_hash);
/**
* ima_post_create_tmpfile - mark newly created tmpfile as new
- * @mnt_userns: user namespace of the mount the inode was found from
- * @file : newly created tmpfile
+ * @mnt_userns: user namespace of the mount the inode was found from
+ * @inode: inode of the newly created tmpfile
*
* No measuring, appraising or auditing of newly created tmpfiles is needed.
* Skip calling process_measurement(), but indicate which newly, created
@@ -643,7 +644,7 @@ void ima_post_create_tmpfile(struct user_namespace *mnt_userns,
/**
* ima_post_path_mknod - mark as a new inode
- * @mnt_userns: user namespace of the mount the inode was found from
+ * @mnt_userns: user namespace of the mount the inode was found from
* @dentry: newly created dentry
*
* Mark files created via the mknodat syscall as new, so that the
@@ -814,8 +815,8 @@ int ima_load_data(enum kernel_load_data_id id, bool contents)
* ima_post_load_data - appraise decision based on policy
* @buf: pointer to in memory file contents
* @size: size of in memory file contents
- * @id: kernel load data caller identifier
- * @description: @id-specific description of contents
+ * @load_id: kernel load data caller identifier
+ * @description: @load_id-specific description of contents
*
* Measure/appraise/audit in memory buffer based on policy. Policy rules
* are written in terms of a policy identifier.
--
2.32.0
^ permalink raw reply related
* Re: [PATCH] ima: Calculate digest in ima_inode_hash() if not available
From: Mimi Zohar @ 2022-02-15 11:16 UTC (permalink / raw)
To: Roberto Sassu, shuah@kernel.org, ast@kernel.org,
daniel@iogearbox.net, andrii@kernel.org, kpsingh@kernel.org,
Florent Revest
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kselftest@vger.kernel.org, netdev@vger.kernel.org,
bpf@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <311b5d0b3b824c548a4032a76a408944@huawei.com>
On Tue, 2022-02-15 at 08:00 +0000, Roberto Sassu wrote:
> > >
> > > I found that just checking that iint->ima_hash is not NULL is not enough
> > > (ima_inode_hash() might still return the old digest after a file write).
> > > Should I replace that check with !(iint->flags & IMA_COLLECTED)?
> > > Or should I do only for ima_file_hash() and recalculate the digest
> > > if necessary?
> >
> > Updating the file hash after each write would really impact IMA
> > performance. If you really want to detect any file change, no matter
> > how frequently it occurs, your best bet would be to track i_generation
> > and i_version. Stefan is already adding "i_generation" for IMA
> > namespacing.
>
> I just wanted the ability to get a fresh digest after a file opened
> for writing is closed. Since in my use case I would not use an IMA
> policy, that would not be a problem.
As I recall, the __fput() delay was to prevent locking ordering issues
- inode, iint.
--
thanks,
Mimi
^ permalink raw reply
* Re: [PATCH net v3 0/2] security: fixups for the security hooks in sctp
From: Richard Haines @ 2022-02-15 9:41 UTC (permalink / raw)
To: Ondrej Mosnacek, netdev, davem, kuba, selinux, Paul Moore
Cc: Xin Long, Vlad Yasevich, Neil Horman, Marcelo Ricardo Leitner,
linux-sctp, linux-security-module, linux-kernel
In-Reply-To: <20220212175922.665442-1-omosnace@redhat.com>
On Sat, 2022-02-12 at 18:59 +0100, Ondrej Mosnacek wrote:
> This is a third round of patches to fix the SCTP-SELinux interaction
> w.r.t. client-side peeloff. The patches are a modified version of Xin
> Long's patches posted previously, of which only a part was merged
> (the
> rest was merged for a while, but was later reverted):
> https://lore.kernel.org/selinux/cover.1635854268.git.lucien.xin@gmail.com/T/
>
> In gist, these patches replace the call to
> security_inet_conn_established() in SCTP with a new hook
> security_sctp_assoc_established() and implement the new hook in
> SELinux
> so that the client-side association labels are set correctly (which
> matters in case the association eventually gets peeled off into a
> separate socket).
>
> Note that other LSMs than SELinux don't implement the SCTP hooks nor
> inet_conn_established, so they shouldn't be affected by any of these
> changes.
>
> These patches were tested by selinux-testsuite [1] with an additional
> patch [2] and by lksctp-tools func_tests [3].
>
> Changes since v2:
> - patches 1 and 2 dropped as they are already in mainline (not
> reverted)
> - in patch 3, the return value of security_sctp_assoc_established()
> is
> changed to int, the call is moved earlier in the function, and if
> the
> hook returns an error value, the packet will now be discarded,
> aborting the association
> - patch 4 has been changed a lot - please see the patch description
> for
> details on how the hook is now implemented and why
>
> [1] https://github.com/SELinuxProject/selinux-testsuite/
> [2]
> https://patchwork.kernel.org/project/selinux/patch/20211021144543.740762-1-omosnace@redhat.com/
> [3] https://github.com/sctp/lksctp-tools/tree/master/src/func_tests
>
> Ondrej Mosnacek (2):
> security: add sctp_assoc_established hook
> security: implement sctp_assoc_established hook in selinux
>
> Documentation/security/SCTP.rst | 22 ++++----
> include/linux/lsm_hook_defs.h | 2 +
> include/linux/lsm_hooks.h | 5 ++
> include/linux/security.h | 8 +++
> net/sctp/sm_statefuns.c | 8 +--
> security/security.c | 7 +++
> security/selinux/hooks.c | 90 ++++++++++++++++++++++++-------
> --
> 7 files changed, 103 insertions(+), 39 deletions(-)
>
Built this patchset on kernel 5.17-rc4 with no problems.
Tested using [PATCH testsuite v3] tests/sctp: add client peeloff tests
Tested-by: Richard Haines <richard_c_haines@btinternet.com>
^ permalink raw reply
* RE: [PATCH] ima: Calculate digest in ima_inode_hash() if not available
From: Roberto Sassu @ 2022-02-15 8:00 UTC (permalink / raw)
To: Mimi Zohar, shuah@kernel.org, ast@kernel.org,
daniel@iogearbox.net, andrii@kernel.org, kpsingh@kernel.org,
Florent Revest
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kselftest@vger.kernel.org, netdev@vger.kernel.org,
bpf@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <36f85113f181f78eda3576823bd92be3f9cd1802.camel@linux.ibm.com>
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Monday, February 14, 2022 11:33 PM
> On Mon, 2022-02-14 at 17:05 +0000, Roberto Sassu wrote:
> > > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > > Sent: Sunday, February 13, 2022 2:06 PM
> > > Hi Roberto,
> > >
> > > On Fri, 2022-02-11 at 11:48 +0100, Roberto Sassu wrote:
> > > > __ima_inode_hash() checks if a digest has been already calculated by
> > > > looking for the integrity_iint_cache structure associated to the passed
> > > > inode.
> > > >
> > > > Users of ima_file_hash() and ima_inode_hash() (e.g. eBPF) might be
> > > > interested in obtaining the information without having to setup an IMA
> > > > policy so that the digest is always available at the time they call one of
> > > > those functions.
> > > >
> > > > Open a new file descriptor in __ima_inode_hash(), so that this function
> > > > could invoke ima_collect_measurement() to calculate the digest if it is not
> > > > available. Still return -EOPNOTSUPP if the calculation failed.
> > > >
> > > > Instead of opening a new file descriptor, the one from ima_file_hash()
> > > > could have been used. However, since ima_inode_hash() was created to
> > > obtain
> > > > the digest when the file descriptor is not available, it could benefit from
> > > > this change too. Also, the opened file descriptor might be not suitable for
> > > > use (file descriptor opened not for reading).
> > > >
> > > > This change does not cause memory usage increase, due to using a
> temporary
> > > > integrity_iint_cache structure for the digest calculation, and due to
> > > > freeing the ima_digest_data structure inside integrity_iint_cache before
> > > > exiting from __ima_inode_hash().
> > > >
> > > > Finally, update the test by removing ima_setup.sh (it is not necessary
> > > > anymore to set an IMA policy) and by directly executing /bin/true.
> > > >
> > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > >
> > > Although this patch doesn't directly modify either ima_file_hash() or
> > > ima_inode_hash(), this change affects both functions. ima_file_hash()
> > > was introduced to be used with eBPF. Based on Florent's post, changing
> > > the ima_file_hash() behavor seems fine. Since I have no idea whether
> > > anyone is still using ima_inode_hash(), perhaps it would be safer to
> > > limit this behavior change to just ima_file_hash().
> >
> > Hi Mimi
> >
> > ok.
> >
> > I found that just checking that iint->ima_hash is not NULL is not enough
> > (ima_inode_hash() might still return the old digest after a file write).
> > Should I replace that check with !(iint->flags & IMA_COLLECTED)?
> > Or should I do only for ima_file_hash() and recalculate the digest
> > if necessary?
>
> Updating the file hash after each write would really impact IMA
> performance. If you really want to detect any file change, no matter
> how frequently it occurs, your best bet would be to track i_generation
> and i_version. Stefan is already adding "i_generation" for IMA
> namespacing.
I just wanted the ability to get a fresh digest after a file opened
for writing is closed. Since in my use case I would not use an IMA
policy, that would not be a problem.
Thanks
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Zhong Ronghua
> > > Please update the ima_file_hash() doc. While touching this area, I'd
> > > appreciate your fixing the first doc line in both ima_file_hash() and
> > > ima_inode_hash() cases, which wraps spanning two lines.
> >
> > Did you mean to make the description shorter or to have everything
> > in one line? According to the kernel documentation (kernel-doc.rst),
> > having the brief description in multiple lines should be fine.
>
> Thanks for checking kernel-doc. The "brief description" not wrapping
> across multiple lines did in fact change.
>
> > > Please split the IMA from the eBPF changes.
> >
> > Ok.
>
> --
> thanks,
>
> Mimi
^ permalink raw reply
* Re: [PATCH net v3 0/2] security: fixups for the security hooks in sctp
From: Xin Long @ 2022-02-15 4:26 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: network dev, davem, Jakub Kicinski, SElinux list, Paul Moore,
Richard Haines, Vlad Yasevich, Neil Horman,
Marcelo Ricardo Leitner, linux-sctp @ vger . kernel . org,
LSM List, LKML
In-Reply-To: <20220212175922.665442-1-omosnace@redhat.com>
On Sun, Feb 13, 2022 at 1:59 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> This is a third round of patches to fix the SCTP-SELinux interaction
> w.r.t. client-side peeloff. The patches are a modified version of Xin
> Long's patches posted previously, of which only a part was merged (the
> rest was merged for a while, but was later reverted):
> https://lore.kernel.org/selinux/cover.1635854268.git.lucien.xin@gmail.com/T/
>
> In gist, these patches replace the call to
> security_inet_conn_established() in SCTP with a new hook
> security_sctp_assoc_established() and implement the new hook in SELinux
> so that the client-side association labels are set correctly (which
> matters in case the association eventually gets peeled off into a
> separate socket).
>
> Note that other LSMs than SELinux don't implement the SCTP hooks nor
> inet_conn_established, so they shouldn't be affected by any of these
> changes.
>
> These patches were tested by selinux-testsuite [1] with an additional
> patch [2] and by lksctp-tools func_tests [3].
>
> Changes since v2:
> - patches 1 and 2 dropped as they are already in mainline (not reverted)
> - in patch 3, the return value of security_sctp_assoc_established() is
> changed to int, the call is moved earlier in the function, and if the
> hook returns an error value, the packet will now be discarded,
> aborting the association
> - patch 4 has been changed a lot - please see the patch description for
> details on how the hook is now implemented and why
>
> [1] https://github.com/SELinuxProject/selinux-testsuite/
> [2] https://patchwork.kernel.org/project/selinux/patch/20211021144543.740762-1-omosnace@redhat.com/
> [3] https://github.com/sctp/lksctp-tools/tree/master/src/func_tests
>
> Ondrej Mosnacek (2):
> security: add sctp_assoc_established hook
> security: implement sctp_assoc_established hook in selinux
>
> Documentation/security/SCTP.rst | 22 ++++----
> include/linux/lsm_hook_defs.h | 2 +
> include/linux/lsm_hooks.h | 5 ++
> include/linux/security.h | 8 +++
> net/sctp/sm_statefuns.c | 8 +--
> security/security.c | 7 +++
> security/selinux/hooks.c | 90 ++++++++++++++++++++++++---------
> 7 files changed, 103 insertions(+), 39 deletions(-)
>
> --
> 2.34.1
>
Reviewed-by: Xin Long <lucien.xin@gmail.com>
^ permalink raw reply
* Re: [PATCH net v3 2/2] security: implement sctp_assoc_established hook in selinux
From: Xin Long @ 2022-02-15 4:13 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: Jakub Kicinski, Paul Moore, Ondrej Mosnacek, netdev, David Miller,
SElinux list, Richard Haines, Vlad Yasevich, Neil Horman,
open list:SCTP PROTOCOL, LSM List, LKML, Prashanth Prahlad
In-Reply-To: <CAFSqH7zC-4Ti_mzK4ZrpCVtNVCxD8h729MezG2avJLGJ2JrMTg@mail.gmail.com>
On Tue, Feb 15, 2022 at 11:58 AM Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
>
>
>
> Em seg., 14 de fev. de 2022 21:54, Jakub Kicinski <kuba@kernel.org> escreveu:
>>
>> On Mon, 14 Feb 2022 17:14:04 -0500 Paul Moore wrote:
>> > If I can get an ACK from one of the SCTP and/or netdev folks I'll
>> > merge this into the selinux/next branch.
>>
>> No objections here FWIW, I'd defer the official acking to the SCTP
>> maintainers.
>
>
> None from my side either, but I really want to hear from Xin. He has worked on this since day 0.
>
Looks okay to me.
The difference from the old one is that: with
selinux_sctp_process_new_assoc() called in
selinux_sctp_assoc_established(), the client sksec->peer_sid is using
the first asoc's peer_secid, instead of the latest asoc's peer_secid.
And not sure if it will cause any problems when doing the extra check
sksec->peer_sid != asoc->peer_secid for the latest asoc and *returns
err*. But I don't know about selinux, I guess there must be a reason
from selinux side.
I will ACK on patch 0/2.
Thanks Ondrej for working on this patiently.
^ permalink raw reply
* Re: [PATCH net v3 2/2] security: implement sctp_assoc_established hook in selinux
From: Jakub Kicinski @ 2022-02-15 0:54 UTC (permalink / raw)
To: Paul Moore
Cc: Ondrej Mosnacek, netdev, davem, selinux, Xin Long, Richard Haines,
Vlad Yasevich, Neil Horman, Marcelo Ricardo Leitner, linux-sctp,
linux-security-module, linux-kernel, Prashanth Prahlad
In-Reply-To: <CAHC9VhT90617FoqQJBCrDQ8gceVVA6a1h74h6T4ZOwNk6RVB3g@mail.gmail.com>
On Mon, 14 Feb 2022 17:14:04 -0500 Paul Moore wrote:
> If I can get an ACK from one of the SCTP and/or netdev folks I'll
> merge this into the selinux/next branch.
No objections here FWIW, I'd defer the official acking to the SCTP
maintainers.
^ permalink raw reply
* Re: [PATCH] ima: Calculate digest in ima_inode_hash() if not available
From: Mimi Zohar @ 2022-02-14 22:32 UTC (permalink / raw)
To: Roberto Sassu, shuah@kernel.org, ast@kernel.org,
daniel@iogearbox.net, andrii@kernel.org, kpsingh@kernel.org,
Florent Revest
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kselftest@vger.kernel.org, netdev@vger.kernel.org,
bpf@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <cc6bcb7742dc432ba990ee38b5909496@huawei.com>
On Mon, 2022-02-14 at 17:05 +0000, Roberto Sassu wrote:
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Sunday, February 13, 2022 2:06 PM
> > Hi Roberto,
> >
> > On Fri, 2022-02-11 at 11:48 +0100, Roberto Sassu wrote:
> > > __ima_inode_hash() checks if a digest has been already calculated by
> > > looking for the integrity_iint_cache structure associated to the passed
> > > inode.
> > >
> > > Users of ima_file_hash() and ima_inode_hash() (e.g. eBPF) might be
> > > interested in obtaining the information without having to setup an IMA
> > > policy so that the digest is always available at the time they call one of
> > > those functions.
> > >
> > > Open a new file descriptor in __ima_inode_hash(), so that this function
> > > could invoke ima_collect_measurement() to calculate the digest if it is not
> > > available. Still return -EOPNOTSUPP if the calculation failed.
> > >
> > > Instead of opening a new file descriptor, the one from ima_file_hash()
> > > could have been used. However, since ima_inode_hash() was created to
> > obtain
> > > the digest when the file descriptor is not available, it could benefit from
> > > this change too. Also, the opened file descriptor might be not suitable for
> > > use (file descriptor opened not for reading).
> > >
> > > This change does not cause memory usage increase, due to using a temporary
> > > integrity_iint_cache structure for the digest calculation, and due to
> > > freeing the ima_digest_data structure inside integrity_iint_cache before
> > > exiting from __ima_inode_hash().
> > >
> > > Finally, update the test by removing ima_setup.sh (it is not necessary
> > > anymore to set an IMA policy) and by directly executing /bin/true.
> > >
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> >
> > Although this patch doesn't directly modify either ima_file_hash() or
> > ima_inode_hash(), this change affects both functions. ima_file_hash()
> > was introduced to be used with eBPF. Based on Florent's post, changing
> > the ima_file_hash() behavor seems fine. Since I have no idea whether
> > anyone is still using ima_inode_hash(), perhaps it would be safer to
> > limit this behavior change to just ima_file_hash().
>
> Hi Mimi
>
> ok.
>
> I found that just checking that iint->ima_hash is not NULL is not enough
> (ima_inode_hash() might still return the old digest after a file write).
> Should I replace that check with !(iint->flags & IMA_COLLECTED)?
> Or should I do only for ima_file_hash() and recalculate the digest
> if necessary?
Updating the file hash after each write would really impact IMA
performance. If you really want to detect any file change, no matter
how frequently it occurs, your best bet would be to track i_generation
and i_version. Stefan is already adding "i_generation" for IMA
namespacing.
>
> > Please update the ima_file_hash() doc. While touching this area, I'd
> > appreciate your fixing the first doc line in both ima_file_hash() and
> > ima_inode_hash() cases, which wraps spanning two lines.
>
> Did you mean to make the description shorter or to have everything
> in one line? According to the kernel documentation (kernel-doc.rst),
> having the brief description in multiple lines should be fine.
Thanks for checking kernel-doc. The "brief description" not wrapping
across multiple lines did in fact change.
> > Please split the IMA from the eBPF changes.
>
> Ok.
--
thanks,
Mimi
^ permalink raw reply
* Re: [PATCH net v3 2/2] security: implement sctp_assoc_established hook in selinux
From: Paul Moore @ 2022-02-14 22:14 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: netdev, davem, kuba, selinux, Xin Long, Richard Haines,
Vlad Yasevich, Neil Horman, Marcelo Ricardo Leitner, linux-sctp,
linux-security-module, linux-kernel, Prashanth Prahlad
In-Reply-To: <20220212175922.665442-3-omosnace@redhat.com>
On Sat, Feb 12, 2022 at 12:59 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> Do this by extracting the peer labeling per-association logic from
> selinux_sctp_assoc_request() into a new helper
> selinux_sctp_process_new_assoc() and use this helper in both
> selinux_sctp_assoc_request() and selinux_sctp_assoc_established(). This
> ensures that the peer labeling behavior as documented in
> Documentation/security/SCTP.rst is applied both on the client and server
> side:
> """
> An SCTP socket will only have one peer label assigned to it. This will be
> assigned during the establishment of the first association. Any further
> associations on this socket will have their packet peer label compared to
> the sockets peer label, and only if they are different will the
> ``association`` permission be validated. This is validated by checking the
> socket peer sid against the received packets peer sid to determine whether
> the association should be allowed or denied.
> """
>
> At the same time, it also ensures that the peer label of the association
> is set to the correct value, such that if it is peeled off into a new
> socket, the socket's peer label will then be set to the association's
> peer label, same as it already works on the server side.
>
> While selinux_inet_conn_established() (which we are replacing by
> selinux_sctp_assoc_established() for SCTP) only deals with assigning a
> peer label to the connection (socket), in case of SCTP we need to also
> copy the (local) socket label to the association, so that
> selinux_sctp_sk_clone() can then pick it up for the new socket in case
> of SCTP peeloff.
>
> Careful readers will notice that the selinux_sctp_process_new_assoc()
> helper also includes the "IPv4 packet received over an IPv6 socket"
> check, even though it hadn't been in selinux_sctp_assoc_request()
> before. While such check is not necessary in
> selinux_inet_conn_request() (because struct request_sock's family field
> is already set according to the skb's family), here it is needed, as we
> don't have request_sock and we take the initial family from the socket.
> In selinux_sctp_assoc_established() it is similarly needed as well (and
> also selinux_inet_conn_established() already has it).
>
> Fixes: 72e89f50084c ("security: Add support for SCTP security hooks")
> Reported-by: Prashanth Prahlad <pprahlad@redhat.com>
> Based-on-patch-by: Xin Long <lucien.xin@gmail.com>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
> security/selinux/hooks.c | 90 +++++++++++++++++++++++++++++-----------
> 1 file changed, 66 insertions(+), 24 deletions(-)
This patch, and patch 1/2, look good to me; I'm assuming this resolves
all of the known SELinux/SCTP problems identified before the new year?
If I can get an ACK from one of the SCTP and/or netdev folks I'll
merge this into the selinux/next branch.
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH v5 2/6] powerpc/kexec_file: Add KEXEC_SIG support.
From: Mimi Zohar @ 2022-02-14 17:09 UTC (permalink / raw)
To: Michal Suchánek
Cc: keyrings, linux-crypto, linux-integrity, kexec, Philipp Rudo,
Nayna, Rob Herring, linux-s390, Vasily Gorbik,
Lakshmi Ramasubramanian, Heiko Carstens, Jessica Yu, linux-kernel,
David Howells, Christian Borntraeger, Luis Chamberlain,
Paul Mackerras, Hari Bathini, Alexander Gordeev, linuxppc-dev,
Frank van der Linden, Thiago Jung Bauermann, Daniel Axtens,
buendgen, Michael Ellerman, Benjamin Herrenschmidt,
Christian Borntraeger, Herbert Xu, David S. Miller,
Dmitry Kasatkin, James Morris, Serge E. Hallyn, Sven Schnelle,
Baoquan He, linux-security-module
In-Reply-To: <20220214155524.GN3113@kunlun.suse.cz>
On Mon, 2022-02-14 at 16:55 +0100, Michal Suchánek wrote:
> Hello,
>
> On Mon, Feb 14, 2022 at 10:14:16AM -0500, Mimi Zohar wrote:
> > Hi Michal,
> >
> > On Sun, 2022-02-13 at 21:59 -0500, Mimi Zohar wrote:
> >
> > >
> > > On Tue, 2022-01-11 at 12:37 +0100, Michal Suchanek wrote:
> > > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> > > > index dea74d7717c0..1cde9b6c5987 100644
> > > > --- a/arch/powerpc/Kconfig
> > > > +++ b/arch/powerpc/Kconfig
> > > > @@ -560,6 +560,22 @@ config KEXEC_FILE
> > > > config ARCH_HAS_KEXEC_PURGATORY
> > > > def_bool KEXEC_FILE
> > > >
> > > > +config KEXEC_SIG
> > > > + bool "Verify kernel signature during kexec_file_load() syscall"
> > > > + depends on KEXEC_FILE && MODULE_SIG_FORMAT
> > > > + help
> > > > + This option makes kernel signature verification mandatory for
>
> This is actually wrong. KEXEC_SIG makes it mandatory that any signature
> that is appended is valid and made by a key that is part of the platform
> keyiring (which is also wrong, built-in keys should be also accepted).
> KEXEC_SIG_FORCE or an IMA policy makes it mandatory that the signature
> is present.
I'm aware of MODULE_SIG_FORCE, which isn't normally enabled by distros,
but enabling MODULE_SIG allows MODULE_SIG_FORCE to be enabled on the
boot command line. In the IMA arch policies, if MODULE_SIG is enabled,
it is then enforced, otherwise an IMA "appraise" policy rule is
defined. This rule would prevent the module_load syscall.
I'm not aware of KEXEC_SIG_FORCE. If there is such a Kconfig, then I
assume it could work similarly.
>
> > > > + the kexec_file_load() syscall.
> > >
> > > When KEXEC_SIG is enabled on other architectures, IMA does not define a
> > > kexec 'appraise' policy rule. Refer to the policy rules in
> > > security/ima/ima_efi.c. Similarly the kexec 'appraise' policy rule in
>
> I suppose you mean security/integrity/ima/ima_efi.c
Yes
>
> I also think it's misguided because KEXEC_SIG in itself does not enforce
> the signature. KEXEC_SIG_FORCE does.
Right, which is why the IMA efi policy calls set_module_sig_enforced().
>
> > > arch/powerpc/kernel/ima_policy.c should not be defined.
>
> I suppose you mean arch/powerpc/kernel/ima_arch.c - see above.
Sorry, yes.
>
>
> Thanks for taking the time to reseach and summarize the differences.
>
> > The discussion shouldn't only be about IMA vs. KEXEC_SIG kernel image
> > signature verification. Let's try and reframe the problem a bit.
> >
> > 1. Unify and simply the existing kexec signature verification so
> > verifying the KEXEC kernel image signature works irrespective of
> > signature type - PE, appended signature.
> >
> > solution: enable KEXEC_SIG (This patch set, with the above powerpc IMA
> > policy changes.)
> >
> > 2. Measure and include the kexec kernel image in a log for attestation,
> > if desired.
> >
> > solution: enable IMA_ARCH_POLICY
> > - Powerpc: requires trusted boot to be enabled.
> > - EFI: requires secure boot to be enabled. The IMA efi policy
> > doesn't differentiate between secure and trusted boot.
> >
> > 3. Carry the kexec kernel image measurement across kexec, if desired
> > and supported on the architecture.
> >
> > solution: enable IMA_KEXEC
> >
> > Comparison:
> > - Are there any differences between IMA vs. KEXEC_SIG measuring the
> > kexec kernel image?
> >
> > One of the main differences is "what" is included in the measurement
> > list differs. In both cases, the 'd-ng' field of the IMA measurement
> > list template (e.g. ima-ng, ima-sig, ima-modsig) is the full file hash
> > including the appended signature. With IMA and the 'ima-modsig'
> > template, an additional hash without the appended signature is defined,
> > as well as including the appended signature in the 'sig' field.
> >
> > Including the file hash and appended signature in the measurement list
> > allows an attestation server, for example, to verify the appended
> > signature without having to know the file hash without the signature.
>
> I don't understand this part. Isn't the hash *with* signature always
> included, and the distinguishing part about IMA is the hash *without*
> signature which is the same irrespective of signature type (PE, appended
> xattr) and irrespective of the keyt used for signoing?
Roberto Sassu added support for IMA templates. These are the
definitions of 'ima-sig' and 'ima-modsig'.
{.name = "ima-sig", .fmt = "d-ng|n-ng|sig"},
{.name = "ima-modsig", .fmt = "d-ng|n-ng|sig|d-modsig|modsig"}
d-ng: is the file hash. With the proposed IMA support for fs-verity
digests, the 'd-ng' field may also include the fsverity digest, based
on policy.
n-ng: is the file pathname.
sig: is the file signature stored as a 'security.ima' xattr (may be
NULL).
d-modsig: is the file hash without the appended signature (may be
NULL).
FYI, changing from "module signature" to "appended signature", might
impact the template field and name. :)
modsig: is the appended signature (May be NULL).
I really haven't looked at PE signatures, so I can't comment on them.
>
> > Other differences are already included in the Kconfig KEXEC_SIG "Notes"
> > section.
>
> Which besides what is already described above would be blacklisting
> specific binaries, which is much more effective if you have hashes of
> binaries without signature.
Thanks, Nayna will be happy to hear you approve.
FYI, IMA calculates the file hash once, which is then added to the IMA
measurement list, extended into the TPM (when available), used to
verify signatures, and included in the audit log.
With the KEXEC_SIG support, assuming the IMA arch policy is enabled,
the file hash would be calculated twice - once for verifying the file
signature and again for the measurement.
--
thanks,
Mimi
^ permalink raw reply
* RE: [PATCH] ima: Calculate digest in ima_inode_hash() if not available
From: Roberto Sassu @ 2022-02-14 17:05 UTC (permalink / raw)
To: Mimi Zohar, shuah@kernel.org, ast@kernel.org,
daniel@iogearbox.net, andrii@kernel.org, kpsingh@kernel.org,
Florent Revest
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kselftest@vger.kernel.org, netdev@vger.kernel.org,
bpf@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <537635732d9cbcc42bcf7be5ed932d284b03d39f.camel@linux.ibm.com>
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Sunday, February 13, 2022 2:06 PM
> Hi Roberto,
>
> On Fri, 2022-02-11 at 11:48 +0100, Roberto Sassu wrote:
> > __ima_inode_hash() checks if a digest has been already calculated by
> > looking for the integrity_iint_cache structure associated to the passed
> > inode.
> >
> > Users of ima_file_hash() and ima_inode_hash() (e.g. eBPF) might be
> > interested in obtaining the information without having to setup an IMA
> > policy so that the digest is always available at the time they call one of
> > those functions.
> >
> > Open a new file descriptor in __ima_inode_hash(), so that this function
> > could invoke ima_collect_measurement() to calculate the digest if it is not
> > available. Still return -EOPNOTSUPP if the calculation failed.
> >
> > Instead of opening a new file descriptor, the one from ima_file_hash()
> > could have been used. However, since ima_inode_hash() was created to
> obtain
> > the digest when the file descriptor is not available, it could benefit from
> > this change too. Also, the opened file descriptor might be not suitable for
> > use (file descriptor opened not for reading).
> >
> > This change does not cause memory usage increase, due to using a temporary
> > integrity_iint_cache structure for the digest calculation, and due to
> > freeing the ima_digest_data structure inside integrity_iint_cache before
> > exiting from __ima_inode_hash().
> >
> > Finally, update the test by removing ima_setup.sh (it is not necessary
> > anymore to set an IMA policy) and by directly executing /bin/true.
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>
> Although this patch doesn't directly modify either ima_file_hash() or
> ima_inode_hash(), this change affects both functions. ima_file_hash()
> was introduced to be used with eBPF. Based on Florent's post, changing
> the ima_file_hash() behavor seems fine. Since I have no idea whether
> anyone is still using ima_inode_hash(), perhaps it would be safer to
> limit this behavior change to just ima_file_hash().
Hi Mimi
ok.
I found that just checking that iint->ima_hash is not NULL is not enough
(ima_inode_hash() might still return the old digest after a file write).
Should I replace that check with !(iint->flags & IMA_COLLECTED)?
Or should I do only for ima_file_hash() and recalculate the digest
if necessary?
> Please update the ima_file_hash() doc. While touching this area, I'd
> appreciate your fixing the first doc line in both ima_file_hash() and
> ima_inode_hash() cases, which wraps spanning two lines.
Did you mean to make the description shorter or to have everything
in one line? According to the kernel documentation (kernel-doc.rst),
having the brief description in multiple lines should be fine.
> Please split the IMA from the eBPF changes.
Ok.
Thanks
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Zhong Ronghua
> --
> thanks,
>
> Mimi
^ permalink raw reply
* Re: [PATCH v5 2/6] powerpc/kexec_file: Add KEXEC_SIG support.
From: Michal Suchánek @ 2022-02-14 15:55 UTC (permalink / raw)
To: Mimi Zohar
Cc: keyrings, linux-crypto, linux-integrity, kexec, Philipp Rudo,
Nayna, Rob Herring, linux-s390, Vasily Gorbik,
Lakshmi Ramasubramanian, Heiko Carstens, Jessica Yu, linux-kernel,
David Howells, Christian Borntraeger, Luis Chamberlain,
Paul Mackerras, Hari Bathini, Alexander Gordeev, linuxppc-dev,
Frank van der Linden, Thiago Jung Bauermann, Daniel Axtens,
buendgen, Michael Ellerman, Benjamin Herrenschmidt,
Christian Borntraeger, Herbert Xu, David S. Miller,
Dmitry Kasatkin, James Morris, Serge E. Hallyn, Sven Schnelle,
Baoquan He, linux-security-module
In-Reply-To: <a8d717a44e5e919676e9b1e197cac781db46da87.camel@linux.ibm.com>
Hello,
On Mon, Feb 14, 2022 at 10:14:16AM -0500, Mimi Zohar wrote:
> Hi Michal,
>
> On Sun, 2022-02-13 at 21:59 -0500, Mimi Zohar wrote:
>
> >
> > On Tue, 2022-01-11 at 12:37 +0100, Michal Suchanek wrote:
> > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> > > index dea74d7717c0..1cde9b6c5987 100644
> > > --- a/arch/powerpc/Kconfig
> > > +++ b/arch/powerpc/Kconfig
> > > @@ -560,6 +560,22 @@ config KEXEC_FILE
> > > config ARCH_HAS_KEXEC_PURGATORY
> > > def_bool KEXEC_FILE
> > >
> > > +config KEXEC_SIG
> > > + bool "Verify kernel signature during kexec_file_load() syscall"
> > > + depends on KEXEC_FILE && MODULE_SIG_FORMAT
> > > + help
> > > + This option makes kernel signature verification mandatory for
This is actually wrong. KEXEC_SIG makes it mandatory that any signature
that is appended is valid and made by a key that is part of the platform
keyiring (which is also wrong, built-in keys should be also accepted).
KEXEC_SIG_FORCE or an IMA policy makes it mandatory that the signature
is present.
> > > + the kexec_file_load() syscall.
> >
> > When KEXEC_SIG is enabled on other architectures, IMA does not define a
> > kexec 'appraise' policy rule. Refer to the policy rules in
> > security/ima/ima_efi.c. Similarly the kexec 'appraise' policy rule in
I suppose you mean security/integrity/ima/ima_efi.c
I also think it's misguided because KEXEC_SIG in itself does not enforce
the signature. KEXEC_SIG_FORCE does.
> > arch/powerpc/kernel/ima_policy.c should not be defined.
I suppose you mean arch/powerpc/kernel/ima_arch.c - see above.
Thanks for taking the time to reseach and summarize the differences.
> The discussion shouldn't only be about IMA vs. KEXEC_SIG kernel image
> signature verification. Let's try and reframe the problem a bit.
>
> 1. Unify and simply the existing kexec signature verification so
> verifying the KEXEC kernel image signature works irrespective of
> signature type - PE, appended signature.
>
> solution: enable KEXEC_SIG (This patch set, with the above powerpc IMA
> policy changes.)
>
> 2. Measure and include the kexec kernel image in a log for attestation,
> if desired.
>
> solution: enable IMA_ARCH_POLICY
> - Powerpc: requires trusted boot to be enabled.
> - EFI: requires secure boot to be enabled. The IMA efi policy
> doesn't differentiate between secure and trusted boot.
>
> 3. Carry the kexec kernel image measurement across kexec, if desired
> and supported on the architecture.
>
> solution: enable IMA_KEXEC
>
> Comparison:
> - Are there any differences between IMA vs. KEXEC_SIG measuring the
> kexec kernel image?
>
> One of the main differences is "what" is included in the measurement
> list differs. In both cases, the 'd-ng' field of the IMA measurement
> list template (e.g. ima-ng, ima-sig, ima-modsig) is the full file hash
> including the appended signature. With IMA and the 'ima-modsig'
> template, an additional hash without the appended signature is defined,
> as well as including the appended signature in the 'sig' field.
>
> Including the file hash and appended signature in the measurement list
> allows an attestation server, for example, to verify the appended
> signature without having to know the file hash without the signature.
I don't understand this part. Isn't the hash *with* signature always
included, and the distinguishing part about IMA is the hash *without*
signature which is the same irrespective of signature type (PE, appended
xattr) and irrespective of the keyt used for signoing?
> Other differences are already included in the Kconfig KEXEC_SIG "Notes"
> section.
Which besides what is already described above would be blacklisting
specific binaries, which is much more effective if you have hashes of
binaries without signature.
Thanks
Michal
^ permalink raw reply
* Re: [PATCH v5 2/6] powerpc/kexec_file: Add KEXEC_SIG support.
From: Mimi Zohar @ 2022-02-14 15:14 UTC (permalink / raw)
To: Michal Suchanek, keyrings, linux-crypto, linux-integrity
Cc: kexec, Philipp Rudo, Nayna, Rob Herring, linux-s390,
Vasily Gorbik, Lakshmi Ramasubramanian, Heiko Carstens,
Jessica Yu, linux-kernel, David Howells, Christian Borntraeger,
Luis Chamberlain, Paul Mackerras, Hari Bathini, Alexander Gordeev,
linuxppc-dev, Frank van der Linden, Thiago Jung Bauermann,
Daniel Axtens, buendgen, Michael Ellerman, Benjamin Herrenschmidt,
Christian Borntraeger, Herbert Xu, David S. Miller,
Dmitry Kasatkin, James Morris, Serge E. Hallyn, Sven Schnelle,
Baoquan He, linux-security-module
In-Reply-To: <cff97dbe262919ff709a5ad2c4af6a702cc72a95.camel@linux.ibm.com>
Hi Michal,
On Sun, 2022-02-13 at 21:59 -0500, Mimi Zohar wrote:
>
> On Tue, 2022-01-11 at 12:37 +0100, Michal Suchanek wrote:
> > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> > index dea74d7717c0..1cde9b6c5987 100644
> > --- a/arch/powerpc/Kconfig
> > +++ b/arch/powerpc/Kconfig
> > @@ -560,6 +560,22 @@ config KEXEC_FILE
> > config ARCH_HAS_KEXEC_PURGATORY
> > def_bool KEXEC_FILE
> >
> > +config KEXEC_SIG
> > + bool "Verify kernel signature during kexec_file_load() syscall"
> > + depends on KEXEC_FILE && MODULE_SIG_FORMAT
> > + help
> > + This option makes kernel signature verification mandatory for
> > + the kexec_file_load() syscall.
>
> When KEXEC_SIG is enabled on other architectures, IMA does not define a
> kexec 'appraise' policy rule. Refer to the policy rules in
> security/ima/ima_efi.c. Similarly the kexec 'appraise' policy rule in
> arch/powerpc/kernel/ima_policy.c should not be defined.
The discussion shouldn't only be about IMA vs. KEXEC_SIG kernel image
signature verification. Let's try and reframe the problem a bit.
1. Unify and simply the existing kexec signature verification so
verifying the KEXEC kernel image signature works irrespective of
signature type - PE, appended signature.
solution: enable KEXEC_SIG (This patch set, with the above powerpc IMA
policy changes.)
2. Measure and include the kexec kernel image in a log for attestation,
if desired.
solution: enable IMA_ARCH_POLICY
- Powerpc: requires trusted boot to be enabled.
- EFI: requires secure boot to be enabled. The IMA efi policy
doesn't differentiate between secure and trusted boot.
3. Carry the kexec kernel image measurement across kexec, if desired
and supported on the architecture.
solution: enable IMA_KEXEC
Comparison:
- Are there any differences between IMA vs. KEXEC_SIG measuring the
kexec kernel image?
One of the main differences is "what" is included in the measurement
list differs. In both cases, the 'd-ng' field of the IMA measurement
list template (e.g. ima-ng, ima-sig, ima-modsig) is the full file hash
including the appended signature. With IMA and the 'ima-modsig'
template, an additional hash without the appended signature is defined,
as well as including the appended signature in the 'sig' field.
Including the file hash and appended signature in the measurement list
allows an attestation server, for example, to verify the appended
signature without having to know the file hash without the signature.
Other differences are already included in the Kconfig KEXEC_SIG "Notes"
section.
--
thanks,
Mimi
^ permalink raw reply
* Re: [PATCH v8 07/17] integrity: restrict INTEGRITY_KEYRING_MACHINE to restrict_link_by_ca
From: Darren Kenny @ 2022-02-14 12:42 UTC (permalink / raw)
To: Eric Snowberg, keyrings, linux-integrity, zohar, dhowells, dwmw2,
herbert, davem, jarkko, jmorris, serge
Cc: eric.snowberg, keescook, torvalds, weiyongjun1, nayna, ebiggers,
ardb, nramas, lszubowi, jason, linux-kernel, linux-crypto,
linux-efi, linux-security-module, James.Bottomley, pjones,
konrad.wilk
In-Reply-To: <20211124044124.998170-8-eric.snowberg@oracle.com>
On Tuesday, 2021-11-23 at 23:41:14 -05, Eric Snowberg wrote:
> Set the restriction check for INTEGRITY_KEYRING_MACHINE keys to
> restrict_link_by_ca. This will only allow CA keys into the machine
> keyring.
>
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
> ---
> v1: Initial version
> v2: Added !IS_ENABLED(CONFIG_INTEGRITY_TRUSTED_KEYRING check so mok
> keyring gets created even when it isn't enabled
> v3: Rename restrict_link_by_system_trusted_or_ca to restrict_link_by_ca
> v4: removed unnecessary restriction->check set
> v5: Rename to machine keyring
> v6: split line over 80 char (suggested by Mimi)
> v8: Unmodified from v6
> ---
> security/integrity/digsig.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index 910fe29a5037..e7dfc55a7c55 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -132,14 +132,18 @@ int __init integrity_init_keyring(const unsigned int id)
> goto out;
> }
>
> - if (!IS_ENABLED(CONFIG_INTEGRITY_TRUSTED_KEYRING))
> + if (!IS_ENABLED(CONFIG_INTEGRITY_TRUSTED_KEYRING) &&
> + id != INTEGRITY_KEYRING_MACHINE)
> return 0;
>
> restriction = kzalloc(sizeof(struct key_restriction), GFP_KERNEL);
> if (!restriction)
> return -ENOMEM;
>
> - restriction->check = restrict_link_to_ima;
> + if (id == INTEGRITY_KEYRING_MACHINE)
> + restriction->check = restrict_link_by_ca;
> + else
> + restriction->check = restrict_link_to_ima;
>
> /*
> * No additional keys shall be allowed to load into the machine
> --
> 2.18.4
^ permalink raw reply
* Re: [PATCH v8 17/17] integrity: Only use machine keyring when uefi_check_trust_mok_keys is true
From: Darren Kenny @ 2022-02-14 12:37 UTC (permalink / raw)
To: Eric Snowberg, keyrings, linux-integrity, zohar, dhowells, dwmw2,
herbert, davem, jarkko, jmorris, serge
Cc: eric.snowberg, keescook, torvalds, weiyongjun1, nayna, ebiggers,
ardb, nramas, lszubowi, jason, linux-kernel, linux-crypto,
linux-efi, linux-security-module, James.Bottomley, pjones,
konrad.wilk
In-Reply-To: <20211124044124.998170-18-eric.snowberg@oracle.com>
On Tuesday, 2021-11-23 at 23:41:24 -05, Eric Snowberg wrote:
> With the introduction of uefi_check_trust_mok_keys, it signifies the end-
> user wants to trust the machine keyring as trusted keys. If they have
> chosen to trust the machine keyring, load the qualifying keys into it
> during boot, then link it to the secondary keyring . If the user has not
> chosen to trust the machine keyring, it will be empty and not linked to
> the secondary keyring.
>
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
> ---
> v4: Initial version
> v5: Rename to machine keyring
> v6: Unmodified from v5
> v7: Made trust_mok static
> v8: Unmodified from v7
> ---
> security/integrity/digsig.c | 2 +-
> security/integrity/integrity.h | 5 +++++
> .../integrity/platform_certs/keyring_handler.c | 2 +-
> .../integrity/platform_certs/machine_keyring.c | 16 ++++++++++++++++
> 4 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index 109b58840d45..1de09c7b5f93 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -116,7 +116,7 @@ static int __init __integrity_init_keyring(const unsigned int id,
> } else {
> if (id == INTEGRITY_KEYRING_PLATFORM)
> set_platform_trusted_keys(keyring[id]);
> - if (id == INTEGRITY_KEYRING_MACHINE)
> + if (id == INTEGRITY_KEYRING_MACHINE && trust_moklist())
> set_machine_trusted_keys(keyring[id]);
> if (id == INTEGRITY_KEYRING_IMA)
> load_module_cert(keyring[id]);
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 730771eececd..2e214c761158 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -287,9 +287,14 @@ static inline void __init add_to_platform_keyring(const char *source,
>
> #ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
> void __init add_to_machine_keyring(const char *source, const void *data, size_t len);
> +bool __init trust_moklist(void);
> #else
> static inline void __init add_to_machine_keyring(const char *source,
> const void *data, size_t len)
> {
> }
> +static inline bool __init trust_moklist(void)
> +{
> + return false;
> +}
> #endif
> diff --git a/security/integrity/platform_certs/keyring_handler.c b/security/integrity/platform_certs/keyring_handler.c
> index 4872850d081f..1db4d3b4356d 100644
> --- a/security/integrity/platform_certs/keyring_handler.c
> +++ b/security/integrity/platform_certs/keyring_handler.c
> @@ -83,7 +83,7 @@ __init efi_element_handler_t get_handler_for_db(const efi_guid_t *sig_type)
> __init efi_element_handler_t get_handler_for_mok(const efi_guid_t *sig_type)
> {
> if (efi_guidcmp(*sig_type, efi_cert_x509_guid) == 0) {
> - if (IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING))
> + if (IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING) && trust_moklist())
> return add_to_machine_keyring;
> else
> return add_to_platform_keyring;
> diff --git a/security/integrity/platform_certs/machine_keyring.c b/security/integrity/platform_certs/machine_keyring.c
> index 09fd8f20c756..7aaed7950b6e 100644
> --- a/security/integrity/platform_certs/machine_keyring.c
> +++ b/security/integrity/platform_certs/machine_keyring.c
> @@ -8,6 +8,8 @@
> #include <linux/efi.h>
> #include "../integrity.h"
>
> +static bool trust_mok;
> +
> static __init int machine_keyring_init(void)
> {
> int rc;
> @@ -59,3 +61,17 @@ static __init bool uefi_check_trust_mok_keys(void)
>
> return false;
> }
> +
> +bool __init trust_moklist(void)
> +{
> + static bool initialized;
> +
> + if (!initialized) {
> + initialized = true;
> +
> + if (uefi_check_trust_mok_keys())
> + trust_mok = true;
> + }
> +
> + return trust_mok;
> +}
> --
> 2.18.4
^ permalink raw reply
* Re: [PATCH v8 16/17] integrity: Trust MOK keys if MokListTrustedRT found
From: Darren Kenny @ 2022-02-14 12:31 UTC (permalink / raw)
To: Eric Snowberg, keyrings, linux-integrity, zohar, dhowells, dwmw2,
herbert, davem, jarkko, jmorris, serge
Cc: eric.snowberg, keescook, torvalds, weiyongjun1, nayna, ebiggers,
ardb, nramas, lszubowi, jason, linux-kernel, linux-crypto,
linux-efi, linux-security-module, James.Bottomley, pjones,
konrad.wilk
In-Reply-To: <20211124044124.998170-17-eric.snowberg@oracle.com>
On Tuesday, 2021-11-23 at 23:41:23 -05, Eric Snowberg wrote:
> A new Machine Owner Key (MOK) variable called MokListTrustedRT has been
> introduced in shim. When this UEFI variable is set, it indicates the
> end-user has made the decision themselves that they wish to trust MOK keys
> within the Linux trust boundary. It is not an error if this variable
> does not exist. If it does not exist, the MOK keys should not be trusted
> within the kernel.
>
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
> ---
> v1: Initial version
> v2: Removed mok_keyring_trust_setup function
> v4: Unmodified from v2
> v5: Rename to machine keyring
> v6: Unmodified from v5
> v7: Use mokvar table instead of EFI var (suggested by Peter Jones)
> v8: Unmodified from v7
> ---
> .../platform_certs/machine_keyring.c | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/security/integrity/platform_certs/machine_keyring.c b/security/integrity/platform_certs/machine_keyring.c
> index ea2ac2f9f2b5..09fd8f20c756 100644
> --- a/security/integrity/platform_certs/machine_keyring.c
> +++ b/security/integrity/platform_certs/machine_keyring.c
> @@ -5,6 +5,7 @@
> * Copyright (c) 2021, Oracle and/or its affiliates.
> */
>
> +#include <linux/efi.h>
> #include "../integrity.h"
>
> static __init int machine_keyring_init(void)
> @@ -40,3 +41,21 @@ void __init add_to_machine_keyring(const char *source, const void *data, size_t
> if (rc)
> pr_info("Error adding keys to machine keyring %s\n", source);
> }
> +
> +/*
> + * Try to load the MokListTrustedRT MOK variable to see if we should trust
> + * the MOK keys within the kernel. It is not an error if this variable
> + * does not exist. If it does not exist, MOK keys should not be trusted
> + * within the machine keyring.
> + */
> +static __init bool uefi_check_trust_mok_keys(void)
> +{
> + struct efi_mokvar_table_entry *mokvar_entry;
> +
> + mokvar_entry = efi_mokvar_entry_find("MokListTrustedRT");
> +
> + if (mokvar_entry)
> + return true;
> +
> + return false;
> +}
> --
> 2.18.4
^ permalink raw reply
* Re: [PATCH v8 15/17] efi/mokvar: move up init order
From: Darren Kenny @ 2022-02-14 12:29 UTC (permalink / raw)
To: Eric Snowberg, keyrings, linux-integrity, zohar, dhowells, dwmw2,
herbert, davem, jarkko, jmorris, serge
Cc: eric.snowberg, keescook, torvalds, weiyongjun1, nayna, ebiggers,
ardb, nramas, lszubowi, jason, linux-kernel, linux-crypto,
linux-efi, linux-security-module, James.Bottomley, pjones,
konrad.wilk
In-Reply-To: <20211124044124.998170-16-eric.snowberg@oracle.com>
On Tuesday, 2021-11-23 at 23:41:22 -05, Eric Snowberg wrote:
> Move up the init order so it can be used by the new machine keyring.
>
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
> ---
> v7: Initial version
> v8: Unmodified from v7
> ---
> drivers/firmware/efi/mokvar-table.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/mokvar-table.c b/drivers/firmware/efi/mokvar-table.c
> index 38722d2009e2..5ed0602c2f75 100644
> --- a/drivers/firmware/efi/mokvar-table.c
> +++ b/drivers/firmware/efi/mokvar-table.c
> @@ -359,4 +359,4 @@ static int __init efi_mokvar_sysfs_init(void)
> }
> return err;
> }
> -device_initcall(efi_mokvar_sysfs_init);
> +fs_initcall(efi_mokvar_sysfs_init);
> --
> 2.18.4
^ permalink raw reply
* Re: [PATCH v8 14/17] KEYS: link machine trusted keys to secondary_trusted_keys
From: Darren Kenny @ 2022-02-14 12:28 UTC (permalink / raw)
To: Eric Snowberg, keyrings, linux-integrity, zohar, dhowells, dwmw2,
herbert, davem, jarkko, jmorris, serge
Cc: eric.snowberg, keescook, torvalds, weiyongjun1, nayna, ebiggers,
ardb, nramas, lszubowi, jason, linux-kernel, linux-crypto,
linux-efi, linux-security-module, James.Bottomley, pjones,
konrad.wilk
In-Reply-To: <20211124044124.998170-15-eric.snowberg@oracle.com>
On Tuesday, 2021-11-23 at 23:41:21 -05, Eric Snowberg wrote:
> Allow the .machine keyring to be linked to the secondary_trusted_keys.
> After the link is created, keys contained in the .machine keyring will
> automatically be searched when searching secondary_trusted_keys.
>
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
> ---
> v3: Initial version
> v4: Unmodified from v3
> v5: Rename to machine keyring
> v7: Unmodified from v5
> v8: Change patch subject name, code unmodified from v7
> ---
> certs/system_keyring.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index 07f410918e62..463f676857f0 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -101,6 +101,9 @@ static __init struct key_restriction *get_secondary_restriction(void)
> void __init set_machine_trusted_keys(struct key *keyring)
> {
> machine_trusted_keys = keyring;
> +
> + if (key_link(secondary_trusted_keys, machine_trusted_keys) < 0)
> + panic("Can't link (machine) trusted keyrings\n");
> }
>
> /**
> --
> 2.18.4
^ permalink raw reply
* Re: [PATCH v8 13/17] integrity: store reference to machine keyring
From: Darren Kenny @ 2022-02-14 12:27 UTC (permalink / raw)
To: Eric Snowberg, keyrings, linux-integrity, zohar, dhowells, dwmw2,
herbert, davem, jarkko, jmorris, serge
Cc: eric.snowberg, keescook, torvalds, weiyongjun1, nayna, ebiggers,
ardb, nramas, lszubowi, jason, linux-kernel, linux-crypto,
linux-efi, linux-security-module, James.Bottomley, pjones,
konrad.wilk
In-Reply-To: <20211124044124.998170-14-eric.snowberg@oracle.com>
On Tuesday, 2021-11-23 at 23:41:20 -05, Eric Snowberg wrote:
> Store a reference to the machine keyring in system keyring code. The
> system keyring code needs this to complete the keyring link to
> to machine keyring.
>
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
> ---
> v2: Initial version
> v3: Unmodified from v2
> v4: Removed trust_moklist check
> v5: Rename to machine keyring
> v8: Unmodified from v5
> ---
> security/integrity/digsig.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index 74f73f7cc4fe..109b58840d45 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -116,6 +116,8 @@ static int __init __integrity_init_keyring(const unsigned int id,
> } else {
> if (id == INTEGRITY_KEYRING_PLATFORM)
> set_platform_trusted_keys(keyring[id]);
> + if (id == INTEGRITY_KEYRING_MACHINE)
> + set_machine_trusted_keys(keyring[id]);
> if (id == INTEGRITY_KEYRING_IMA)
> load_module_cert(keyring[id]);
> }
> --
> 2.18.4
^ permalink raw reply
* Re: [PATCH v8 11/17] KEYS: Introduce link restriction for machine keys
From: Darren Kenny @ 2022-02-14 12:23 UTC (permalink / raw)
To: Eric Snowberg, keyrings, linux-integrity, zohar, dhowells, dwmw2,
herbert, davem, jarkko, jmorris, serge
Cc: eric.snowberg, keescook, torvalds, weiyongjun1, nayna, ebiggers,
ardb, nramas, lszubowi, jason, linux-kernel, linux-crypto,
linux-efi, linux-security-module, James.Bottomley, pjones,
konrad.wilk
In-Reply-To: <20211124044124.998170-12-eric.snowberg@oracle.com>
On Tuesday, 2021-11-23 at 23:41:18 -05, Eric Snowberg wrote:
> Introduce a new link restriction that includes the trusted builtin,
> secondary and machine keys. The restriction is based on the key to be
> added being vouched for by a key in any of these three keyrings.
>
> Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
> ---
> v3: Initial version
> v4: moved code under CONFIG_INTEGRITY_MOK_KEYRING
> v5: Rename to machine keyring
> v6: Change subject name (suggested by Mimi)
> Rename restrict_link_by_builtin_secondary_and_ca_trusted
> to restrict_link_by_builtin_secondary_and_machine (suggested by
> Mimi)
> v7: Unmodified from v6
> v8: Add missing parameter definitions (suggested by Mimi)
> ---
> certs/system_keyring.c | 27 +++++++++++++++++++++++++++
> include/keys/system_keyring.h | 6 ++++++
> 2 files changed, 33 insertions(+)
>
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index bc7e44fc82c2..8a2fd1dc15db 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -99,6 +99,33 @@ void __init set_machine_trusted_keys(struct key *keyring)
> {
> machine_trusted_keys = keyring;
> }
> +
> +/**
> + * restrict_link_by_builtin_secondary_and_machine - Restrict keyring addition.
> + * @dest_keyring: Keyring being linked to.
> + * @type: The type of key being added.
> + * @payload: The payload of the new key.
> + * @restrict_key: A ring of keys that can be used to vouch for the new cert.
> + *
> + * Restrict the addition of keys into a keyring based on the key-to-be-added
> + * being vouched for by a key in either the built-in, the secondary, or
> + * the machine keyrings.
> + */
> +int restrict_link_by_builtin_secondary_and_machine(
> + struct key *dest_keyring,
> + const struct key_type *type,
> + const union key_payload *payload,
> + struct key *restrict_key)
> +{
> + if (machine_trusted_keys && type == &key_type_keyring &&
> + dest_keyring == secondary_trusted_keys &&
> + payload == &machine_trusted_keys->payload)
> + /* Allow the machine keyring to be added to the secondary */
> + return 0;
> +
> + return restrict_link_by_builtin_and_secondary_trusted(dest_keyring, type,
> + payload, restrict_key);
> +}
> #endif
>
> /*
> diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
> index 98c9b10cdc17..2419a735420f 100644
> --- a/include/keys/system_keyring.h
> +++ b/include/keys/system_keyring.h
> @@ -39,8 +39,14 @@ extern int restrict_link_by_builtin_and_secondary_trusted(
> #endif
>
> #ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
> +extern int restrict_link_by_builtin_secondary_and_machine(
> + struct key *dest_keyring,
> + const struct key_type *type,
> + const union key_payload *payload,
> + struct key *restrict_key);
> extern void __init set_machine_trusted_keys(struct key *keyring);
> #else
> +#define restrict_link_by_builtin_secondary_and_machine restrict_link_by_builtin_trusted
> static inline void __init set_machine_trusted_keys(struct key *keyring)
> {
> }
> --
> 2.18.4
^ permalink raw reply
* Re: [PATCH v8 10/17] KEYS: add a reference to machine keyring
From: Darren Kenny @ 2022-02-14 12:18 UTC (permalink / raw)
To: Eric Snowberg, keyrings, linux-integrity, zohar, dhowells, dwmw2,
herbert, davem, jarkko, jmorris, serge
Cc: eric.snowberg, keescook, torvalds, weiyongjun1, nayna, ebiggers,
ardb, nramas, lszubowi, jason, linux-kernel, linux-crypto,
linux-efi, linux-security-module, James.Bottomley, pjones,
konrad.wilk
In-Reply-To: <20211124044124.998170-11-eric.snowberg@oracle.com>
On Tuesday, 2021-11-23 at 23:41:17 -05, Eric Snowberg wrote:
> Expose the .machine keyring created in integrity code by adding
> a reference. This makes the machine keyring accessible for keyring
> restrictions in the future.
>
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
> ---
> v2: Initial version
> v3: set_mok_trusted_keys only available when secondary is enabled
> v4: Moved code under CONFIG_INTEGRITY_MOK_KEYRING
> v5: Rename to machine keyring
> v8: Unmodified from v5
> ---
> certs/system_keyring.c | 9 +++++++++
> include/keys/system_keyring.h | 8 ++++++++
> 2 files changed, 17 insertions(+)
>
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index 8f1f87579819..bc7e44fc82c2 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -22,6 +22,9 @@ static struct key *builtin_trusted_keys;
> #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> static struct key *secondary_trusted_keys;
> #endif
> +#ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
> +static struct key *machine_trusted_keys;
> +#endif
> #ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
> static struct key *platform_trusted_keys;
> #endif
> @@ -91,6 +94,12 @@ static __init struct key_restriction *get_secondary_restriction(void)
> return restriction;
> }
> #endif
> +#ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
> +void __init set_machine_trusted_keys(struct key *keyring)
> +{
> + machine_trusted_keys = keyring;
> +}
> +#endif
>
> /*
> * Create the trusted keyrings
> diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
> index 6acd3cf13a18..98c9b10cdc17 100644
> --- a/include/keys/system_keyring.h
> +++ b/include/keys/system_keyring.h
> @@ -38,6 +38,14 @@ extern int restrict_link_by_builtin_and_secondary_trusted(
> #define restrict_link_by_builtin_and_secondary_trusted restrict_link_by_builtin_trusted
> #endif
>
> +#ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
> +extern void __init set_machine_trusted_keys(struct key *keyring);
> +#else
> +static inline void __init set_machine_trusted_keys(struct key *keyring)
> +{
> +}
> +#endif
> +
> extern struct pkcs7_message *pkcs7;
> #ifdef CONFIG_SYSTEM_BLACKLIST_KEYRING
> extern int mark_hash_blacklisted(const char *hash);
> --
> 2.18.4
^ 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