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

[Casey Schaufler <casey@schaufler-ca.com>]

On 3/11/2013 4:50 AM, Igor Zhbanov wrote:
> This patch fixes kernel Oops because of wrong common_audit_data type
> in smack_inode_unlink() and smack_inode_rmdir().

This patch does not apply to any tree I have. The lines:

	smk_ad_setfield_u_fs_path_dentry(&ad, NULL);

that the patch is deleting are not present. Can you make
sure that the patch applies to Linus' tree and resubmit?

Thank you.

>
> 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);
>  	}