[PATCH] Fix common_audit_data type for smack_inode_unlink() and smack_inode_rmdir()

* [PATCH] Fix common_audit_data type for smack_inode_unlink() and smack_inode_rmdir()
@ 2013-03-11 11:50 Igor Zhbanov
  2013-03-11 17:20 ` Casey Schaufler
  2013-03-20 15:00 ` Casey Schaufler
  0 siblings, 2 replies; 4+ messages in thread
From: Igor Zhbanov @ 2013-03-11 11:50 UTC (permalink / raw)
  To: linux-kernel; +Cc: Kyungmin Park, Igor Zhbanov

This patch fixes kernel Oops because of wrong common_audit_data type
in smack_inode_unlink() and smack_inode_rmdir().

When SMACK security module is enabled and SMACK logging is on (/smack/logging
is not zero) and you try to delete the file which
1) you cannot delete due to SMACK rules and logging of failures is on
or
2) you can delete and logging of success is on,

you will see following:

	Unable to handle kernel NULL pointer dereference at virtual address 000002d7

	[<...>] (strlen+0x0/0x28)
	[<...>] (audit_log_untrustedstring+0x14/0x28)
	[<...>] (common_lsm_audit+0x108/0x6ac)
	[<...>] (smack_log+0xc4/0xe4)
	[<...>] (smk_curacc+0x80/0x10c)
	[<...>] (smack_inode_unlink+0x74/0x80)
	[<...>] (security_inode_unlink+0x2c/0x30)
	[<...>] (vfs_unlink+0x7c/0x100)
	[<...>] (do_unlinkat+0x144/0x16c)

The function smack_inode_unlink() (and smack_inode_rmdir()) need
to log two structures of different types. First of all it does:

	smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_DENTRY);
	smk_ad_setfield_u_fs_path_dentry(&ad, dentry);

This will set common audit data type to LSM_AUDIT_DATA_DENTRY
and store dentry for auditing (by function smk_curacc(), which in turn calls
dump_common_audit_data(), which is actually uses provided data and logs it).

	/*
	 * You need write access to the thing you're unlinking
	 */
	rc = smk_curacc(smk_of_inode(ip), MAY_WRITE, &ad);
	if (rc == 0) {
		/*
		 * You also need write access to the containing directory
		 */

Then this function wants to log anoter data:

		smk_ad_setfield_u_fs_path_dentry(&ad, NULL);
		smk_ad_setfield_u_fs_inode(&ad, dir);

The function sets inode field, but don't change common_audit_data type.

		rc = smk_curacc(smk_of_inode(dir), MAY_WRITE, &ad);
	}

So the dump_common_audit() function incorrectly interprets inode structure
as dentry, and Oops will happen.

This patch reinitializes common_audit_data structures with correct type.
Also I removed unneeded
	smk_ad_setfield_u_fs_path_dentry(&ad, NULL);
initialization, because both dentry and inode pointers are stored
in the same union.

Signed-off-by: Igor Zhbanov <i.zhbanov@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 security/smack/smack_lsm.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index fa64740..d52c780 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -654,7 +654,7 @@ static int smack_inode_unlink(struct inode *dir, struct dentry *dentry)
 		/*
 		 * You also need write access to the containing directory
 		 */
-		smk_ad_setfield_u_fs_path_dentry(&ad, NULL);
+		smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_INODE);
 		smk_ad_setfield_u_fs_inode(&ad, dir);
 		rc = smk_curacc(smk_of_inode(dir), MAY_WRITE, &ad);
 	}
@@ -685,7 +685,7 @@ static int smack_inode_rmdir(struct inode *dir, struct dentry *dentry)
 		/*
 		 * You also need write access to the containing directory
 		 */
-		smk_ad_setfield_u_fs_path_dentry(&ad, NULL);
+		smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_INODE);
 		smk_ad_setfield_u_fs_inode(&ad, dir);
 		rc = smk_curacc(smk_of_inode(dir), MAY_WRITE, &ad);
 	}
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 4+ messages in thread