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 that it doesn't
> + * bother with the LSM hook in these cases.
> + */
> + if (info != SEND_SIG_NOINFO &&
> + (is_si_special(info) || SI_FROMKERNEL(info)))
> + return 0;
> + /*
> + * Sending a signal requires that the sender
> + * can write the receiver.
> + */
> + if (secid == 0)
> + return smk_curacc(p->security, MAY_WRITE);
> + /*
> + * If the secid isn't 0 we're dealing with some USB IO
> + * specific behavior. This is not clean. For one thing
> + * we can't take privilege into account.
> + */
> + return smk_access(smack_from_secid(secid), p->security, MAY_WRITE);
> +}
remind me again why some things are smk_foo and others are smack_foo?
Internal versus external? Is the code consistent in this?
> +/**
> + * smack_sk_alloc_security - Allocate a socket blob
> + * @sk: the socket
> + * @family: unused
> + * @priority: memory allocation priority
"gfp_flags" or "gfp_mask" would reduce surprise.
> + *
> + * Assign Smack pointers to current
> + *
> + * Returns 0 on success, -ENOMEM is there's no memory
> + */
> +static int smack_sk_alloc_security(struct sock *sk, int family, gfp_t priority)
> +{
> + char *csp = current->security;
> + struct socket_smack *ssp;
> +
> + ssp = kzalloc(sizeof(struct socket_smack), priority);
> + if (ssp == NULL)
> + return -ENOMEM;
> +
> + ssp->smk_in = csp;
> + ssp->smk_out = csp;
> +
> + sk->sk_security = ssp;
> +
> + return 0;
> +}
> +
> +/**
> + * smack_sk_free_security - Free a socket blob
> + * @sk: the socket
> + *
> + * Clears the blob pointer
> + */
> +static void smack_sk_free_security(struct sock *sk)
> +{
> + kfree(sk->sk_security);
> + sk->sk_security = NULL;
> +}
Hopefully that assignment of NULL isn't needed.
> +
> +DEFINE_MUTEX(smack_list_lock);
> +DEFINE_MUTEX(smack_known_lock);
> +DEFINE_MUTEX(smack_cipso_lock);
> +
> +/**
> + * smack_init - initialize the smack system
> + *
> + * Returns 0
> + */
> +static __init int smack_init(void)
> +{
> + printk(KERN_INFO "Smack: Initializing.\n");
> +
> + /*
> + * Set the security state for the initial task.
> + */
> + current->security = &smack_known_floor.smk_known;
> +
> + /*
> + * Initialize locks
> + */
> + spin_lock_init(&smack_known_unset.smk_cipsolock);
> + spin_lock_init(&smack_known_huh.smk_cipsolock);
> + spin_lock_init(&smack_known_hat.smk_cipsolock);
> + spin_lock_init(&smack_known_star.smk_cipsolock);
> + spin_lock_init(&smack_known_floor.smk_cipsolock);
> + spin_lock_init(&smack_known_invalid.smk_cipsolock);
> +
> + mutex_init(&smack_known_lock);
> + mutex_init(&smack_list_lock);
> + mutex_init(&smack_cipso_lock);
The mutex_init()s aren't needed.
It might be feasible to replace the spin_lock_init()s with compile-time
initialisation too.
> + /*
> + * Register with LSM
> + */
> + if (register_security(&smack_ops))
> + panic("smack: Unable to register with kernel.\n");
> +
> + return 0;
> +}
> +
> +/*
> + * Smack requires early initialization in order to label
> + * all processes and objects when they are created.
> + */
> +security_initcall(smack_init);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
