| Message ID | 20220302134703.1273041-24-stefanb@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ima: Namespace IMA with audit support in IMA-ns | expand |
Hi Stefan,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v5.17-rc6]
[cannot apply to zohar-integrity/next-integrity linux/master jmorris-security/next-testing next-20220302]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Stefan-Berger/ima-Namespace-IMA-with-audit-support-in-IMA-ns/20220302-215707
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git fb184c4af9b9f4563e7a126219389986a71d5b5b
config: arm64-randconfig-r006-20220302 (https://download.01.org/0day-ci/archive/20220303/202203030340.kolQS5ma-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project d271fc04d5b97b12e6b797c6067d3c96a8d7470e)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
# https://github.com/0day-ci/linux/commit/59a9ba1130510d6693a61c6eb84c29983fa696df
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Stefan-Berger/ima-Namespace-IMA-with-audit-support-in-IMA-ns/20220302-215707
git checkout 59a9ba1130510d6693a61c6eb84c29983fa696df
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash security/integrity/ima/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> security/integrity/ima/ima_fs.c:591:3: warning: variable 'ret' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
if (IS_ERR(active))
^~~~~~~~~~~~~~~~~~~
include/linux/compiler.h:56:28: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/compiler.h:58:30: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
security/integrity/ima/ima_fs.c:608:9: note: uninitialized use occurs here
return ret;
^~~
security/integrity/ima/ima_fs.c:591:3: note: remove the 'if' if its condition is always false
if (IS_ERR(active))
^~~~~~~~~~~~~~~~~~~
include/linux/compiler.h:56:23: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^
security/integrity/ima/ima_fs.c:516:9: note: initialize the variable 'ret' to silence this warning
int ret;
^
= 0
1 warning generated.
vim +591 security/integrity/ima/ima_fs.c
504
505 int ima_fs_ns_init(struct user_namespace *user_ns, struct dentry *root)
506 {
507 struct ima_namespace *ns = ima_ns_from_user_ns(user_ns);
508 struct dentry *int_dir;
509 struct dentry *ima_dir = NULL;
510 struct dentry *ima_symlink = NULL;
511 struct dentry *binary_runtime_measurements = NULL;
512 struct dentry *ascii_runtime_measurements = NULL;
513 struct dentry *runtime_measurements_count = NULL;
514 struct dentry *violations = NULL;
515 struct dentry *active = NULL;
516 int ret;
517
518 /* FIXME: update when evm and integrity are namespaced */
519 if (user_ns != &init_user_ns) {
520 int_dir = securityfs_create_dir("integrity", root);
521 if (IS_ERR(int_dir))
522 return PTR_ERR(int_dir);
523 } else {
524 int_dir = integrity_dir;
525 }
526
527 ima_dir = securityfs_create_dir("ima", int_dir);
528 if (IS_ERR(ima_dir)) {
529 ret = PTR_ERR(ima_dir);
530 goto out;
531 }
532
533 ima_symlink = securityfs_create_symlink("ima", root, "integrity/ima",
534 NULL);
535 if (IS_ERR(ima_symlink)) {
536 ret = PTR_ERR(ima_symlink);
537 goto out;
538 }
539
540 binary_runtime_measurements =
541 securityfs_create_file("binary_runtime_measurements",
542 S_IRUSR | S_IRGRP, ima_dir, NULL,
543 &ima_measurements_ops);
544 if (IS_ERR(binary_runtime_measurements)) {
545 ret = PTR_ERR(binary_runtime_measurements);
546 goto out;
547 }
548
549 ascii_runtime_measurements =
550 securityfs_create_file("ascii_runtime_measurements",
551 S_IRUSR | S_IRGRP, ima_dir, NULL,
552 &ima_ascii_measurements_ops);
553 if (IS_ERR(ascii_runtime_measurements)) {
554 ret = PTR_ERR(ascii_runtime_measurements);
555 goto out;
556 }
557
558 runtime_measurements_count =
559 securityfs_create_file("runtime_measurements_count",
560 S_IRUSR | S_IRGRP, ima_dir, NULL,
561 &ima_measurements_count_ops);
562 if (IS_ERR(runtime_measurements_count)) {
563 ret = PTR_ERR(runtime_measurements_count);
564 goto out;
565 }
566
567 violations =
568 securityfs_create_file("violations", S_IRUSR | S_IRGRP,
569 ima_dir, NULL, &ima_htable_violations_ops);
570 if (IS_ERR(violations)) {
571 ret = PTR_ERR(violations);
572 goto out;
573 }
574
575 if (!ns->ima_policy_removed) {
576 ns->ima_policy =
577 securityfs_create_file("policy", POLICY_FILE_FLAGS,
578 ima_dir, NULL,
579 &ima_measure_policy_ops);
580 if (IS_ERR(ns->ima_policy)) {
581 ret = PTR_ERR(ns->ima_policy);
582 goto out;
583 }
584 }
585
586 if (ns != &init_ima_ns) {
587 active =
588 securityfs_create_file("active",
589 S_IRUSR | S_IWUSR | S_IRGRP, ima_dir,
590 NULL, &ima_active_ops);
> 591 if (IS_ERR(active))
592 goto out;
593 }
594
595 return 0;
596 out:
597 securityfs_remove(active);
598 securityfs_remove(ns->ima_policy);
599 securityfs_remove(violations);
600 securityfs_remove(runtime_measurements_count);
601 securityfs_remove(ascii_runtime_measurements);
602 securityfs_remove(binary_runtime_measurements);
603 securityfs_remove(ima_symlink);
604 securityfs_remove(ima_dir);
605 if (user_ns != &init_user_ns)
606 securityfs_remove(int_dir);
607
608 return ret;
609 }
610
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 801dc3c8bfde..2cc286f9e839 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -126,6 +126,7 @@ struct ima_namespace { unsigned long ima_ns_flags; /* Bit numbers for above flags; use BIT() to get flag */ #define IMA_NS_LSM_UPDATE_RULES 0 +#define IMA_NS_ACTIVE 1 struct rb_root ns_status_tree; rwlock_t ns_tree_lock; @@ -158,6 +159,11 @@ struct ima_namespace { } __randomize_layout; extern struct ima_namespace init_ima_ns; +static inline bool ns_is_active(struct ima_namespace *ns) +{ + return (ns && test_bit(IMA_NS_ACTIVE, &ns->ima_ns_flags)); +} + extern const int read_idmap[]; #ifdef CONFIG_HAVE_IMA_KEXEC diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c index 84cd02a9e19b..8254c4e683d5 100644 --- a/security/integrity/ima/ima_fs.c +++ b/security/integrity/ima/ima_fs.c @@ -451,6 +451,57 @@ static const struct file_operations ima_measure_policy_ops = { .llseek = generic_file_llseek, }; +static ssize_t ima_show_active(struct file *filp, + char __user *buf, + size_t count, loff_t *ppos) +{ + struct ima_namespace *ns = &init_ima_ns; + char tmpbuf[2]; + ssize_t len; + + len = scnprintf(tmpbuf, sizeof(tmpbuf), + "%d\n", !!test_bit(IMA_NS_ACTIVE, &ns->ima_ns_flags)); + return simple_read_from_buffer(buf, count, ppos, tmpbuf, len); +} + +static ssize_t ima_write_active(struct file *filp, + const char __user *buf, + size_t count, loff_t *ppos) +{ + struct ima_namespace *ns = &init_ima_ns; + unsigned int active; + char *kbuf; + int err; + + if (ns_is_active(ns)) + return -EBUSY; + + /* accepting '1\n' and '1\0' and no partial writes */ + if (count >= 3 || *ppos != 0) + return -EINVAL; + + kbuf = memdup_user_nul(buf, count); + if (IS_ERR(kbuf)) + return PTR_ERR(kbuf); + + err = kstrtouint(kbuf, 10, &active); + kfree(kbuf); + if (err) + return err; + + if (active != 1) + return -EINVAL; + + set_bit(IMA_NS_ACTIVE, &ns->ima_ns_flags); + + return count; +} + +static const struct file_operations ima_active_ops = { + .read = ima_show_active, + .write = ima_write_active, +}; + int ima_fs_ns_init(struct user_namespace *user_ns, struct dentry *root) { struct ima_namespace *ns = ima_ns_from_user_ns(user_ns); @@ -461,6 +512,7 @@ int ima_fs_ns_init(struct user_namespace *user_ns, struct dentry *root) struct dentry *ascii_runtime_measurements = NULL; struct dentry *runtime_measurements_count = NULL; struct dentry *violations = NULL; + struct dentry *active = NULL; int ret; /* FIXME: update when evm and integrity are namespaced */ @@ -531,8 +583,18 @@ int ima_fs_ns_init(struct user_namespace *user_ns, struct dentry *root) } } + if (ns != &init_ima_ns) { + active = + securityfs_create_file("active", + S_IRUSR | S_IWUSR | S_IRGRP, ima_dir, + NULL, &ima_active_ops); + if (IS_ERR(active)) + goto out; + } + return 0; out: + securityfs_remove(active); securityfs_remove(ns->ima_policy); securityfs_remove(violations); securityfs_remove(runtime_measurements_count); diff --git a/security/integrity/ima/ima_init_ima_ns.c b/security/integrity/ima/ima_init_ima_ns.c index 41e7db0c9749..5c57abfc70ea 100644 --- a/security/integrity/ima/ima_init_ima_ns.c +++ b/security/integrity/ima/ima_init_ima_ns.c @@ -58,5 +58,6 @@ struct ima_namespace init_ima_ns = { .ima_lsm_policy_notifier = { .notifier_call = ima_lsm_policy_change, }, + .ima_ns_flags = BIT(IMA_NS_ACTIVE), }; EXPORT_SYMBOL(init_ima_ns); diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 720b51180b00..fa12080b8b94 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -441,7 +441,7 @@ static int process_measurement(struct user_namespace *user_ns, while (user_ns) { ns = ima_ns_from_user_ns(user_ns); - if (ns) { + if (ns_is_active(ns)) { int rc; rc = __process_measurement(ns, file, cred, secid, buf,
Introduce the IMA securityfs file 'active' that users now need to write a "1" into (precisely a '1\0' or '1\n') to activate an IMA namespace. When reading from the file, it shows either '0' or '1'. The rationale for introducing a file to activate an IMA namespace is that subsequent support for IMA-measurement and IMA-appraisal will add configuration files for selecting for example the template that an IMA namespace is supposed to use for logging measurements, which will add an IMA namespace configuration stage (using securityfs files) before its activation. Besides that it allows to create a user namespace with securityfs mounted and an inactive IMA namespace. Also, introduce ns_is_active() to be used in those places where the ima_namespace pointer may either be NULL or where the active flag may not have been set, yet. An inactive IMA namespace should never be accessed since it has not been initialized, yet. Set the init_ima_ns's active flag since it is considered active right from the beginning. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- v11: - Move code from ima_fs_add_ns_files() into ima_fs_ns_init() - Use ima_ns_flags and IMA_NS_ACTIVE instead of 'atomic_t active' v10: - use memdup_user_nul to copy input from user - propagating error returned from ima_fs_add_ns_files() --- security/integrity/ima/ima.h | 6 +++ security/integrity/ima/ima_fs.c | 62 ++++++++++++++++++++++++ security/integrity/ima/ima_init_ima_ns.c | 1 + security/integrity/ima/ima_main.c | 2 +- 4 files changed, 70 insertions(+), 1 deletion(-)