* [PATCH 0/1] fix IMA + Apparmor kernel panic @ 2014-05-08 13:16 Dmitry Kasatkin 2014-05-08 13:16 ` [PATCH 1/1] ima: introduce ima_kernel_read() Dmitry Kasatkin 2014-05-09 3:10 ` IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic) J. R. Okajima 0 siblings, 2 replies; 14+ messages in thread From: Dmitry Kasatkin @ 2014-05-08 13:16 UTC (permalink / raw) To: viro, ebiederm, linux-security-module, eparis, zohar Cc: dmitry.kasatkin, linux-kernel, 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/1] ima: introduce ima_kernel_read() 2014-05-08 13:16 [PATCH 0/1] fix IMA + Apparmor kernel panic Dmitry Kasatkin @ 2014-05-08 13:16 ` Dmitry Kasatkin 2014-05-13 18:01 ` Mimi Zohar 2014-05-09 3:10 ` IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic) J. R. Okajima 1 sibling, 1 reply; 14+ messages in thread From: Dmitry Kasatkin @ 2014-05-08 13:16 UTC (permalink / raw) To: viro, ebiederm, linux-security-module, eparis, zohar Cc: dmitry.kasatkin, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/1] ima: introduce ima_kernel_read() 2014-05-08 13:16 ` [PATCH 1/1] ima: introduce ima_kernel_read() Dmitry Kasatkin @ 2014-05-13 18:01 ` Mimi Zohar 0 siblings, 0 replies; 14+ messages in thread From: Mimi Zohar @ 2014-05-13 18:01 UTC (permalink / raw) To: Dmitry Kasatkin Cc: viro, ebiederm, linux-security-module, eparis, dmitry.kasatkin, linux-kernel 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; ^ permalink raw reply [flat|nested] 14+ messages in thread
* IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic) 2014-05-08 13:16 [PATCH 0/1] fix IMA + Apparmor kernel panic Dmitry Kasatkin 2014-05-08 13:16 ` [PATCH 1/1] ima: introduce ima_kernel_read() Dmitry Kasatkin @ 2014-05-09 3:10 ` J. R. Okajima 2014-05-09 8:14 ` Dmitry Kasatkin 2014-05-09 17:56 ` Al Viro 1 sibling, 2 replies; 14+ messages in thread From: J. R. Okajima @ 2014-05-09 3:10 UTC (permalink / raw) To: Dmitry Kasatkin Cc: viro, ebiederm, linux-security-module, eparis, zohar, dmitry.kasatkin, linux-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic) 2014-05-09 3:10 ` IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic) J. R. Okajima @ 2014-05-09 8:14 ` Dmitry Kasatkin 2014-05-09 9:17 ` J. R. Okajima 2014-05-09 17:56 ` Al Viro 1 sibling, 1 reply; 14+ messages in thread From: Dmitry Kasatkin @ 2014-05-09 8:14 UTC (permalink / raw) To: J. R. Okajima Cc: viro, ebiederm, linux-security-module, eparis, zohar, dmitry.kasatkin, linux-kernel 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic) 2014-05-09 8:14 ` Dmitry Kasatkin @ 2014-05-09 9:17 ` J. R. Okajima 2014-05-09 14:58 ` Mimi Zohar 0 siblings, 1 reply; 14+ messages in thread From: J. R. Okajima @ 2014-05-09 9:17 UTC (permalink / raw) To: Dmitry Kasatkin Cc: viro, ebiederm, linux-security-module, eparis, zohar, dmitry.kasatkin, linux-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic) 2014-05-09 9:17 ` J. R. Okajima @ 2014-05-09 14:58 ` Mimi Zohar 2014-05-09 16:01 ` J. R. Okajima 0 siblings, 1 reply; 14+ messages in thread From: Mimi Zohar @ 2014-05-09 14:58 UTC (permalink / raw) To: J. R. Okajima Cc: Dmitry Kasatkin, viro, ebiederm, linux-security-module, eparis, dmitry.kasatkin, linux-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic) 2014-05-09 14:58 ` Mimi Zohar @ 2014-05-09 16:01 ` J. R. Okajima 2014-05-09 16:15 ` J. R. Okajima 2014-05-09 19:44 ` Mimi Zohar 0 siblings, 2 replies; 14+ messages in thread From: J. R. Okajima @ 2014-05-09 16:01 UTC (permalink / raw) To: Mimi Zohar Cc: Dmitry Kasatkin, viro, ebiederm, linux-security-module, eparis, dmitry.kasatkin, linux-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic) 2014-05-09 16:01 ` J. R. Okajima @ 2014-05-09 16:15 ` J. R. Okajima 2014-05-09 19:44 ` Mimi Zohar 1 sibling, 0 replies; 14+ messages in thread From: J. R. Okajima @ 2014-05-09 16:15 UTC (permalink / raw) To: Mimi Zohar, Dmitry Kasatkin, viro, ebiederm, linux-security-module, eparis, dmitry.kasatkin, linux-kernel "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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic) 2014-05-09 16:01 ` J. R. Okajima 2014-05-09 16:15 ` J. R. Okajima @ 2014-05-09 19:44 ` Mimi Zohar 2014-05-09 20:07 ` J. R. Okajima 1 sibling, 1 reply; 14+ messages in thread From: Mimi Zohar @ 2014-05-09 19:44 UTC (permalink / raw) To: J. R. Okajima Cc: Dmitry Kasatkin, viro, ebiederm, linux-security-module, eparis, dmitry.kasatkin, linux-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic) 2014-05-09 19:44 ` Mimi Zohar @ 2014-05-09 20:07 ` J. R. Okajima 2014-05-10 17:30 ` Dmitry Kasatkin 0 siblings, 1 reply; 14+ messages in thread From: J. R. Okajima @ 2014-05-09 20:07 UTC (permalink / raw) To: Mimi Zohar Cc: Dmitry Kasatkin, viro, ebiederm, linux-security-module, eparis, dmitry.kasatkin, linux-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic) 2014-05-09 20:07 ` J. R. Okajima @ 2014-05-10 17:30 ` Dmitry Kasatkin 0 siblings, 0 replies; 14+ messages in thread From: Dmitry Kasatkin @ 2014-05-10 17:30 UTC (permalink / raw) To: J. R. Okajima Cc: Mimi Zohar, Dmitry Kasatkin, Al Viro, Eric W. Biederman, linux-security-module, eparis, linux-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic) 2014-05-09 3:10 ` IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic) J. R. Okajima 2014-05-09 8:14 ` Dmitry Kasatkin @ 2014-05-09 17:56 ` Al Viro 2014-05-09 18:28 ` Mimi Zohar 1 sibling, 1 reply; 14+ messages in thread From: Al Viro @ 2014-05-09 17:56 UTC (permalink / raw) To: J. R. Okajima Cc: Dmitry Kasatkin, ebiederm, linux-security-module, eparis, zohar, dmitry.kasatkin, linux-kernel 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic) 2014-05-09 17:56 ` Al Viro @ 2014-05-09 18:28 ` Mimi Zohar 0 siblings, 0 replies; 14+ messages in thread From: Mimi Zohar @ 2014-05-09 18:28 UTC (permalink / raw) To: Al Viro Cc: J. R. Okajima, Dmitry Kasatkin, ebiederm, linux-security-module, eparis, dmitry.kasatkin, linux-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2014-05-13 18:02 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-05-08 13:16 [PATCH 0/1] fix IMA + Apparmor kernel panic Dmitry Kasatkin 2014-05-08 13:16 ` [PATCH 1/1] ima: introduce ima_kernel_read() Dmitry Kasatkin 2014-05-13 18:01 ` Mimi Zohar 2014-05-09 3:10 ` IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic) J. R. Okajima 2014-05-09 8:14 ` Dmitry Kasatkin 2014-05-09 9:17 ` J. R. Okajima 2014-05-09 14:58 ` Mimi Zohar 2014-05-09 16:01 ` J. R. Okajima 2014-05-09 16:15 ` J. R. Okajima 2014-05-09 19:44 ` Mimi Zohar 2014-05-09 20:07 ` J. R. Okajima 2014-05-10 17:30 ` Dmitry Kasatkin 2014-05-09 17:56 ` Al Viro 2014-05-09 18:28 ` Mimi Zohar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).