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

From: Casey Schaufler
Date: Wed Mar 20 2013 - 11:00:30 EST


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().
>
> 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@xxxxxxxxxxx>
> Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>

Applied to git://git.gitorious.org/smack-next/kernel.git#stage-for-3.10

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

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