fix IMA + Apparmor kernel panic

Commit 8aac62706 "move exit_task_namespaces() outside of exit_notify" introduced the kernel opps since the kernel v3.10, which happens when Apparmor and IMA-appraisal are enabled at the same time.

----------------------------------------------------------------------

[  106.750167] BUG: unable to handle kernel NULL pointer dereference at
0000000000000018
[  106.750221] IP: [<ffffffff811ec7da>] our_mnt+0x1a/0x30
[  106.750241] PGD 0
[  106.750254] Oops: 0000 [#1] SMP
[  106.750272] Modules linked in: cuse parport_pc ppdev bnep rfcomm

bluetooth rpcsec_gss_krb5 nfsd auth_rpcgss nfs_acl nfs lockd sunrpc fscache dm_crypt intel_rapl x86_pkg_temp_thermal intel_powerclamp kvm_intel snd_hda_codec_hdmi kvm crct10dif_pclmul crc32_pclmul ghash_clmulni_intel aesni_intel aes_x86_64 glue_helper lrw gf128mul ablk_helper cryptd snd_hda_codec_realtek dcdbas snd_hda_intel snd_hda_codec snd_hwdep snd_pcm snd_page_alloc snd_seq_midi snd_seq_midi_event snd_rawmidi psmouse snd_seq microcode serio_raw snd_timer snd_seq_device snd soundcore video lpc_ich coretemp mac_hid lp parport mei_me mei nbd hid_generic e1000e usbhid ahci ptp hid libahci pps_core

[  106.750658] CPU: 6 PID: 1394 Comm: mysqld Not tainted 3.13.0-rc7-kds+ #15
[  106.750673] Hardware name: Dell Inc. OptiPlex 9010/0M9KCM, BIOS A08
09/19/2012
[  106.750689] task: ffff8800de804920 ti: ffff880400fca000 task.ti:
ffff880400fca000
[  106.750704] RIP: 0010:[<ffffffff811ec7da>]  [<ffffffff811ec7da>]
our_mnt+0x1a/0x30
[  106.750725] RSP: 0018:ffff880400fcba60  EFLAGS: 00010286
[  106.750738] RAX: 0000000000000000 RBX: 0000000000000100 RCX:
ffff8800d51523e7
[  106.750764] RDX: ffffffffffffffea RSI: ffff880400fcba34 RDI:
ffff880402d20020
[  106.750791] RBP: ffff880400fcbae0 R08: 0000000000000000 R09:
0000000000000001
[  106.750817] R10: 0000000000000000 R11: 0000000000000001 R12:
ffff8800d5152300
[  106.750844] R13: ffff8803eb8df510 R14: ffff880400fcbb28 R15:
ffff8800d51523e7
[  106.750871] FS:  0000000000000000(0000) GS:ffff88040d200000(0000)
knlGS:0000000000000000
[  106.750910] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  106.750935] CR2: 0000000000000018 CR3: 0000000001c0e000 CR4:
00000000001407e0
[  106.750962] Stack:
[  106.750981]  ffffffff813434eb ffff880400fcbb20 ffff880400fcbb18
0000000000000000
[  106.751037]  ffff8800de804920 ffffffff8101b9b9 0001800000000000
0000000000000100
[  106.751093]  0000010000000000 0000000000000002 000000000000000e
ffff8803eb8df500
[  106.751149] Call Trace:
[  106.751172]  [<ffffffff813434eb>] ? aa_path_name+0x2ab/0x430
[  106.751199]  [<ffffffff8101b9b9>] ? sched_clock+0x9/0x10
[  106.751225]  [<ffffffff8134a68d>] aa_path_perm+0x7d/0x170
[  106.751250]  [<ffffffff8101b945>] ? native_sched_clock+0x15/0x80
[  106.751276]  [<ffffffff8134aa73>] aa_file_perm+0x33/0x40
[  106.751301]  [<ffffffff81348c5e>] common_file_perm+0x8e/0xb0
[  106.751327]  [<ffffffff81348d78>] apparmor_file_permission+0x18/0x20
[  106.751355]  [<ffffffff8130c853>] security_file_permission+0x23/0xa0
[  106.751382]  [<ffffffff811c77a2>] rw_verify_area+0x52/0xe0
[  106.751407]  [<ffffffff811c789d>] vfs_read+0x6d/0x170
[  106.751432]  [<ffffffff811cda31>] kernel_read+0x41/0x60
[  106.751457]  [<ffffffff8134fd45>] ima_calc_file_hash+0x225/0x280
[  106.751483]  [<ffffffff8134fb52>] ? ima_calc_file_hash+0x32/0x280
[  106.751509]  [<ffffffff8135022d>] ima_collect_measurement+0x9d/0x160
[  106.751536]  [<ffffffff810b552d>] ? trace_hardirqs_on+0xd/0x10
[  106.751562]  [<ffffffff8134f07c>] ? ima_file_free+0x6c/0xd0
[  106.751587]  [<ffffffff81352824>] ima_update_xattr+0x34/0x60
[  106.751612]  [<ffffffff8134f0d0>] ima_file_free+0xc0/0xd0
[  106.751637]  [<ffffffff811c9635>] __fput+0xd5/0x300
[  106.751662]  [<ffffffff811c98ae>] ____fput+0xe/0x10
[  106.751687]  [<ffffffff81086774>] task_work_run+0xc4/0xe0
[  106.751712]  [<ffffffff81066fad>] do_exit+0x2bd/0xa90
[  106.751738]  [<ffffffff8173c958>] ? retint_swapgs+0x13/0x1b
[  106.751763]  [<ffffffff8106780c>] do_group_exit+0x4c/0xc0
[  106.751788]  [<ffffffff81067894>] SyS_exit_group+0x14/0x20
[  106.751814]  [<ffffffff8174522d>] system_call_fastpath+0x1a/0x1f
[  106.751839] Code: c3 0f 1f 44 00 00 55 48 89 e5 e8 22 fe ff ff 5d c3
0f 1f 44 00 00 55 65 48 8b 04 25 c0 c9 00 00 48 8b 80 28 06 00 00 48 89
e5 5d <48> 8b 40 18 48 39 87 c0 00 00 00 0f 94 c0 c3 0f 1f 80 00 00 00
[  106.752185] RIP  [<ffffffff811ec7da>] our_mnt+0x1a/0x30
[  106.752214]  RSP <ffff880400fcba60>
[  106.752236] CR2: 0000000000000018
[  106.752258] ---[ end trace 3c520748b4732721 ]---

----------------------------------------------------------------------

The reason for the oops is that IMA-appraisal uses "kernel_read()" when file is closed. kernel_read() honors LSM security hook which calls Apparmor handler, which uses current->nsproxy->mnt_ns. The 'guilty' commit changed the order of cleanup code so that nsproxy->mnt_ns was not already available for Apparmor.

Discussion about the issue with Al Viro and Eric W. Biederman suggested that kernel_read() is too high-level for IMA. Another issue, except security checking, that was identified is mandatory locking. kernel_read honors it as well and it might prevent IMA from calculating necessary hash. It was suggested to use simplified version of the function without security and locking checks.

This patch introduces special version ima_kernel_read(), which skips security, mandatory locking checking and fsnotify. It prevents the kernel oops to happen.

Suggested-by Eric W. Biederman <[email protected]>

Signed-off-by Dmitry Kasatkin <[email protected]>

---
 security/integrity/ima/ima_crypto.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

security/integrity/ima/ima_crypto.c +31 -1

index 1612a02..951523e 100644
@@ -27,6 +27,36 @@
 
 static struct crypto_shash *ima_shash_tfm;
 
+/**
+ * ima_kernel_read - read file content
+ *
+ * This is a function for reading file content instead of kernel_read().
+ * It does not perform locking checks to ensure it cannot be blocked.
+ * It does not perform security checks because it is irrelevant for IMA.
+ *
+ */
+static int ima_kernel_read(struct file *file, loff_t offset,
+			   char *addr, unsigned long count)
+{
+	mm_segment_t old_fs;
+	char __user *buf = addr;
+	ssize_t ret;
+
+	if (!(file->f_mode & FMODE_READ))
+		return -EBADF;
+	if (!file->f_op->read && !file->f_op->aio_read)
+		return -EINVAL;
+
+	old_fs = get_fs();
+	set_fs(get_ds());
+	if (file->f_op->read)
+		ret = file->f_op->read(file, buf, count, &offset);
+	else
+		ret = do_sync_read(file, buf, count, &offset);
+	set_fs(old_fs);
+	return ret;
+}
+
 int ima_init_crypto(void)
 {
 	long rc;

@@ -104,7 +134,7 @@ static int ima_calc_file_hash_tfm(struct file *file,
 	while (offset < i_size) {
 		int rbuf_len;
 
-		rbuf_len = kernel_read(file, offset, rbuf, PAGE_SIZE);
+		rbuf_len = ima_kernel_read(file, offset, rbuf, PAGE_SIZE);
 		if (rbuf_len < 0) {
 			rc = rbuf_len;
 			break;

Following patch replaces IMA usage of kernel_read() with special version which skips security check that triggers kernel panic when Apparmor and IMA appraisal are enabled together.

Dmitry Kasatkin:

Quoted 3 lines

Following patch replaces IMA usage of kernel_read() with special version which skips security check that triggers kernel panic when Apparmor and IMA appraisal are enabled together.

I know this is related to exit(2), but this behaviour of IMA is related to open(2) too.
When O_DIRECT is specified, some filesystems (for example, ext2) call do_blockdev_direct_IO() which acquires i_mutex. But IMA:process_measurement() already acquires i_mutex before kernel_read(). It causes a deadlock even if you replace kernel_read() by a simpler one.

How can we stop reading the file from IMA?

J. R. Okajima

It is a different issue.

I made patch more than a year ago which fix the problem

https://lkml.org/lkml/2013/2/20/601

Dmitry Kasatkin:

Quoted 5 lines

It is a different issue.

I made patch more than a year ago which fix the problem

https://lkml.org/lkml/2013/2/20/601

Yes, I know this is a different issue, but I didn't know the patch. While I am not sure how ugly (or beautiful) is the approache the patch took, as long as IMA needs to read the file, we should merge the merge. Or we should wait for your another patch.

Another approach was posted here
http://marc.info/?l=linux-security-module&m=138919062430367&w=2 which also was not upstreamed.

do_blockdev_direct_IO(), I'd suggest
- make two new static inline functions like
r = ima_aware_file_inode_mutex_lock(file) and ..._unlock(r, file).
- these new functions are complied when CONFIG_IMA is enabled, otherwise they are plain mutex_lock/unlock().
- then do_blockdev_direct_IO() can call them blindly.
- of course, O_DIRECT_HAVELOCK should be complied only when CONFIG_IMA is enabled too.

Mimi Zohar:

Quoted 3 lines

Another approach was posted here
http://marc.info/?l=linux-security-module&m=138919062430367&w=2 which also was not upstreamed.

It might be better a little than previous one which handles the flag temporarily. But, in order to make the code cleaner particulary for do_blockdev_direct_IO(), I'd suggest
- make two new static inline functions like
r = ima_aware_file_inode_mutex_lock(file) and ..._unlock(r, file).
- these new functions are complied when CONFIG_IMA is enabled, otherwise they are plain mutex_lock/unlock().
- then do_blockdev_direct_IO() can call them blindly.
- of course, O_DIRECT_HAVELOCK should be complied only when CONFIG_IMA is enabled too.

I can guess that several people thinks that is still "ugly locking", but the deadlock is much ugly in real world. And we need some workaround for it.

I assume so, as there wasn't any comment. As a temporary fix, would it make sense not to measure/appraise/audit files opened with the direct-io flag based policy? Define a new IMA policy option 'directio'. A sample rule would look like:

dont_appraise bprm_check directio fsuuid=...

Mimi Zohar:

Quoted 5 lines

I assume so, as there wasn't any comment. As a temporary fix, would it make sense not to measure/appraise/audit files opened with the direct-io flag based policy? Define a new IMA policy option 'directio'. A sample rule would look like:

Quoted 1 line

dont_appraise bprm_check directio fsuuid=...

I prefer such approach or anything addressing in IMA only, so it makes sense.

J. R. Okajima

Dmitry Kasatkin:

Quoted 3 lines

Following patch replaces IMA usage of kernel_read() with special version which skips security check that triggers kernel panic when Apparmor and IMA appraisal are enabled together.

I know this is related to exit(2), but this behaviour of IMA is related to open(2) too.
When O_DIRECT is specified, some filesystems (for example, ext2) call do_blockdev_direct_IO() which acquires i_mutex. But IMA:process_measurement() already acquires i_mutex before kernel_read(). It causes a deadlock even if you replace kernel_read() by a simpler one. How can we stop reading the file from IMA?

Quoted 6 lines · J. R. Okajima

Dmitry Kasatkin:

Quoted 3 lines

Following patch replaces IMA usage of kernel_read() with special version which skips security check that triggers kernel panic when Apparmor and IMA appraisal are enabled together.

Quoted 7 lines

I know this is related to exit(2), but this behaviour of IMA is related to open(2) too.
When O_DIRECT is specified, some filesystems (for example, ext2) call do_blockdev_direct_IO() which acquires i_mutex. But IMA:process_measurement() already acquires i_mutex before kernel_read(). It causes a deadlock even if you replace kernel_read() by a simpler one. How can we stop reading the file from IMA?

Disabling it would be a good start... And no, I'm not kidding - the mess you are proposing is such that it's not at all obvious that this stuff is worth the trouble.

Why the devil is it playing with ->i_mutex? IMA, that is. Its use for data is absolutely fs-dependent. Again, filesystem is *NOT* required to take ->i_mutex anywhere on the write path. At all.