On Mon, 19 Nov 2007 21:54:37 -0800
Casey Schaufler [EMAIL PROTECTED] wrote:
From: Casey Schaufler [EMAIL PROTECTED]
Smack is the Simplified Mandatory Access Control Kernel.
This patch seems bigger than the first version ;)
random-trivial-comments-just-to-show-i-read-it:
+static int smack_inode_setsecurity(struct inode *inode, const char *name,
+const void *value, size_t size, int flags)
+{
+ char *sp;
+ struct inode_smack *nsp = (struct inode_smack *)inode-i_security;
Please avoid casting when assigning to and from void* - it's unneeded and
defeats typechecking - if someone goes and turns inode.i_security into a
float or a struct capiloaddatapart* then we do want this code to warn about
it.
+ struct socket_smack *ssp;
+ struct socket *sock;
+
+ if (value == NULL || size SMK_LABELLEN)
+ return -EACCES;
+
+ sp = smk_import(value, size);
+ if (sp == NULL)
+ return -EINVAL;
+
+ if (strcmp(name, XATTR_SMACK_SUFFIX) == 0) {
+ mutex_lock(nsp-smk_lock);
+ nsp-smk_inode = sp;
+ mutex_unlock(nsp-smk_lock);
Does this locking actually do anything? The only place where it makes
sense is if there is some code elsewhere which reads nsp-smk_inode twice,
both instances under the same taking of -smk_lock, and in which it expects
both reads to return the same value.
IOW: it's fishy.
+ return 0;
+ }
+ /*
+ * The rest of the Smack xattrs are only on sockets.
+ */
+ if (inode-i_sb-s_magic != SOCKFS_MAGIC)
+ return -EOPNOTSUPP;
+
+ sock = SOCKET_I(inode);
+ if (sock == NULL)
+ return -EOPNOTSUPP;
+
+ ssp = sock-sk-sk_security;
+
+ if (strcmp(name, XATTR_SMACK_IPIN) == 0)
+ ssp-smk_in = sp;
+ else if (strcmp(name, XATTR_SMACK_IPOUT) == 0)
+ ssp-smk_out = sp;
+ else
+ return -EOPNOTSUPP;
+ return 0;
+}
+
...
+
+/**
+ * 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 int smack_file_set_fowner(struct file *file)
+{
+ file-f_security = current-security;
+ return 0;
+}
hm. I'd have expected to see some refcounting when a ref to an object is
copied like this.
+/**
+ * smack_file_send_sigiotask - Smack on sigio
+ * @tsk: The target task
+ * @fown: the object the signal come from
+ * @signum: unused
+ *
+ * Allow a privileged task to get signals even if it shouldn't
+ *
+ * Returns 0 if a subject with the object's smack could
+ * write to the task, an error code otherwise.
+ */
+static int smack_file_send_sigiotask(struct task_struct *tsk,
+ struct fown_struct *fown, int signum)
+{
+ struct file *file;
+ int rc;
+
+ /*
+ * struct fown_struct is never outside the context of a struct file
+ */
+ file = (struct file *)((long)fown - offsetof(struct file, f_owner));
Will container_of() work here?
+ rc = smk_access(file-f_security, tsk-security, MAY_WRITE);
+ if (rc != 0 __capable(tsk, CAP_MAC_OVERRIDE))
+ return 0;
+ return rc;
+}
+
+/**
+ * smack_file_receive - Smack file receive check
+ * @file: the object
+ *
+ * Returns 0 if current has access, error code otherwise
+ */
+static int smack_file_receive(struct file *file)
+{
+ int may = 0;
+
+ /*
+ * This code relies on bitmasks.
+ */
+ if (file-f_mode FMODE_READ)
+ may = MAY_READ;
+ if (file-f_mode FMODE_WRITE)
+ may |= MAY_WRITE;
+
+ return smk_curacc(file-f_security, may);
+}
+
+/*
+ * Task hooks
+ */
+
+/**
+ * smack_task_alloc_security - allocate a task blob
+ * @tsk: the task in need of a blob
+ *
+ * Smack isn't using copies of blobs. Everyone
+ * points to an immutible list. No alloc required.
+ * No data copy required.
I guess that answers my refcounting question above.
Spello: immutable.
+ * Always returns 0
+ */
+static int smack_task_alloc_security(struct task_struct *tsk)
+{
+ tsk-security = current-security;
+
+ return 0;
+}
+
+/**
+ * smack_task_free_security - free a task blob
+ * @task: the task with the blob
+ *
+ * Smack isn't using copies of blobs. Everyone
+ * points to an immutible list. The blobs never go away.
+ * There is no leak here.
Thoroughly answered.
Ditto on the spello tho.
+ */
+static void smack_task_free_security(struct task_struct *task)
+{
+ task-security = NULL;
+}
+
...
+static int smack_task_kill(struct task_struct *p, struct siginfo *info,
+int sig, u32 secid)
+{
+ /*
+ * Special cases where signals really ought to go through
+ * in spite of policy. Stephen Smalley suggests it may
+ * make sense to change the caller so
