[PATCH 0/1] fix IMA + Apparmor kernel panic

[PATCH 0/1] fix IMA + Apparmor kernel panic

[Dmitry Kasatkin]

Hi,

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

Dmitry Kasatkin (1):
  ima: introduce ima_kernel_read()

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

-- 
1.8.3.2

[PATCH 1/1] ima: introduce ima_kernel_read()

[Dmitry Kasatkin]

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 <ebiederm@xmission.com>
Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
---
 security/integrity/ima/ima_crypto.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 1612a02..951523e 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -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;
-- 
1.8.3.2

IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic)

[J. R. Okajima]

Dmitry Kasatkin:
> 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

Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic)

[Dmitry Kasatkin]

On 09/05/14 06:10, J. R. Okajima wrote:
> Dmitry Kasatkin:
>> 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.

Hi,

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

I think we had to declare the purpose of the patch in a bit different way.
IMA really does not need direct-io, and can temporarily drop the flag.
As side affect, it would fix the deadlock problem

But I have a different patch now. I will post it today.


> How can we stop reading the file from IMA?

It is actually very interesting question...

1) if you would like to use IMA without it reading a file, then I think
I must disappoint you.
It is not possible.. IMA needs reading a file.

2) if you do not use IMA, then there is no problem for you, because IMA
will not read file if it is not used...

Have a nice day.

- Dmitry

>
> J. R. Okajima
>

Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic)

[J. R. Okajima]

Dmitry Kasatkin:
> 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.


J. R. Okajima

Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic)

[Mimi Zohar]

On Fri, 2014-05-09 at 18:17 +0900, J. R. Okajima wrote: 
> Dmitry Kasatkin:
> > 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.

Mimi

Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic)

[J. R. Okajima]

Mimi Zohar:
> 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.


J. R. Okajima

Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic)

[J. R. Okajima]

"J. R. Okajima":
> 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.

One more thing.
Since struct file is a sharable object, it might be better to put
task-id into struct file. Hmm, then should we support for multiple tasks
by list or something? Oh the code grows...


J. R. Okajima

Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic)

[Al Viro]

On Fri, May 09, 2014 at 12:10:03PM +0900, J. R. Okajima wrote:
> 
> Dmitry Kasatkin:
> > 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?

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.

Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic)

[Mimi Zohar]

On Fri, 2014-05-09 at 18:56 +0100, Al Viro wrote: 
> On Fri, May 09, 2014 at 12:10:03PM +0900, J. R. Okajima wrote:
> > 
> > Dmitry Kasatkin:
> > > 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?
> 
> 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.

Al, perhaps we didn't do a good job describing the different use case
scenarios for the different aspects of the integrity subsystem.  Are you
interested in hearing about them?

> 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.

Agreed it shouldn't be taking the i_mutex.  However, there was a lock
ordering issue when writing extended attributes, which does take the
i_mutex.

Mimi

Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic)

[Mimi Zohar]

On Sat, 2014-05-10 at 01:01 +0900, J. R. Okajima wrote: 
> Mimi Zohar:
> > 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

Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic)

[J. R. Okajima]

Mimi Zohar:
> 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=... 

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


J. R. Okajima

Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic)

[Dmitry Kasatkin]

On 9 May 2014 23:07, J. R. Okajima <hooanon05g@gmail.com> wrote:
>
> Mimi Zohar:
>> 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=...
>
> I prefer such approach or anything addressing in IMA only, so it makes
> sense.
>
>
> J. R. Okajima


We are working on i_mutex free solution.
Let's hold on on this...

-- 
Thanks,
Dmitry

Re: [PATCH 1/1] ima: introduce ima_kernel_read()

[Mimi Zohar]

On Thu, 2014-05-08 at 16:16 +0300, Dmitry Kasatkin wrote: 
> 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 <ebiederm@xmission.com>
> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>

As there has been no comment on this patch, I'll assume everyone is ok
with introducing an IMA specific version of kernel_read().

Mimi

> ---
>  security/integrity/ima/ima_crypto.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index 1612a02..951523e 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -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;