Re: [RFC][PATCH 3/6] SLIM main patch

Dave Hansen
2006-07-14T18:27:51Z

On Fri, 2006-07-14 at 10:24 -0700, Kylene Jo Hall wrote:
> +static int is_guard_integrity(struct slm_file_xattr *level)
> +{
> + if ((level->guard.iac_r != SLM_IAC_NOTDEFINED)
> + && (level->guard.iac_wx != SLM_IAC_NOTDEFINED))
> + return 1;
> + return 0;
> +}
> +
> +static int is_guard_secrecy(struct slm_file_xattr *level)
> +{
> + if ((level->guard.sac_rx != SLM_SAC_NOTDEFINED)
> + && (level->guard.sac_w != SLM_SAC_NOTDEFINED))
> + return 1;
> + return 0;
> +}

This is a nice helper function. I think there are a couple of other
places where nice helpers like this could really clean things up.

> +static void revoke_file_wperm(struct slm_file_xattr *cur_level)
> +{
> + int i, j = 0;
> + struct files_struct *files = current->files;
> + unsigned long fd = 0;
> + struct fdtable *fdt;
> + struct file *file;
> +
> + if (!files || !cur_level)
> + return;
> +
> + spin_lock(&files->file_lock);
> + fdt = files_fdtable(files);
> +
> + for (;;) {
> + i =j * __NFDBITS;
> + if ( i>= fdt->max_fdset || i >= fdt->max_fds)
> + break;
> + fd = fdt->open_fds->fds_bits[j++];
> + while(fd) {
> + if (fd & 1) {
> + file = fdt->fd[i++];
> + if (file && file->f_dentry)
> + do_revoke_file_wperm(file, cur_level);
> + }
> + fd >>= 1;
> + }
> + }
> + spin_unlock(&files->file_lock);
> +}

This is an awfully ugly function ;)

Instead of actually walking the fd table and revoking permissions, would
doing a hook in generic_write_permission() help? It might be easier to
switch back and forth.

> +static inline void do_revoke_mmap_wperm(struct vm_area_struct *mpnt,
> + struct slm_isec_data *isec,
> + struct slm_file_xattr *cur_level)
> +{
> + unsigned long start = mpnt->vm_start;
> + unsigned long end = mpnt->vm_end;
> + size_t len = end - start;
> + struct dentry *dentry = mpnt->vm_file->f_dentry;
> +
> + if ((mpnt->vm_flags & VM_WRITE)
> + && (mpnt->vm_flags & VM_SHARED)
> + && (cur_level->iac_level < isec->level.iac_level)) {
> + if (strncmp(dentry->d_name.name, "SYSV", 4) == 0) {
> + down_write(&current->mm->mmap_sem);
> + do_munmap(current->mm, start, len);
> + up_write(&current->mm->mmap_sem);
> + } else
> + do_mprotect(start, len, PROT_READ);
> + }
> +}

What is special about "SYSV"?

Do you care about VM_MAYWRITE as well here?

> +static int using_shmem(void)
> +{
> + struct task_struct *group_tsk;
> +
> + if (!current->group_leader)
> + return 0;
> +
> + group_tsk = current->group_leader;
> + if ((current->pid != group_tsk->pid) && (current->mm == group_tsk->mm))
> + return 1;
> + return 0;
> +}

I'm not sure this function name matches what it does. Are you trying to
determine whether or not a task shares any address space with another?
When I think of "shmem", I think of shmfs.

> +static void do_demote_thread_entry(struct task_struct *thread_tsk)
> +
> +{
> + struct slm_tsec_data *cur_tsec = current->security,
> + *thread_tsec = thread_tsk->security;
> +
> + if (thread_tsk->pid == 1)
> + return;

Why is init special-cased? (these checks are near and dear to the
people doing containers :)

> + if (current->pid != thread_tsk->pid)
> + return;
> + if (current->mm == thread_tsk->mm)
> + return;
> + if (!thread_tsec)
> + return;
> +
> + spin_lock(&thread_tsec->lock);
> + thread_tsec->iac_r = cur_tsec->iac_r;
> + thread_tsec->iac_wx = cur_tsec->iac_wx;
> + spin_unlock(&thread_tsec->lock);
> +}
> +
> +#define do_demote_thread_list(head, member) { \
> + struct task_struct *thread_tsk; \
> + list_for_each_entry(thread_tsk, head, member) \
> + do_demote_thread_entry(thread_tsk); \
> +}

Can this be an inline function instead?

> +static void demote_threads(void)
> +{
> + do_demote_thread_list(&current->sibling, sibling);
> + do_demote_thread_list(&current->children, children);
> +}
> +
> +/*
> + * Revoke write permissions and demote threads using shared memory
> + */
> +static void revoke_permissions(struct slm_file_xattr *cur_level)
> +{
> + if ((!is_kernel_thread(current)) && (current->pid != 1)) {
> + if (using_shmem())
> + demote_threads();
> +
> + revoke_mmap_wperm(cur_level);
> + revoke_file_wperm(cur_level);
> + }
> +}

Is that using_shmem() check really necessary? IF you're not a threaded
process and you get asked to demote your threads, I would imagine that
the code would fall out of the loop immediately. What does this protect
against?

> +static enum slm_iac_level set_iac(char *token)
> +{
> + int iac;
> +
> + if (memcmp(token, EXEMPT_STR, strlen(EXEMPT_STR)) == 0)
> + return SLM_IAC_EXEMPT;
> + else {

Might as well add brackets here. Or, just kill the else{} block and
pull the code back to the lower indenting level. The else is really
unnecessary because of the return;

> + for (iac = 0; iac < sizeof(slm_iac_str) / sizeof(char *); iac++) {
> + if (memcmp(token, slm_iac_str[iac],
> + strlen(slm_iac_str[iac])) == 0)
> + return iac;

Why not use strcmp?

> +static enum slm_sac_level set_sac(char *token)
> +{
> + int sac;
> +
> + if (memcmp(token, EXEMPT_STR, strlen(EXEMPT_STR)) == 0)
> + return SLM_SAC_EXEMPT;
> + else {
> + for (sac = 0; sac < sizeof(slm_sac_str) / sizeof(char *); sac++) {
> + if (memcmp(token, slm_sac_str[sac],
> + strlen(slm_sac_str[sac])) == 0)
> + return sac;
> + }
> + }
> + return SLM_SAC_ERROR;
> +}

This function looks awfully similar :). Can you just pass that array in
as an argument, and get rid of one of the functions?

> +static inline int set_bounds(char *token)
> +{
> + if (memcmp(token, UNLIMITED_STR, strlen(UNLIMITED_STR)) == 0)
> + return 1;
> + return 0;
> +}

strcmp?

> +/*
> + * Get the 7 access class levels from the extended attribute
> + * Format: TIMESTAMP INTEGRITY SECRECY [INTEGRITY_GUARD INTEGRITY_GUARD] [SECRECY_GUARD SECRECY_GUARD] [GUARD_ TYPE]
> + */
> +static int slm_parse_xattr(char *xattr_value, int xattr_len,
> + struct slm_file_xattr *level)
> +{
> + char *token;
> + int token_len;
> + char *buf, *buf_end;
> + int fieldno = 0;
> + int rc = -1;
> +
> + buf = xattr_value + sizeof(time_t);
> + if (*buf == 0x20)
> + buf++; /* skip blank after timestamp */
> + buf_end = xattr_value + xattr_len;
> +
> + while ((token = get_token(buf, buf_end, ' ', &token_len)) != NULL) {
> + buf = token + token_len;
> + switch (++fieldno) {
> + case 1:
> + if ((level->iac_level =
> + set_iac(token)) != SLM_IAC_ERROR)
> + rc = 0;
> + break;

How about:

level->iac_level = set_iac(token);
if (level->iac_level != SLM_IAC_ERROR)
rc = 0;
break;

> + case 2:
> + level->sac_level = set_sac(token);
> + break;
> + case 3:
> + level->guard.iac_r = set_iac(token);
> + break;
> + case 4:
> + level->guard.iac_wx = set_iac(token);
> + break;
> + case 5:
> + level->guard.unlimited = set_bounds(token);
> + level->guard.sac_w = set_sac(token);
> + break;
> + case 6:
> + level->guard.sac_rx = set_sac(token);
> + break;
> + case 7:
> + level->guard.unlimited = set_bounds(token);
> + default:
> + break;
> + }
> + }
> + return rc;
> +}
> +
> +/*
> + * Possible return codes: INTEGRITY_PASS, INTEGRITY_FAIL, INTEGRITY_NOLABEL,
> + * or -EINVAL
> + */
> +static int slm_get_xattr(struct dentry *dentry,
> + struct slm_file_xattr *level, int *xattr_status)
> +{
> + int xattr_len;
> + char *xattr_value = NULL;
> + int rc, error = -EINVAL;
> +
> + rc = integrity_verify_metadata(dentry, slm_xattr_name,
> + &xattr_value, &xattr_len, xattr_status);
> + if (rc < 0) {
> + printk(KERN_INFO
> + "%s integrity_verify_metadata failed (%d)\n",
> + dentry->d_name.name, rc);
> + return rc;
> + }
> +
> + if (xattr_value) {
> + memset(level, 0, sizeof(struct slm_file_xattr));
> + error = slm_parse_xattr(xattr_value, xattr_len, level);
> + kfree(xattr_value);
> + }
> +
> + if (level->iac_level != SLM_IAC_UNTRUSTED) {
> + rc = integrity_verify_data(dentry);
> + if (rc < 0) {
> + printk(KERN_INFO "%s integrity_verify_data failed "
> + " (%d)\n", dentry->d_name.name, rc);
> + return rc;
> + }
> + }
> +
> + return error < 0 ? -EINVAL : rc;
> +}

How about expanding this to a normal if()?

> +static void get_sock_level(struct dentry *dentry, struct slm_file_xattr *level)
> +{
> + struct slm_tsec_data *cur_tsec;
> + int rc, xattr_status = 0;
> +
> + cur_tsec = current->security;
> +
> + rc = slm_get_xattr(dentry, level, &xattr_status);
> + if (rc == -EINVAL) {

How about just 'if (rc)' just in case somebody decides to return a
different error code in the future?

> + if (xattr_status == -EOPNOTSUPP) {
> + level->iac_level = SLM_IAC_EXEMPT;
> + level->sac_level = SLM_SAC_EXEMPT;
> + } else {
> + level->iac_level = cur_tsec->iac_r;
> + level->sac_level = cur_tsec->sac_rx;
> + }
> + }
> +}
> +
> +static void get_level(struct dentry *dentry, struct slm_file_xattr *level)
> +{
> + int rc, xattr_status = 0;
> +
> + rc = slm_get_xattr(dentry, level, &xattr_status);
> + if ((rc == INTEGRITY_FAIL) || (rc == INTEGRITY_NOLABEL)) {
> + level->iac_level = SLM_IAC_UNTRUSTED;
> + level->sac_level = SLM_SAC_PUBLIC;
> + } else if (xattr_status == -EOPNOTSUPP) {
> + level->iac_level = SLM_IAC_EXEMPT;
> + level->sac_level = SLM_SAC_EXEMPT;
> + } else if (rc == -EINVAL) { /* improperly formatted */
> + level->iac_level = SLM_IAC_UNTRUSTED;
> + level->sac_level = SLM_SAC_PUBLIC;
> + }
> +}
> +
> +static struct slm_isec_data *slm_alloc_security(void)
> +{
> + struct slm_isec_data *isec;
> +
> + isec = kzalloc(sizeof(struct slm_isec_data), GFP_KERNEL);
> + if (!isec)
> + return NULL;
> +
> + isec->lock = SPIN_LOCK_UNLOCKED;
> + return isec;
> +}

Is that safe, or is will the spin_lock_init() version make the lock
debugging code happier?

> +static struct slm_isec_data * slm_inode_alloc_and_lock(struct inode *inode)
> +{
> + struct slm_isec_data *isec = slm_alloc_security();
> + if (!isec)
> + return NULL;
> +
> + spin_lock(&slm_inode_sec_lock);
> + if (inode->i_security) {
> + kfree(isec);
> + isec = inode->i_security;
> + } else
> + inode->i_security = isec;
> + spin_unlock(&slm_inode_sec_lock);
> +
> + return isec;
> +}
> +
> +/*
> + * Exempt objects without extended attribute support
> + */
> +static int is_exempt(struct inode *inode)
> +{
> + if ((inode->i_sb->s_magic == PROC_SUPER_MAGIC)
> + || S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode))
> + return 1;
> + return 0;
> +}

This could probably be a much more generic function, no?

inode_supports_xaddr()? Seems like something that should check a
superblock flag or something.

Man, there's a lot of code in here. ;) I'll look over some more this
weekend.

-- Dave