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 <[email protected]>
---
include/linux/security.h | 2 ++
security/security.c | 44 +++++++++++++++++++++++++++++++++-------
2 files changed, 39 insertions(+), 7 deletions(-)
diff --git a/include/linux/security.h b/include/linux/security.h
index 5b67f208f7de..33ed1860b96f 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -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,
diff --git a/security/security.c b/security/security.c
index 07a8fe7f92bf..a590fa98ddd6 100644
--- a/security/security.c
+++ b/security/security.c
@@ -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_SECURITY_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",
@@ -388,7 +408,7 @@ static void __init ordered_lsm_init(void)
for (lsm = ordered_lsms; *lsm; lsm++)
initialize_lsm(*lsm);
- kfree(ordered_lsms);
+ init_debug("lsm count = %d\n", lsm_active_cnt);
}
int __init early_security_init(void)
@@ -513,6 +533,16 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
{
int i;
+ /*
+ * 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;
+
+ if (lsm_active_cnt > LSM_COUNT)
+ panic("%s Too many LSMs registered.\n", __func__);
+
for (i = 0; i < count; i++) {
hooks[i].lsmid = lsmid;
hlist_add_tail_rcu(&hooks[i].list, hooks[i].head);
--
2.39.0
Greeting,
FYI, we noticed WARNING:at_security/security.c:#append_ordered_lsm due to commit (built with gcc-11):
commit: 98221e36622f4dde4afa923518706bd2c8870f00 ("[PATCH v5 2/8] 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/20230110-030739
base: https://git.kernel.org/cgit/linux/kernel/git/tip/tip.git 0ecc518e5c1a83fbfc6262d20d0df289eafc2207
patch link: https://lore.kernel.org/all/[email protected]/
patch subject: [PATCH v5 2/8] LSM: Maintain a table of LSM attribute data
in testcase: boot
on test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
[ 0.472992][ T0] ------------[ cut here ]------------
[ 0.472996][ T0] builtin: out of LSM slots!?
[ 0.474006][ T0] WARNING: CPU: 0 PID: 0 at security/security.c:173 append_ordered_lsm (security/security.c:173 (discriminator 1))
[ 0.474995][ T0] Modules linked in:
[ 0.476300][ T0] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 6.2.0-rc1-00014-g98221e36622f #1
[ 0.477324][ T0] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.0-debian-1.16.0-5 04/01/2014
[ 0.478996][ T0] RIP: 0010:append_ordered_lsm (security/security.c:173 (discriminator 1))
[ 0.480366][ T0] Code: 89 f5 53 48 89 fb e8 de fe ff ff 84 c0 75 79 48 63 05 54 76 13 00 83 f8 03 75 13 48 89 ee 48 c7 c7 4d e8 67 82 e8 66 97 55 fe <0f> 0b eb 5a 48 83 7b 18 00 75 08 48 c7 43 18 28 df a3 83 8d 50 01
All code
========
0: 89 f5 mov %esi,%ebp
2: 53 push %rbx
3: 48 89 fb mov %rdi,%rbx
6: e8 de fe ff ff callq 0xfffffffffffffee9
b: 84 c0 test %al,%al
d: 75 79 jne 0x88
f: 48 63 05 54 76 13 00 movslq 0x137654(%rip),%rax # 0x13766a
16: 83 f8 03 cmp $0x3,%eax
19: 75 13 jne 0x2e
1b: 48 89 ee mov %rbp,%rsi
1e: 48 c7 c7 4d e8 67 82 mov $0xffffffff8267e84d,%rdi
25: e8 66 97 55 fe callq 0xfffffffffe559790
2a:* 0f 0b ud2 <-- trapping instruction
2c: eb 5a jmp 0x88
2e: 48 83 7b 18 00 cmpq $0x0,0x18(%rbx)
33: 75 08 jne 0x3d
35: 48 c7 43 18 28 df a3 movq $0xffffffff83a3df28,0x18(%rbx)
3c: 83
3d: 8d 50 01 lea 0x1(%rax),%edx
Code starting with the faulting instruction
===========================================
0: 0f 0b ud2
2: eb 5a jmp 0x5e
4: 48 83 7b 18 00 cmpq $0x0,0x18(%rbx)
9: 75 08 jne 0x13
b: 48 c7 43 18 28 df a3 movq $0xffffffff83a3df28,0x18(%rbx)
12: 83
13: 8d 50 01 lea 0x1(%rax),%edx
[ 0.481996][ T0] RSP: 0000:ffffffff82e03e90 EFLAGS: 00010286
[ 0.483388][ T0] RAX: 0000000000000000 RBX: ffffffff83ae7938 RCX: c0000000ffff7fff
[ 0.484995][ T0] RDX: 0000000000000000 RSI: 0000000000027ffb RDI: 0000000000000001
[ 0.486542][ T0] RBP: ffffffff8265f33d R08: 0000000000000000 R09: 00000000ffff7fff
[ 0.487490][ T0] R10: ffffffff82e03d40 R11: ffffffff831d66e8 R12: ffff888100188bd3
[ 0.488517][ T0] R13: ffff888100188ba0 R14: 0000000000000001 R15: 0000000000000000
[ 0.489495][ T0] FS: 0000000000000000(0000) GS:ffff88842fc00000(0000) knlGS:0000000000000000
[ 0.490529][ T0] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 0.491364][ T0] CR2: ffff88843ffff000 CR3: 0000000002e0a000 CR4: 00000000000406b0
[ 0.492997][ T0] Call Trace:
[ 0.494012][ T0] <TASK>
[ 0.494774][ T0] ordered_lsm_parse (security/security.c:307)
[ 0.495996][ T0] ordered_lsm_init (security/security.c:379)
[ 0.496996][ T0] security_init (security/security.c:461)
[ 0.498304][ T0] start_kernel (init/main.c:1129)
[ 0.499310][ T0] secondary_startup_64_no_verify (arch/x86/kernel/head_64.S:358)
[ 0.500222][ T0] </TASK>
[ 0.501166][ T0] ---[ end trace 0000000000000000 ]---
If you fix the issue, kindly add following tag
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-lkp/[email protected]
To reproduce:
# build kernel
cd linux
cp config-6.2.0-rc1-00014-g98221e36622f .config
make HOSTCC=gcc-11 CC=gcc-11 ARCH=x86_64 olddefconfig prepare modules_prepare bzImage modules
make HOSTCC=gcc-11 CC=gcc-11 ARCH=x86_64 INSTALL_MOD_PATH=<mod-install-dir> modules_install
cd <mod-install-dir>
find lib/ | cpio -o -H newc --quiet | gzip > modules.cgz
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp qemu -k <bzImage> -m modules.cgz job-script # job-script is attached in this email
# if come across any failure that blocks the test,
# please remove ~/.lkp and /lkp dir to run from a clean state.
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
On Mon, Jan 9, 2023 at 1:07 PM Casey Schaufler <[email protected]> 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 <[email protected]>
> ---
> include/linux/security.h | 2 ++
> security/security.c | 44 +++++++++++++++++++++++++++++++++-------
> 2 files changed, 39 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 5b67f208f7de..33ed1860b96f 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -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,
> diff --git a/security/security.c b/security/security.c
> index 07a8fe7f92bf..a590fa98ddd6 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -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_SECURITY_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];
I'm guessing this 'LSM_COUNT + 1' logic is basically just copied from
ordered_lsm_init() - which is okay - but can you remind me why it is
'LSM_COUNT + 1' and not just 'LSM_COUNT'? Based on the LSM_COUNT
macro above it seems like LSM_COUNT should be enough, no?
> 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",
> @@ -388,7 +408,7 @@ static void __init ordered_lsm_init(void)
> for (lsm = ordered_lsms; *lsm; lsm++)
> initialize_lsm(*lsm);
>
> - kfree(ordered_lsms);
> + init_debug("lsm count = %d\n", lsm_active_cnt);
> }
Given 86ef3c735ec8 ("LSM: Better reporting of actual LSMs at boot"),
is this needed?
--
paul-moore.com
On 1/11/2023 1:01 PM, Paul Moore wrote:
> On Mon, Jan 9, 2023 at 1:07 PM Casey Schaufler <[email protected]> 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 <[email protected]>
>> ---
>> include/linux/security.h | 2 ++
>> security/security.c | 44 +++++++++++++++++++++++++++++++++-------
>> 2 files changed, 39 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index 5b67f208f7de..33ed1860b96f 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -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,
>> diff --git a/security/security.c b/security/security.c
>> index 07a8fe7f92bf..a590fa98ddd6 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -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_SECURITY_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];
> I'm guessing this 'LSM_COUNT + 1' logic is basically just copied from
> ordered_lsm_init() - which is okay - but can you remind me why it is
> 'LSM_COUNT + 1' and not just 'LSM_COUNT'? Based on the LSM_COUNT
> macro above it seems like LSM_COUNT should be enough, no?
Yup. I didn't spend a lot of time investigating why the + 1.
I'll look more deeply and correct if appropriate.
>> 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",
>> @@ -388,7 +408,7 @@ static void __init ordered_lsm_init(void)
>> for (lsm = ordered_lsms; *lsm; lsm++)
>> initialize_lsm(*lsm);
>>
>> - kfree(ordered_lsms);
>> + init_debug("lsm count = %d\n", lsm_active_cnt);
>> }
> Given 86ef3c735ec8 ("LSM: Better reporting of actual LSMs at boot"),
> is this needed?
None of what comes out from lsm.debug is strictly necessary, and
human or script can parse "initializing lsm=", but sometimes the
number of LSMs is interesting.
>
> --
> paul-moore.com
On Wed, Jan 11, 2023 at 7:36 PM Casey Schaufler <[email protected]> wrote:
> On 1/11/2023 1:01 PM, Paul Moore wrote:
> > On Mon, Jan 9, 2023 at 1:07 PM Casey Schaufler <[email protected]> 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 <[email protected]>
> >> ---
> >> include/linux/security.h | 2 ++
> >> security/security.c | 44 +++++++++++++++++++++++++++++++++-------
> >> 2 files changed, 39 insertions(+), 7 deletions(-)
...
> >> diff --git a/security/security.c b/security/security.c
> >> index 07a8fe7f92bf..a590fa98ddd6 100644
> >> --- a/security/security.c
> >> +++ b/security/security.c
> >> @@ -388,7 +408,7 @@ static void __init ordered_lsm_init(void)
> >> for (lsm = ordered_lsms; *lsm; lsm++)
> >> initialize_lsm(*lsm);
> >>
> >> - kfree(ordered_lsms);
> >> + init_debug("lsm count = %d\n", lsm_active_cnt);
> >> }
> > Given 86ef3c735ec8 ("LSM: Better reporting of actual LSMs at boot"),
> > is this needed?
>
> None of what comes out from lsm.debug is strictly necessary, and
> human or script can parse "initializing lsm=", but sometimes the
> number of LSMs is interesting.
I guess what I was questioning is if printing the @lsm_active_cnt
variable provides any better information that what is already provided
by commit 86ef3c735ec8? We currently print the enabled/active LSMs
with lsm.debug, printing a count seems a bit redundant to me.
--
paul-moore.com