| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <1152905158.23584.35.camel@localhost.localdomain>
Date: Fri, 14 Jul 2006 12:25:57 -0700
From: Kylene Jo Hall <kjhall@...ibm.com>
To: Dave Hansen <haveblue@...ibm.com>
Cc: linux-kernel <linux-kernel@...r.kernel.org>,
LSM ML <linux-security-module@...r.kernel.org>,
Dave Safford <safford@...ibm.com>,
Mimi Zohar <zohar@...ibm.com>, Serge Hallyn <sergeh@...ibm.com>
Subject: Re: [RFC][PATCH 3/6] SLIM main patch
Comments inline below.
On Fri, 2006-07-14 at 11:27 -0700, Dave Hansen wrote:
> 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.
>
I'll try to clean this up better in the next version.
> > +
> > +#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?
>
I wanted to make it a static inline but how would I pass the member
field name that list_for_each_entry needs. I presume this is why the
list_for_each_* functions are #defines themselves.
> > +static void demote_threads(void)
> > +{
> > + do_demote_thread_list(¤t->sibling, sibling);
> > + do_demote_thread_list(¤t->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?
I'll test it out.
>
> > +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;
I'll fix next revision.
>
> > + 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?
Sure that shouldn't be a problem.
>
> > +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;
ok
> > + isec->lock = SPIN_LOCK_UNLOCKED;
> > + return isec;
> > +}
>
> Is that safe, or is will the spin_lock_init() version make the lock
> debugging code happier?
Ok.
> > +/*
> > + * 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.
I don't know of any such flags.
Thanks,
Kylie
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists