--- Seth Arnold <[EMAIL PROTECTED]> wrote:
> On Sun, Jul 22, 2007 at 09:44:49PM -0700, Casey Schaufler wrote:
> >
> > I appears that everyone else took the weekend to read
> > "Deathly Hallows"* as it's been pretty quiet here. Well,
> > my wife took first dibs on our copy so I did some polishing
> > on smack instead. Since no one complained about the size last
>
> Sorry to hear she got to it first :) I'll try to not spoil it..
I got there before anyone spoiled it.
> > +/*
> > + * mapping for symlinks
> > +#define SMK_TMPPATH_SIZE 32
> > +#define SMK_TMPPATH_ROOT "/moldy/"
>
> This define isn't used here, is it used in the userspace?
Now used (below)
> > + /*
> > + * There will be only one smackfs. Casey says so.
> > + */
> > + smk_sb = sb;
>
> :)
>
> > + /*
> > + * Create a directory for /smack/tmp
> > + */
> > + smk_add_symlink("tmp", "/moldy/");
>
> I'm confused :)
Using the constant now. I must have gotten sidetracked.
>
> > +/*
> > + * Inode smack data
> > + */
> > +struct inode_smack {
> > + smack_t smk_inode; /* label of the fso */
> > + spinlock_t smk_lock; /* initialization lock */
> > + int smk_flags; /* smack inode flags */
> > +};
>
> No chance to use the inode's locks?
I don't see one that would work, but then, I haven't
demonstrated a great amount of lock-savey here.
> > +/*
> > + * There are not enough CAP bits available to make this
> > + * real, so Casey borrowed the capability that looks to
> > + * him like it has the best balance of similarity amd
> > + * low use.
> > + */
> > +#define CAP_MAC_OVERRIDE CAP_LINUX_IMMUTABLE
>
> Perhaps someday we should explore 64 bit cap space..
Soon.
> > +static smack_t *free_smack_t(smack_t *sp)
> > +{
> > + if (sp != NULL)
> > + kfree(sp);
>
> kfree already does this checking :)
I fixed.
> > + return NULL;
> > +}
> > +
>
> > +static void smack_sb_free_security(struct super_block *sb)
> > +{
> > + if (sb->s_security == NULL)
> > + return;
> > +
> > + kfree(sb->s_security);
> > + sb->s_security = NULL;
> > +}
>
> kfree doesn't need this, but it does avoid setting a NULL back to NULL.
I fixed.
> > +static int smack_sb_kern_mount(struct super_block *sb, void *data)
> > +{
> > + int rc;
> > + struct dentry *root = sb->s_root;
> > + struct inode *inode = root->d_inode;
> > + struct superblock_smack *sp = sb->s_security;
> > + struct inode_smack *isp;
> > + smack_t smack = SMK_UNSET;
> > + char *op;
> > + char *commap;
> > +
> > + if (sp == NULL) {
> > + rc = smack_sb_alloc_security(sb);
> > + if (rc != 0)
> > + return rc;
> > + }
> > + if (sp->smk_initialized != 0)
> > + return 0;
> > + if (inode == NULL)
> > + return 0;
> > +
> > + sp->smk_initialized = 1;
> > +
> > + /*
> > + * Not everyone supports extended attributes.
> > + */
> > + if (inode->i_op->getxattr != NULL) {
> > + rc = smk_fetch(inode, root, &smack);
> > + switch (rc) {
> > + case 0:
> > + case -ENODATA:
> > + case -EOPNOTSUPP:
> > + break;
> > + case sizeof(smack_t):
>
> What does this case do?
They entire smk_fetch thing is obsolete code. It's gone now.
> > + default:
> > + printk(KERN_WARNING "%s:%d (dev %s, type "
> > + "%s) \"%s\" rc = %d\n", __FUNCTION__, __LINE__,
> > + sb->s_id, sb->s_type->name, (char *)&smack, rc);
> > + break;
> > + }
> > + }
>
> > + switch (sbp->s_magic) {
> > + case SMACK_MAGIC:
> > + /*
> > + * Casey says that it's a little embarassing
> > + * that the smack file system doesn't do
> > + * extended attributes.
> > + */
>
> :)
>
> > + case DEVPTS_SUPER_MAGIC:
> > + /*
> > + * Casey says this is here because
> > + * devpts is "clean" of security hooks.
> > + *
> > + * pty's need to be handled for real.
> > + * Using "*" is expedient for bring-up.
> > + *
> > + */
> > + final = *csp;
> > + break;
>
> Old comment?
Fixed.
> > +static void smack_sk_free_security(struct sock *sk)
> > +{
> > + if (sk->sk_security == NULL)
> > + return;
> > +
> > + kfree(sk->sk_security);
> > + sk->sk_security = NULL;
> > +}
>
> kfree's fine with NULL
Stop saying that!
> > +static void smack_to_secattr(smack_t smack, struct netlbl_lsm_secattr
> *nlsp)
> > +{
> > + struct smk_cipso_entry *scp;
> > +
> > + switch (smack_net_nltype) {
> > + case NETLBL_NLTYPE_CIPSOV4:
> > + nlsp->domain = NULL;
> > + nlsp->flags = NETLBL_SECATTR_DOMAIN;
> > + nlsp->flags |= NETLBL_SECATTR_MLS_LVL;
> > +
> > + for (scp = smack_cipso; scp != NULL; scp = scp->smk_next)
> > + if (scp->smk_smack == smack)
> > + break;
> > +
> > + if (scp != NULL) {
> > + nlsp->mls_lvl = scp->smk_level;
> > + smack_set_catset(scp->smk_catset, nlsp);
> > + }
> > + else {
>
> } else { please
Though shalt adhere to The One True Brace Style, without
regard to how though feelth.
Fixed.
> > + nlsp->mls_lvl = smack_cipso_direct;
> > + smack_set_catset(smack, nlsp);
> > + }
> > + break;
> > + case NETLBL_NLTYPE_NONE:
> > + case NETLBL_NLTYPE_UNLABELED:
> > + break;
> > + default:
> > + break;
> > + }
> > +}
> > +
>
> > +static int smack_shm_associate(struct shmid_kernel *shp, int shmflg)
> > +{
> > + smack_t *ssp = smack_of_shm(shp);
> > + int rc;
> > +
> > + if (ssp == NULL)
> > + return 0;
> > +
> > + rc = smk_curacc(ssp, MAY_READWRITE);
> > + return rc;
> > +}
>
> No read-only or write-only shm mappings?
Still thinking about it.
> > +static int smack_shm_shmctl(struct shmid_kernel *shp, int cmd)
> > +{
> > + smack_t *ssp = smack_of_shm(shp);
> > + int rc;
> > +
> > + if (ssp == NULL)
> > + return 0;
> > +
> > + rc = smk_curacc(ssp, MAY_READWRITE);
> > + return rc;
> > +}
> > +
> > +static int smack_shm_shmat(struct shmid_kernel *shp, char __user *shmaddr,
> int shmflg)
> > +{
> > + smack_t *ssp = smack_of_shm(shp);
> > + int rc;
> > +
> > + if (ssp == NULL)
> > + return 0;
> > +
> > + rc = smk_curacc(ssp, MAY_READWRITE);
> > + return rc;
> > +}
>
> > +static int smack_sem_associate(struct sem_array *sma, int semflg)
> > +{
> > + smack_t *ssp = smack_of_sem(sma);
> > + int rc;
> > +
> > + if (ssp == NULL)
> > + return 0;
> > +
> > + rc = smk_curacc(ssp, MAY_READWRITE);
> > + return rc;
> > +}
> > +
> > +static int smack_sem_semctl(struct sem_array *sma, int cmd)
> > +{
> > + smack_t *ssp = smack_of_sem(sma);
> > + int rc;
> > +
> > + if (ssp == NULL)
> > + return 0;
> > +
> > + rc = smk_curacc(ssp, MAY_READWRITE);
> > + return rc;
> > +}
> > +
> > +static int smack_sem_semop(struct sem_array *sma, struct sembuf *sops,
> unsigned nsops, int alter)
> > +{
> > + smack_t *ssp = smack_of_sem(sma);
> > + int rc;
> > +
> > + if (ssp == NULL)
> > + return 0;
> > +
> > + rc = smk_curacc(ssp, MAY_READWRITE);
> > + return rc;
> > +}
>
> No read-only or write-only sem mappings?
Still thinking about it.
> > +static int smack_msg_queue_associate(struct msg_queue *msq, int msqflg)
> > +{
> > + smack_t *msp = smack_of_msq(msq);
> > + int rc;
> > +
> > + if (msp == NULL)
> > + return 0;
> > +
> > + rc = smk_curacc(msp, MAY_READWRITE);
> > + return rc;
> > +}
> > +
> > +static int smack_msg_queue_msgctl(struct msg_queue *msq, int cmd)
> > +{
> > + smack_t *msp = smack_of_msq(msq);
> > + int rc;
> > +
> > + if (msp == NULL)
> > + return 0;
> > +
> > + rc = smk_curacc(msp, MAY_READWRITE);
> > + return rc;
> > +}
> > +
> > +static int smack_msg_queue_msgsnd(struct msg_queue *msq, struct msg_msg
> *msg, int msqflg)
> > +{
> > + smack_t *msp = smack_of_msq(msq);
> > + int rc;
> > +
> > + if (msp == NULL)
> > + return 0;
> > +
> > + rc = smk_curacc(msp, MAY_READWRITE);
> > + return rc;
> > +}
> > +
> > +static int smack_msg_queue_msgrcv(struct msg_queue *msq, struct msg_msg
> *msg, struct task_struct *target, long type, int mode)
> > +{
> > + smack_t *msp = smack_of_msq(msq);
> > + int rc;
> > +
> > + if (msp == NULL)
> > + return 0;
> > +
> > + rc = smk_curacc(msp, MAY_READWRITE);
> > + return rc;
> > +}
>
> No read-only or write-only msg mappings?
I'll have to revisit these.
> > +static void smack_task_to_inode(struct task_struct *p, struct inode
> *inode)
> > +{
> > + smack_t *tsp = smk_of_task(p);
> > + smack_t *isp = smk_of_inode(inode);
> > +
> > + *isp = *tsp;
> > +}
>
> Interesting, I read this to myself as "[set] task to inode", rather than
> "[give] task's label to inode".
Look in fs/proc/base.c if you don't believe me.
> > +static int smack_task_wait(struct task_struct *p)
> > +{
> > + smack_t *tsp = smk_of_task(p);
> > + smack_t *csp = smk_of_task(current);
> > + int rc;
> > +
> > + rc = smk_access(csp, tsp, MAY_WRITE);
>
> No smk_curacc()?
smk_curacc() does the capability check. I want to call it
out explicitly below as it's special.
> > + if (rc == 0)
> > + return 0;
> > +
> > + /*
> > + * Allow the operation to succeed if either task
> > + * has privilege to perform operations that might
> > + * account for the smack labels having gotten to
> > + * be different in the first place.
> > + *
> > + * This breaks the strict subjet/object access
> > + * control ideal, taking the object's privilege
> > + * state into account in the decision as well as
> > + * the smack value.
> > + */
> > + if (__capable(current, CAP_MAC_OVERRIDE) ||
> > + __capable(p, CAP_MAC_OVERRIDE))
> > + return 0;
>
> This worries me a little. Just because the object _could_ have changed
> labels doesn't mean it _did_ change labels..
Since the labels are different we know that somebody changed.
>From the standpoint of analysis (ok, convinience) it doesn't
much matter which side it was.
> > + return rc;
> > +}
>
>
> Thanks Casey
Thank you. The comments have been a huge help.
Casey Schaufler
[EMAIL PROTECTED]
-
To unsubscribe from this list: send the line "unsubscribe
linux-security-module" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
