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

[Igor Zhbanov]

Hi Casey,

Casey Schaufler wrote:

> 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?

My patch is applied against linux-next:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/security/smack/smack_lsm.c

And the patch successfully applies to linux-3.8.2 too.
What tree you have?

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

--
Best regards,
Igor Zhbanov,
Sub-Project Leader,
phone: +7 (495) 797 25 00 ext 3981
e-mail: i.zhbanov@xxxxxxxxxxx

Mobile group, Moscow R&D center, Samsung Electronics
12 Dvintsev street, building 1
127018, Moscow, Russian Federation

--
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/