Messages in this thread | | | Date | Mon, 11 Mar 2013 10:20:12 -0700 | From | Casey Schaufler <> | Subject | Re: [PATCH] Fix common_audit_data type for smack_inode_unlink() and smack_inode_rmdir() |
| |
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); > }
| |