[PATCH] evm: Correct inode_init_security hooks behaviors
Nicolas Bouchinet
nicolas.bouchinet at clip-os.org
Fri Oct 21 14:04:28 UTC 2022
Hi Mickaël, thanks for you review,
On Thu, Oct 20, 2022 at 05:14:16PM +0200, Mickaël Salaün wrote:
>
> On 20/10/2022 15:55, Nicolas Bouchinet wrote:
> > From: Nicolas Bouchinet <nicolas.bouchinet at ssi.gouv.fr>
> >
> > Fixes a NULL pointer dereference occuring in the
>
> As pointed out by checkpatch.pl, there is a typo.
Taken into account.
>
>
> > `evm_protected_xattr_common` function of the EVM LSM. The bug is
> > triggered if a `inode_init_security` hook returns 0 without initializing
> > the given `struct xattr` fields (which is the case of BPF) and if no
> > other LSM overrides thoses fields after. This also leads to memory
> > leaks.
>
> Could you give more details how to trigger this bug and to test this fix?
To trigger the bug, I configured a fedora VM using mkinitcpio because fedora
has every tool to configure EVM. I then injected the archlinux dracut module tree
(/usr/lib/dracut/modules.d) in it.
The only thing to do next is to configure IMA/EVM enough for it to be enabled at boot.
You may follow this https://en.opensuse.org/SDB:Ima_evm#EVM_Protection_Using_HMACs
tutorial up to the end of the "Creation of Kernel Master Key and EVM HMAC key" part.
Next, regenerate your initrd using dracut and its integrity module
`dracut --add integrity /PATH/TO/initrd --force --verbose` and reboot.
Here is the stacktrace you should get:
```
[ OK ] Finished dracut-pre-pivot.…dracut pre-pivot and cleanup hook.
[ 38.159571] BUG: kernel NULL pointer dereference, address: 0000000000000000
[ 38.160155] #PF: supervisor read access in kernel mode
[ 38.160567] #PF: error_code(0x0000) - not-present page
[ 38.160988] PGD 0 P4D 0
[ 38.161209] Oops: 0000 [#1] PREEMPT SMP NOPTI
[ 38.161548] CPU: 0 PID: 1 Comm: systemd Not tainted 5.19.9-200.fc36.x86_64 #1
[ 38.162112] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
[ 38.162713] RIP: 0010:strlen+0x0/0x20
[ 38.163002] Code: b6 07 38 d0 74 14 48 83 c7 01 84 c0 74 05 48 39 f7 75 ec 31 c0 c3 cc cc cc cc 48 89 f8 c3 cc cc cc cc 0f 1f 84 00 00 00 00 00 <80> 3f 00 74 14 48 89 f8 48 83 c0 01 80 38 00 75 f7 48 29 f8 c3 cc
[ 38.164380] RSP: 0018:ffffc9000001fcf8 EFLAGS: 00010246
[ 38.164770] RAX: 0000000000000000 RBX: ffffc9000001fd78 RCX: ffffc9000001fd78
[ 38.165280] RDX: ffffc9000001fd90 RSI: 0000000000000000 RDI: 0000000000000000
[ 38.165813] RBP: 0000000000000000 R08: ffffc9000001fd80 R09: ffffc9000001fd88
[ 38.166308] R10: 0000000000000000 R11: 0000000000000000 R12: ffff8880061c18c8
[ 38.166772] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[ 38.167238] FS: 00007f59e2addb40(0000) GS:ffff88803d400000(0000) knlGS:0000000000000000
[ 38.167871] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 38.168322] CR2: 0000000000000000 CR3: 000000000322e002 CR4: 0000000000370ef0
[ 38.168876] Call Trace:
[ 38.169088] <TASK>
[ 38.169246] evm_protected_xattr_common+0x1a/0xb0
[ 38.169610] evm_inode_init_security+0x32/0xb0
[ 38.169983] security_inode_init_security+0xd3/0x130
[ 38.170373] ? shmem_encode_fh+0x90/0x90
[ 38.170676] shmem_symlink+0x7c/0x290
[ 38.170948] vfs_symlink+0x5d/0xe0
[ 38.171229] do_symlinkat+0xf7/0x110
[ 38.171487] __x64_sys_symlink+0x37/0x40
[ 38.171779] do_syscall_64+0x58/0x80
[ 38.172071] ? syscall_exit_to_user_mode+0x17/0x40
[ 38.172462] ? do_syscall_64+0x67/0x80
[ 38.172738] ? syscall_exit_to_user_mode+0x17/0x40
[ 38.173116] ? do_syscall_64+0x67/0x80
[ 38.173405] ? do_syscall_64+0x67/0x80
[ 38.173672] entry_SYSCALL_64_after_hwframe+0x63/0xcd
[ 38.174021] RIP: 0033:0x7f59e35f405b
[ 38.174309] Code: f0 ff ff 73 01 c3 48 8b 0d c2 4d 0f 00 f7 d8 64 89 01 48 83 c8 ff c3 0f 1f 84 00 00 00 00 00 f3 0f 1e fa b8 58 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 95 4d 0f 00 f7 d8 64 89 01 48
[ 38.175713] RSP: 002b:00007ffc5960eb28 EFLAGS: 00000246 ORIG_RAX: 0000000000000058
[ 38.176310] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f59e35f405b
[ 38.176879] RDX: 000055cef7249570 RSI: 000055cbaba83260 RDI: 000055cbaba54ac0
[ 38.177393] RBP: 000055cbaba5e9d0 R08: 000055cbabac04b0 R09: 0000000000000000
[ 38.177968] R10: 00007ffc5960e849 R11: 0000000000000246 R12: 000055cbaba54ac0
[ 38.178558] R13: 000055cbaba5e9d0 R14: 000055cbab9fae00 R15: 0000000000000001
[ 38.179148] </TASK>
[ 38.179341] Modules linked in: dm_crypt crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel serio_raw virtio_scsi qemu_fw_cfg
[ 38.180224] CR2: 0000000000000000
[ 38.180501] ---[ end trace 0000000000000000 ]---
[ 38.180879] RIP: 0010:strlen+0x0/0x20
[ 38.181177] Code: b6 07 38 d0 74 14 48 83 c7 01 84 c0 74 05 48 39 f7 75 ec 31 c0 c3 cc cc cc cc 48 89 f8 c3 cc cc cc cc 0f 1f 84 00 00 00 00 00 <80> 3f 00 74 14 48 89 f8 48 83 c0 01 80 38 00 75 f7 48 29 f8 c3 cc
[ 38.182565] RSP: 0018:ffffc9000001fcf8 EFLAGS: 00010246
[ 38.182944] RAX: 0000000000000000 RBX: ffffc9000001fd78 RCX: ffffc9000001fd78
[ 38.183491] RDX: ffffc9000001fd90 RSI: 0000000000000000 RDI: 0000000000000000
[ 38.184035] RBP: 0000000000000000 R08: ffffc9000001fd80 R09: ffffc9000001fd88
[ 38.184556] R10: 0000000000000000 R11: 0000000000000000 R12: ffff8880061c18c8
[ 38.185141] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[ 38.185724] FS: 00007f59e2addb40(0000) GS:ffff88803d400000(0000) knlGS:0000000000000000
[ 38.186311] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 38.186775] CR2: 0000000000000000 CR3: 000000000322e002 CR4: 0000000000370ef0
[ 38.187804] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000009
[ 38.188443] Kernel Offset: disabled
[ 38.188705] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000009 ]---
```
>
>
> >
> > Adds a `call_int_hook_xattr` macro that fetches and feed the
> > `new_xattrs` array with every called hook xattr values.
> >
> > Adds a `evm_init_hmacs` function which init the EVM hmac using every
> > entry of the array contrary to `evm_init_hmac`.
> >
> > Fixes the `evm_inode_init_security` function to use `evm_init_hmacs`.
> >
> > The `MAX_LSM_EVM_XATTR` value has been raised to 5 which gives room for
> > SMACK, SELinux, Apparmor, BPF and IMA/EVM security attributes.
> >
> > Changes the default return value of the `inode_init_security` hook
> > definition to `-EOPNOTSUPP`.
> >
> > Changes the hook documentation to match the behavior of the LSMs using
> > it (only xattr->value is initialised with kmalloc and thus is the only
> > one that should be kfreed by the caller).
> >
> > Cc: stable at vger.kernel.org
> > Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet at ssi.gouv.fr>
> > ---
> > include/linux/lsm_hook_defs.h | 2 +-
> > include/linux/lsm_hooks.h | 4 ++--
> > security/integrity/evm/evm.h | 2 ++
> > security/integrity/evm/evm_crypto.c | 23 ++++++++++++++++++++++-
> > security/integrity/evm/evm_main.c | 11 ++++++-----
> > security/security.c | 29 ++++++++++++++++++++++++++---
> > 6 files changed, 59 insertions(+), 12 deletions(-)
> >
> > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> > index 806448173033..e5dd0c0f6345 100644
> > --- a/include/linux/lsm_hook_defs.h
> > +++ b/include/linux/lsm_hook_defs.h
> > @@ -111,7 +111,7 @@ LSM_HOOK(int, 0, path_notify, const struct path *path, u64 mask,
> > unsigned int obj_type)
> > LSM_HOOK(int, 0, inode_alloc_security, struct inode *inode)
> > LSM_HOOK(void, LSM_RET_VOID, inode_free_security, struct inode *inode)
> > -LSM_HOOK(int, 0, inode_init_security, struct inode *inode,
> > +LSM_HOOK(int, -EOPNOTSUPP, inode_init_security, struct inode *inode,
> > struct inode *dir, const struct qstr *qstr, const char **name,
> > void **value, size_t *len)
> > LSM_HOOK(int, 0, inode_init_security_anon, struct inode *inode,
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index 84a0d7e02176..95aff9383de1 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -229,8 +229,8 @@
> > * This hook is called by the fs code as part of the inode creation
> > * transaction and provides for atomic labeling of the inode, unlike
> > * the post_create/mkdir/... hooks called by the VFS. The hook function
> > - * is expected to allocate the name and value via kmalloc, with the caller
> > - * being responsible for calling kfree after using them.
> > + * is expected to allocate the value via kmalloc, with the caller
> > + * being responsible for calling kfree after using it.
> > * If the security module does not use security attributes or does
> > * not wish to put a security attribute on this particular inode,
> > * then it should return -EOPNOTSUPP to skip this processing.
> > diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h
> > index f8b8c5004fc7..a2f9886e924d 100644
> > --- a/security/integrity/evm/evm.h
> > +++ b/security/integrity/evm/evm.h
> > @@ -60,6 +60,8 @@ int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name,
> > struct evm_digest *data);
> > int evm_init_hmac(struct inode *inode, const struct xattr *xattr,
> > char *hmac_val);
> > +int evm_init_hmacs(struct inode *inode, const struct xattr *xattrs,
> > + char *hmac_val);
> > int evm_init_secfs(void);
> > #endif
> > diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> > index 708de9656bbd..e5a34306cab6 100644
> > --- a/security/integrity/evm/evm_crypto.c
> > +++ b/security/integrity/evm/evm_crypto.c
> > @@ -347,7 +347,6 @@ static int evm_is_immutable(struct dentry *dentry, struct inode *inode)
> > return rc;
> > }
> > -
>
> This kind of cosmetic change should not be part of this patch.
Removed, thanks.
>
>
> > /*
> > * Calculate the hmac and update security.evm xattr
> > *
> > @@ -385,6 +384,28 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
> > return rc;
> > }
> > +int evm_protected_xattr(const char *req_xattr_name);
> > +
> > +int evm_init_hmacs(struct inode *inode, const struct xattr *lsm_xattrs,
> > + char *hmac_val)
> > +{
> > + struct shash_desc *desc;
> > +
> > + desc = init_desc(EVM_XATTR_HMAC, HASH_ALGO_SHA1);
> > + if (IS_ERR(desc)) {
> > + pr_info("init_desc failed\n");
> > + return PTR_ERR(desc);
> > + }
> > +
> > + for (int i = 0; lsm_xattrs[i].value != NULL; i++) {
> > + if (evm_protected_xattr(lsm_xattrs[i].name))
> > + crypto_shash_update(desc, lsm_xattrs[i].value, lsm_xattrs[i].value_len);
> > + }
> > + hmac_add_misc(desc, inode, EVM_XATTR_HMAC, hmac_val);
> > + kfree(desc);
> > + return 0;
> > +}
> > +
> > int evm_init_hmac(struct inode *inode, const struct xattr *lsm_xattr,
> > char *hmac_val)
> > {
> > diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> > index 2e6fb6e2ffd2..bb071c55d656 100644
> > --- a/security/integrity/evm/evm_main.c
> > +++ b/security/integrity/evm/evm_main.c
> > @@ -284,6 +284,8 @@ static int evm_protected_xattr_common(const char *req_xattr_name,
> > int found = 0;
> > struct xattr_list *xattr;
> > + if (!req_xattr_name)
> > + return found;
> > namelen = strlen(req_xattr_name);
> > list_for_each_entry_lockless(xattr, &evm_config_xattrnames, list) {
> > if (!all_xattrs && !xattr->enabled)
> > @@ -305,7 +307,7 @@ static int evm_protected_xattr_common(const char *req_xattr_name,
> > return found;
> > }
> > -static int evm_protected_xattr(const char *req_xattr_name)
> > +int evm_protected_xattr(const char *req_xattr_name)
> > {
> > return evm_protected_xattr_common(req_xattr_name, false);
> > }
> > @@ -835,14 +837,13 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
> > * evm_inode_init_security - initializes security.evm HMAC value
> > */
> > int evm_inode_init_security(struct inode *inode,
> > - const struct xattr *lsm_xattr,
> > + const struct xattr *lsm_xattrs,
> > struct xattr *evm_xattr)
> > {
> > struct evm_xattr *xattr_data;
> > int rc;
> > - if (!(evm_initialized & EVM_INIT_HMAC) ||
> > - !evm_protected_xattr(lsm_xattr->name))
> > + if (!(evm_initialized & EVM_INIT_HMAC))
> > return 0;
> > xattr_data = kzalloc(sizeof(*xattr_data), GFP_NOFS);
> > @@ -850,7 +851,7 @@ int evm_inode_init_security(struct inode *inode,
> > return -ENOMEM;
> > xattr_data->data.type = EVM_XATTR_HMAC;
> > - rc = evm_init_hmac(inode, lsm_xattr, xattr_data->digest);
> > + rc = evm_init_hmacs(inode, lsm_xattrs, xattr_data->digest);
> > if (rc < 0)
> > goto out;
> > diff --git a/security/security.c b/security/security.c
> > index 14d30fec8a00..47012c118536 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -30,7 +30,7 @@
> > #include <linux/msg.h>
> > #include <net/flow.h>
> > -#define MAX_LSM_EVM_XATTR 2
> > +#define MAX_LSM_EVM_XATTR 5
>
> I looks like AppArmor is not using the inode_init_security hook. There
> should be a comment explaining why this number is correct, and a runtime
> check (given that a static_assert call is not possible in this case) to make
> sure this is correct at boot time, maybe in security_init().
Your right, sorry for this, I took this into account and removed AppArmor from the count.
>
>
> > /* How many LSMs were built into the kernel? */
> > #define LSM_COUNT (__end_lsm_info - __start_lsm_info)
> > @@ -746,6 +746,29 @@ static int lsm_superblock_alloc(struct super_block *sb)
> > RC; \
> > })
> > +#define call_int_hook_xattr(XATTRS, FUNC, IRC, ...) ({ \
> > + int RC = IRC; \
> > + int i = 0; \
> > + do { \
> > + struct security_hook_list *P; \
> > + \
> > + hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
> > + RC = P->hook.FUNC(__VA_ARGS__); \
> > + if (RC == -EOPNOTSUPP) \
> > + continue; \
> > + if (RC != 0 && RC != IRC) \
> > + break; \
> > + if (i >= MAX_LSM_EVM_XATTR) { \
>
> You can use `if (WARN_ON_ONCE(i >= MAX_LSM_EVM_XATTR))` instead.
Neat, done.
>
>
> > + RC = -ENOMEM; \
> > + break; \
> > + } \
> > + XATTRS++; \
> > + i++; \
> > + } \
> > + } while (0); \
> > + RC; \
> > +})
>
> The content of this macro can be included in its only caller:
> security_inode_init_security().
Casey and Paul also asked for this, taken into account.
>
>
> > +
> > /* Security operations */
> > int security_binder_set_context_mgr(const struct cred *mgr)
> > @@ -1103,7 +1126,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> > dir, qstr, NULL, NULL, NULL);
> > memset(new_xattrs, 0, sizeof(new_xattrs));
> > lsm_xattr = new_xattrs;
> > - ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir, qstr,
> > + ret = call_int_hook_xattr(lsm_xattr, inode_init_security, -EOPNOTSUPP, inode, dir, qstr,
> > &lsm_xattr->name,
> > &lsm_xattr->value,
> > &lsm_xattr->value_len);
> > @@ -1111,7 +1134,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> > goto out;
> > evm_xattr = lsm_xattr + 1;
> > - ret = evm_inode_init_security(inode, lsm_xattr, evm_xattr);
> > + ret = evm_inode_init_security(inode, new_xattrs, evm_xattr);
> > if (ret)
> > goto out;
> > ret = initxattrs(inode, new_xattrs, fs_data);
>
> This looks good overall.
Thanks for your time,
Best regards,
Nicolas Bouchinet
More information about the Linux-security-module-archive
mailing list