Smack: Rework file hooks
This is one of those cases where you look at code you did
years ago and wonder what you might have been thinking.
There are a number of LSM hooks that work off of file pointers,
and most of them really want the security data from the inode.
Some, however, really want the security context that the process
had when the file was opened. The difference went undetected in
Smack until it started getting used in a real system with real
testing. At that point it was clear that something was amiss.
This patch corrects the misuse of the f_security value in several
of the hooks. The behavior will not usually be any different, as
the process had to be able to open the file in the first place, and
the old check almost always succeeded, as will the new, but for
different reasons.
Thanks to the Samsung Tizen development team that identified this.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 654345d..1fa7231 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -160,7 +160,7 @@
{
struct task_smack *tsp = current_security();
struct smack_known *sskp = tsp->smk_task;
- struct inode *inode = file->f_inode;
+ struct inode *inode = file_inode(file);
char acc[SMK_NUM_ACCESS_TYPE + 1];
if (rc <= 0)
@@ -168,7 +168,7 @@
smk_bu_mode(mode, acc);
pr_info("Smack Bringup: (%s %s %s) file=(%s %ld %pD) %s\n",
- sskp->smk_known, (char *)file->f_security, acc,
+ sskp->smk_known, smk_of_inode(inode)->smk_known, acc,
inode->i_sb->s_id, inode->i_ino, file,
current->comm);
return 0;
@@ -1347,6 +1347,9 @@
* The security blob for a file is a pointer to the master
* label list, so no allocation is done.
*
+ * f_security is the owner security information. It
+ * isn't used on file access checks, it's for send_sigio.
+ *
* Returns 0
*/
static int smack_file_alloc_security(struct file *file)
@@ -1384,17 +1387,18 @@
{
int rc = 0;
struct smk_audit_info ad;
+ struct inode *inode = file_inode(file);
smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_PATH);
smk_ad_setfield_u_fs_path(&ad, file->f_path);
if (_IOC_DIR(cmd) & _IOC_WRITE) {
- rc = smk_curacc(file->f_security, MAY_WRITE, &ad);
+ rc = smk_curacc(smk_of_inode(inode), MAY_WRITE, &ad);
rc = smk_bu_file(file, MAY_WRITE, rc);
}
if (rc == 0 && (_IOC_DIR(cmd) & _IOC_READ)) {
- rc = smk_curacc(file->f_security, MAY_READ, &ad);
+ rc = smk_curacc(smk_of_inode(inode), MAY_READ, &ad);
rc = smk_bu_file(file, MAY_READ, rc);
}
@@ -1412,10 +1416,11 @@
{
struct smk_audit_info ad;
int rc;
+ struct inode *inode = file_inode(file);
smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_PATH);
smk_ad_setfield_u_fs_path(&ad, file->f_path);
- rc = smk_curacc(file->f_security, MAY_LOCK, &ad);
+ rc = smk_curacc(smk_of_inode(inode), MAY_LOCK, &ad);
rc = smk_bu_file(file, MAY_LOCK, rc);
return rc;
}
@@ -1437,7 +1442,7 @@
{
struct smk_audit_info ad;
int rc = 0;
-
+ struct inode *inode = file_inode(file);
switch (cmd) {
case F_GETLK:
@@ -1446,14 +1451,14 @@
case F_SETLKW:
smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_PATH);
smk_ad_setfield_u_fs_path(&ad, file->f_path);
- rc = smk_curacc(file->f_security, MAY_LOCK, &ad);
+ rc = smk_curacc(smk_of_inode(inode), MAY_LOCK, &ad);
rc = smk_bu_file(file, MAY_LOCK, rc);
break;
case F_SETOWN:
case F_SETSIG:
smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_PATH);
smk_ad_setfield_u_fs_path(&ad, file->f_path);
- rc = smk_curacc(file->f_security, MAY_WRITE, &ad);
+ rc = smk_curacc(smk_of_inode(inode), MAY_WRITE, &ad);
rc = smk_bu_file(file, MAY_WRITE, rc);
break;
default:
@@ -1571,14 +1576,10 @@
* smack_file_set_fowner - set the file security blob value
* @file: object in question
*
- * Returns 0
- * Further research may be required on this one.
*/
static void smack_file_set_fowner(struct file *file)
{
- struct smack_known *skp = smk_of_current();
-
- file->f_security = skp;
+ file->f_security = smk_of_current();
}
/**
@@ -1630,6 +1631,7 @@
int rc;
int may = 0;
struct smk_audit_info ad;
+ struct inode *inode = file_inode(file);
smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_PATH);
smk_ad_setfield_u_fs_path(&ad, file->f_path);
@@ -1641,7 +1643,7 @@
if (file->f_mode & FMODE_WRITE)
may |= MAY_WRITE;
- rc = smk_curacc(file->f_security, may, &ad);
+ rc = smk_curacc(smk_of_inode(inode), may, &ad);
rc = smk_bu_file(file, may, rc);
return rc;
}
@@ -1661,21 +1663,17 @@
static int smack_file_open(struct file *file, const struct cred *cred)
{
struct task_smack *tsp = cred->security;
- struct inode_smack *isp = file_inode(file)->i_security;
+ struct inode *inode = file_inode(file);
struct smk_audit_info ad;
int rc;
- if (smack_privileged(CAP_MAC_OVERRIDE)) {
- file->f_security = isp->smk_inode;
+ if (smack_privileged(CAP_MAC_OVERRIDE))
return 0;
- }
smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_PATH);
smk_ad_setfield_u_fs_path(&ad, file->f_path);
- rc = smk_access(tsp->smk_task, isp->smk_inode, MAY_READ, &ad);
+ rc = smk_access(tsp->smk_task, smk_of_inode(inode), MAY_READ, &ad);
rc = smk_bu_credfile(cred, file, MAY_READ, rc);
- if (rc == 0)
- file->f_security = isp->smk_inode;
return rc;
}
