[v9,02/11] LSM: Maintain a table of LSM attribute data
Commit Message
As LSMs are registered add their lsm_id pointers to a table.
This will be used later for attribute reporting.
Determine the number of possible security modules based on
their respective CONFIG options. This allows the number to be
known at build time. This allows data structures and tables
to use the constant.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
include/linux/security.h | 2 ++
security/security.c | 43 ++++++++++++++++++++++++++++++++--------
2 files changed, 37 insertions(+), 8 deletions(-)
Comments
On Fri, Apr 21, 2023 at 10:42:50AM -0700, Casey Schaufler wrote:
> As LSMs are registered add their lsm_id pointers to a table.
> This will be used later for attribute reporting.
>
> Determine the number of possible security modules based on
> their respective CONFIG options. This allows the number to be
> known at build time. This allows data structures and tables
> to use the constant.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Nit below...
> [...]
> @@ -513,6 +531,15 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
> {
> int i;
>
> + if (lsm_active_cnt >= LSM_COUNT)
> + panic("%s Too many LSMs registered.\n", __func__);
> + /*
> + * A security module may call security_add_hooks() more
> + * than once. Landlock is one such case.
> + */
> + if (lsm_active_cnt == 0 || lsm_idlist[lsm_active_cnt - 1] != lsmid)
> + lsm_idlist[lsm_active_cnt++] = lsmid;
> +
I find this logic hard to parse. I think this might be better, since
lsm_idlist will be entirely initialized to LSM_UNDEF, yes?
/*
* A security module may call security_add_hooks() more
* than once during initialization, and LSM initialization
* is serialized. Landlock is one such case.
*/
if (lsm_idlist[lsm_active_cnt] != lsmid)
lsm_idlist[lsm_active_cnt++] = lsmid;
Hello,
kernel test robot noticed "WARNING:at_security/security.c:#append_ordered_lsm" on:
commit: 9898687125b05a8db163332bfa897796e6044da7 ("[PATCH v9 02/11] LSM: Maintain a table of LSM attribute data")
url: https://github.com/intel-lab-lkp/linux/commits/Casey-Schaufler/LSM-Maintain-a-table-of-LSM-attribute-data/20230422-024331
base: https://git.kernel.org/cgit/linux/kernel/git/tip/tip.git 15def34e2635ab7e0e96f1bc32e1b69609f14942
patch link: https://lore.kernel.org/all/20230421174259.2458-3-casey@schaufler-ca.com/
patch subject: [PATCH v9 02/11] LSM: Maintain a table of LSM attribute data
in testcase: phoronix-test-suite
version:
with following parameters:
need_x: true
test: jxrendermark-1.2.4
option_a: Transformed Blit Linear
option_b: 32x32
cpufreq_governor: performance
test-description: The Phoronix Test Suite is the most comprehensive testing and benchmarking platform available that provides an extensible framework for which new tests can be easily added.
test-url: http://www.phoronix-test-suite.com/
compiler: gcc-11
test machine: 12 threads 1 sockets Intel(R) Core(TM) i7-8700 CPU @ 3.20GHz (Coffee Lake) with 32G memory
(please refer to attached dmesg/kmsg for entire log/backtrace)
If you fix the issue, kindly add following tag
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Link: https://lore.kernel.org/oe-lkp/202304231519.c7ec2a3f-oliver.sang@intel.com
[ 2.554563][ T0] ------------[ cut here ]------------
[ 2.554563][ T0] builtin: out of LSM slots!?
[ 2.554563][ T0] WARNING: CPU: 0 PID: 0 at security/security.c:173 append_ordered_lsm (security/security.c:173 (discriminator 1))
[ 2.554563][ T0] Modules linked in:
[ 2.554563][ T0] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 6.3.0-rc3-00006-g9898687125b0 #1
[ 2.554563][ T0] Hardware name: Dell Inc. OptiPlex 7060/0C96W1, BIOS 1.4.2 06/11/2019
[ 2.554563][ T0] RIP: 0010:append_ordered_lsm (security/security.c:173 (discriminator 1))
[ 2.554563][ T0] Code: 82 48 0f 45 c8 48 8b 17 48 c7 c7 70 a4 67 82 e9 34 03 e4 fd 48 c7 47 18 88 0c 49 83 eb ae 48 c7 c7 58 a4 67 82 e8 de 97 da fd <0f> 0b e9 cb 34 bd fe 48 c7 c1 8c 7e 65 82 eb cb 66 66 2e 0f 1f 84
All code
========
0: 82 (bad)
1: 48 0f 45 c8 cmovne %rax,%rcx
5: 48 8b 17 mov (%rdi),%rdx
8: 48 c7 c7 70 a4 67 82 mov $0xffffffff8267a470,%rdi
f: e9 34 03 e4 fd jmpq 0xfffffffffde40348
14: 48 c7 47 18 88 0c 49 movq $0xffffffff83490c88,0x18(%rdi)
1b: 83
1c: eb ae jmp 0xffffffffffffffcc
1e: 48 c7 c7 58 a4 67 82 mov $0xffffffff8267a458,%rdi
25: e8 de 97 da fd callq 0xfffffffffdda9808
2a:* 0f 0b ud2 <-- trapping instruction
2c: e9 cb 34 bd fe jmpq 0xfffffffffebd34fc
31: 48 c7 c1 8c 7e 65 82 mov $0xffffffff82657e8c,%rcx
38: eb cb jmp 0x5
3a: 66 data16
3b: 66 data16
3c: 2e cs
3d: 0f .byte 0xf
3e: 1f (bad)
3f: 84 .byte 0x84
Code starting with the faulting instruction
===========================================
0: 0f 0b ud2
2: e9 cb 34 bd fe jmpq 0xfffffffffebd34d2
7: 48 c7 c1 8c 7e 65 82 mov $0xffffffff82657e8c,%rcx
e: eb cb jmp 0xffffffffffffffdb
10: 66 data16
11: 66 data16
12: 2e cs
13: 0f .byte 0xf
14: 1f (bad)
15: 84 .byte 0x84
[ 2.554563][ T0] RSP: 0000:ffffffff82803eb0 EFLAGS: 00010286
[ 2.554563][ T0] RAX: 0000000000000000 RBX: ffffffff8353be48 RCX: c0000000ffff7fff
[ 2.554563][ T0] RDX: 0000000000000000 RSI: 0000000000027ffb RDI: 0000000000000001
[ 2.554563][ T0] RBP: ffffffff8265acb1 R08: 0000000000000000 R09: 00000000ffff7fff
[ 2.554563][ T0] R10: ffffffff82803d50 R11: ffffffff82bd8d88 R12: ffff888100192300
[ 2.554563][ T0] R13: ffffffff8353bde8 R14: ffff888100192333 R15: 0000000000000001
[ 2.554563][ T0] FS: 0000000000000000(0000) GS:ffff888853e00000(0000) knlGS:0000000000000000
[ 2.554563][ T0] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 2.554563][ T0] CR2: ffff888000000413 CR3: 000000087b418001 CR4: 00000000000606b0
[ 2.554563][ T0] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 2.554563][ T0] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 2.554563][ T0] Call Trace:
[ 2.554563][ T0] <TASK>
[ 2.554563][ T0] ordered_lsm_parse (security/security.c:303)
[ 2.554563][ T0] ordered_lsm_init (security/security.c:379)
[ 2.554563][ T0] security_init (security/security.c:459)
[ 2.554563][ T0] start_kernel (init/main.c:1129)
[ 2.554563][ T0] secondary_startup_64_no_verify (arch/x86/kernel/head_64.S:358)
[ 2.554563][ T0] </TASK>
[ 2.554563][ T0] ---[ end trace 0000000000000000 ]---
[ 2.554563][ T0] LSM: initializing lsm=capability,yama,integrity
[ 2.554563][ T0] Yama: becoming mindful.
[ 2.554563][ T0] Mount-cache hash table entries: 65536 (order: 7, 524288 bytes, linear)
[ 2.554563][ T0] Mountpoint-cache hash table entries: 65536 (order: 7, 524288 bytes, linear)
[ 2.554563][ T0] CPU0: Thermal monitoring enabled (TM1)
[ 2.554563][ T0] process: using mwait in idle threads
[ 2.554563][ T0] Last level iTLB entries: 4KB 64, 2MB 8, 4MB 8
[ 2.554563][ T0] Last level dTLB entries: 4KB 64, 2MB 0, 4MB 0, 1GB 4
To reproduce:
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
sudo bin/lkp install job.yaml # job file is attached in this email
bin/lkp split-job --compatible job.yaml # generate the yaml file for lkp run
sudo bin/lkp run generated-yaml-file
# if come across any failure that blocks the test,
# please remove ~/.lkp and /lkp dir to run from a clean state.
On 4/21/2023 12:20 PM, Kees Cook wrote:
> On Fri, Apr 21, 2023 at 10:42:50AM -0700, Casey Schaufler wrote:
>> As LSMs are registered add their lsm_id pointers to a table.
>> This will be used later for attribute reporting.
>>
>> Determine the number of possible security modules based on
>> their respective CONFIG options. This allows the number to be
>> known at build time. This allows data structures and tables
>> to use the constant.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> Nit below...
>
>> [...]
>> @@ -513,6 +531,15 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
>> {
>> int i;
>>
>> + if (lsm_active_cnt >= LSM_COUNT)
>> + panic("%s Too many LSMs registered.\n", __func__);
>> + /*
>> + * A security module may call security_add_hooks() more
>> + * than once. Landlock is one such case.
>> + */
>> + if (lsm_active_cnt == 0 || lsm_idlist[lsm_active_cnt - 1] != lsmid)
>> + lsm_idlist[lsm_active_cnt++] = lsmid;
>> +
> I find this logic hard to parse. I think this might be better, since
> lsm_idlist will be entirely initialized to LSM_UNDEF, yes?
>
> /*
> * A security module may call security_add_hooks() more
> * than once during initialization, and LSM initialization
> * is serialized. Landlock is one such case.
> */
> if (lsm_idlist[lsm_active_cnt] != lsmid)
> lsm_idlist[lsm_active_cnt++] = lsmid;
This code won't do the job. lsm_active_count indexes the first unset
entry, not the last set entry.
@@ -138,6 +138,8 @@ enum lockdown_reason {
};
extern const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1];
+extern u32 lsm_active_cnt;
+extern struct lsm_id *lsm_idlist[];
/* These functions are in security/commoncap.c */
extern int cap_capable(const struct cred *cred, struct user_namespace *ns,
@@ -28,12 +28,29 @@
#include <linux/backing-dev.h>
#include <linux/string.h>
#include <linux/msg.h>
+#include <uapi/linux/lsm.h>
#include <net/flow.h>
#define MAX_LSM_EVM_XATTR 2
-/* How many LSMs were built into the kernel? */
-#define LSM_COUNT (__end_lsm_info - __start_lsm_info)
+/*
+ * How many LSMs are built into the kernel as determined at
+ * build time. Used to determine fixed array sizes.
+ * The capability module is accounted for by CONFIG_SECURITY
+ */
+#define LSM_COUNT ( \
+ (IS_ENABLED(CONFIG_SECURITY) ? 1 : 0) + \
+ (IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \
+ (IS_ENABLED(CONFIG_SECURITY_SMACK) ? 1 : 0) + \
+ (IS_ENABLED(CONFIG_SECURITY_TOMOYO) ? 1 : 0) + \
+ (IS_ENABLED(CONFIG_IMA) ? 1 : 0) + \
+ (IS_ENABLED(CONFIG_SECURITY_APPARMOR) ? 1 : 0) + \
+ (IS_ENABLED(CONFIG_SECURITY_YAMA) ? 1 : 0) + \
+ (IS_ENABLED(CONFIG_SECURITY_LOADPIN) ? 1 : 0) + \
+ (IS_ENABLED(CONFIG_SECURITY_SAFESETID) ? 1 : 0) + \
+ (IS_ENABLED(CONFIG_SECURITY_LOCKDOWN_LSM) ? 1 : 0) + \
+ (IS_ENABLED(CONFIG_BPF_LSM) ? 1 : 0) + \
+ (IS_ENABLED(CONFIG_SECURITY_LANDLOCK) ? 1 : 0))
/*
* These are descriptions of the reasons that can be passed to the
@@ -90,7 +107,7 @@ static __initdata const char *chosen_major_lsm;
static __initconst const char * const builtin_lsm_order = CONFIG_LSM;
/* Ordered list of LSMs to initialize. */
-static __initdata struct lsm_info **ordered_lsms;
+static __initdata struct lsm_info *ordered_lsms[LSM_COUNT + 1];
static __initdata struct lsm_info *exclusive;
static __initdata bool debug;
@@ -341,13 +358,16 @@ static void __init report_lsm_order(void)
pr_cont("\n");
}
+/*
+ * Current index to use while initializing the lsm id list.
+ */
+u32 lsm_active_cnt __lsm_ro_after_init;
+struct lsm_id *lsm_idlist[LSM_COUNT] __lsm_ro_after_init;
+
static void __init ordered_lsm_init(void)
{
struct lsm_info **lsm;
- ordered_lsms = kcalloc(LSM_COUNT + 1, sizeof(*ordered_lsms),
- GFP_KERNEL);
-
if (chosen_lsm_order) {
if (chosen_major_lsm) {
pr_warn("security=%s is ignored because it is superseded by lsm=%s\n",
@@ -387,8 +407,6 @@ static void __init ordered_lsm_init(void)
lsm_early_task(current);
for (lsm = ordered_lsms; *lsm; lsm++)
initialize_lsm(*lsm);
-
- kfree(ordered_lsms);
}
int __init early_security_init(void)
@@ -513,6 +531,15 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
{
int i;
+ if (lsm_active_cnt >= LSM_COUNT)
+ panic("%s Too many LSMs registered.\n", __func__);
+ /*
+ * A security module may call security_add_hooks() more
+ * than once. Landlock is one such case.
+ */
+ if (lsm_active_cnt == 0 || lsm_idlist[lsm_active_cnt - 1] != lsmid)
+ lsm_idlist[lsm_active_cnt++] = lsmid;
+
for (i = 0; i < count; i++) {
hooks[i].lsmid = lsmid;
hlist_add_tail_rcu(&hooks[i].list, hooks[i].head);