Date:	Tue, 13 May 2014 14:01:46 -0400
From:	Mimi Zohar <zohar@...ux.vnet.ibm.com>
To:	Dmitry Kasatkin <d.kasatkin@...sung.com>
Cc:	viro@...iv.linux.org.uk, ebiederm@...ssion.com,
	linux-security-module@...r.kernel.org, eparis@...hat.com,
	dmitry.kasatkin@...il.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] ima: introduce ima_kernel_read()

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@...ssion.com>
> Signed-off-by: Dmitry Kasatkin <d.kasatkin@...sung.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;


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists