On Aug 11 2007 10:57, Casey Schaufler wrote:
>
> "*" - pronounced "star"
wall
> "_" - pronounced "floor"
floor
> "^" - pronounced "hat"
roof
> "?" - pronounced "huh"
it's dark in here :)
>+config SECURITY_SMACK
>+ bool "Simplified Mandatory Access Control Kernel Support"
>+ depends on NETLABEL && SECURITY_NETWORK
Is this a hard dependency, or can this work without network sec labels?
>+ default n
>+ help
>+ This selects the Simplified Mandatory Access Control Kernel.
>+ SMACK is useful for sensitivity, integrity, and a variety
>+ of other madatory security schemes.
>+ If you are unsure how to answer this question, answer N.
I smell broken tabs.
>+++ linux-2.6.22/security/smack/Makefile 2007-07-10 01:08:05.000000000
>-0700
>@@ -0,0 +1,8 @@
>+#
>+# Makefile for the SMACK LSM
>+#
>+
>+obj-$(CONFIG_SECURITY_SMACK) := smack.o
>+
>+smack-y := smack_lsm.o smack_access.o smackfs.o
smack-objs :=
>+++ linux-2.6.22/security/smack/smack_access.c 2007-07-24 15:36:18.000000000
>-0700
>@@ -0,0 +1,113 @@
>+extern struct smk_list_entry *smack_list;
>+
>+static int smk_get_access(smack_t sub, smack_t obj)
>+{
>+ struct smk_list_entry *sp = smack_list;
proposes const struct. should be used elsewhere too.
>+ if (sub == SMK_STAR)
I just remember MechWarrior2 (game from Activison), where SMK stood for their
movie format... it's also a nice abbreviation for Seamonkey (browser suite).
>+ if (sub == SMK_HAT && ((request & MAY_ANYREAD) == request))
>+ return 0;
>+
>+ if (obj == SMK_FLOOR && ((request & MAY_ANYREAD) == request))
>+ return 0;
Redundant parentheses, be gone.
Never was it easier to say what ^ is called in your language :)
if (sub == '^' && ...)
return 0;
if (obj == '_' && ...)
>+/*
>+ * The value that this adds is that everything after any
>+ * character that's not allowed in a smack will be null
>+ */
>+smack_t smk_from_string(char *str)
>+{
>+ smack_t smack = 0LL;
"smack_t smack = 0;" is enough here.
>+ char *cp;
>+ int i;
>+
>+ for (cp = (char *)&smack, i = 0; i < sizeof(smack_t); str++,cp++,i++) {
Whatever it tries to do, this is not endian-safe. Except if @str
actually points to another smack_t. Yuck.
>+ if (*str <= ' ' || *str > '~')
>+ return smack;
>+ *cp = *str;
>+ }
>+ /*
>+ * Too long.
>+ */
>+ return SMK_INVALID;
>+}
>diff -uprN -X linux-2.6.22-base/Documentation/dontdiff
>linux-2.6.22-base/security/smack/smackfs.c
>linux-2.6.22/security/smack/smackfs.c
>--- linux-2.6.22-base/security/smack/smackfs.c 1969-12-31 16:00:00.000000000
>-0800
>+++ linux-2.6.22/security/smack/smackfs.c 2007-07-24 21:51:30.000000000
>-0700
Can't securityfs and/or sysfs be used?
>+enum smk_inos {
>+ SMK_ROOT_INO = 2,
>+ SMK_LOAD = 3, /* load policy */
>+ SMK_LINKS = 4, /* symlinks */
>+ SMK_CIPSO = 5, /* load label -> CIPSO mapping */
>+ SMK_DOI = 6, /* CIPSO DOI */
>+ SMK_DIRECT = 7, /* CIPSO level indicating direct label */
>+ SMK_AMBIENT = 8, /* internet ambient label */
>+ SMK_NLTYPE = 9, /* label scheme to use by default */
>+ SMK_TMP = 100, /* MUST BE LAST! /smack/tmp */
>+};
Generally, =s are aligned too.
>+static struct smk_cipso_entry smack_cipso_floor = {
>+ .smk_next = NULL,
>+ .smk_smack = SMK_FLOOR,
>+ .smk_level = 0,
>+ .smk_catset = 0LL,
>+};
const me.
>+/*
>+ * 'ssssssss oooooooo mmmm\n\0'
>+ */
I wonder why it's limited to 8 characters? Ah right.. sizeof(smack_t).
uhm.. are you trying to tell me that smack_t [typedef'ed to u64]
are actually meant as a char[8]? (/me scrathces head)
>+ for (cp = result; slp != NULL; slp = slp->smk_next) {
>+ srp = &slp->smk_rule;
>+ sprintf(cp, "%-8s %-8s",
>+ (char *)&srp->smk_subject, (char *)&srp->smk_object);
I like (const char *).
>+ printk("%s:%d bad scan\n",
>+ __FUNCTION__, __LINE__);
__FUNCTION__ is a GNU extension, C99 uses __func__.
Not sure what they've got for __LINE__.
>+ if (strchr(modestr, 'r') || strchr(modestr, 'R'))
>+ rule.smk_access |= MAY_READ;
>+ if (strchr(modestr, 'w') || strchr(modestr, 'W'))
>+ rule.smk_access |= MAY_WRITE;
>+ if (strchr(modestr, 'x') || strchr(modestr, 'X'))
>+ rule.smk_access |= MAY_EXEC;
>+ if (strchr(modestr, 'a') || strchr(modestr, 'A'))
>+ rule.smk_access |= MAY_APPEND;
There's strpbrk() available for you.
>+static char *smk_digit(char *cp)
>+{
>+ for (; *cp != '\0'; cp++)
>+ if (*cp >= '0' && *cp <= '9')
>+ return cp;
>+
>+ return NULL;
>+}
strpbrk again.
>+static int smk_cipso_doied;
>+static int smk_cipso_written;
Votes for unsigned int if it {can never be, is not intended to be} negative.
>+ for (slp = smack_cipso; slp != NULL; slp = slp->smk_next) {
>+ sprintf(cp, "%-8s %3d", (char *)&slp->smk_smack,slp->smk_level);
>+ cp += strlen(cp);
sprintf returns the number of bytes it has written, so
cp += sprintf(cp, ...)
might be used. [Can sprintf even return -1 here?]
>+ *(data + count) = '\0';
data[count] = '\0';
>+ char temp[80];
>+ ssize_t rc;
>+
>+ if (*ppos != 0)
>+ return 0;
>+
>+ sprintf(temp, "%d", smk_cipso_doi_value);
>+ rc = simple_read_from_buffer(buf, count, ppos, temp, strlen(temp));
80 is plenty for a 11 char string.
Look, they've got funny ideas! :)
net/ipv4/netfilter/nf_nat_irc.c:char buffer[sizeof("4294967296 65635")];
>+static struct option_names netlbl_choices[] = {
>+ { NETLBL_NLTYPE_RIPSO,
>+ NETLBL_NLTYPE_RIPSO_NAME, "ripso" },
>+ { NETLBL_NLTYPE_CIPSOV4,
>+ NETLBL_NLTYPE_CIPSOV4_NAME, "cipsov4" },
>+ { NETLBL_NLTYPE_CIPSOV4,
>+ NETLBL_NLTYPE_CIPSOV4_NAME, "cipso" },
>+ { NETLBL_NLTYPE_CIPSOV6,
>+ NETLBL_NLTYPE_CIPSOV6_NAME, "cipsov6" },
>+ { NETLBL_NLTYPE_UNLABELED,
>+ NETLBL_NLTYPE_UNLABELED_NAME, "unlabeled" },
>+};
>+#define NCHOICES (sizeof(netlbl_choices) / sizeof(struct option_names))
ARRAY_SIZE()..
>+ for (i = 0; i < NCHOICES; i++)
>+ if (smack_net_nltype == netlbl_choices[i].o_number) {
>+ sprintf(bound, "%s", netlbl_choices[i].o_name);
looks dangerous; to overflow.
>+ for (i = 0; i < NCHOICES; i++)
>+ if ((strcmp(bound, netlbl_choices[i].o_name) == 0) ||
>+ (strcmp(bound, netlbl_choices[i].o_alias) == 0)) {
redundant ()
>+#define SMK_TMPPATH_SIZE 32
>+#define SMK_TMPPATH_ROOT "/moldy/"
What is going to be mold?
>+ if (slp == NULL) {
>+ printk("%s:%d failed\n", __FUNCTION__, __LINE__);
KERN_<level> is missing. (Check other places too)
>+#define SMK_MAXLEN (sizeof(smack_t) - 1) /* NULL terminated */
>+
>+/*
>+ * I hope these are the hokeyist lines of code in the module. Casey.
>+ */
>+#define DEVPTS_SUPER_MAGIC 0x1cd1
>+#define SOCKFS_MAGIC 0x534F434B
>+#define PIPEFS_MAGIC 0x50495045
>+#define TMPFS_MAGIC 0x01021994
Try moving them to linux/magic.h.
>+extern int smack_net_nltype;
>+extern int smack_cipso_direct;
>+extern struct smk_cipso_entry *smack_cipso;
for consistency reasons, add extern to the other vars too...
>+struct inode_smack *new_inode_smack(smack_t smack)
>+{
>+ struct inode_smack *isp;
>+
>+ isp = kzalloc(sizeof(struct inode_smack), GFP_KERNEL);
>+ if (isp == NULL)
>+ return NULL;
>+
>+ isp->smk_inode = smack;
>+ isp->smk_flags = 0;
>+ mutex_init(&isp->smk_lock);
>+
>+ return isp;
>+}
Tabs or so.
>+static int smack_task_movememory(struct task_struct *p)
>+{
>+ int rc;
>+
>+ rc = smk_curacc(smk_of_task(p), MAY_WRITE);
>+ return rc;
>+}
Uh...
{
return smk_curacc(smk_of_task(p), MAY_WRITE);
}
(also others)
>+
>+static int smack_task_kill(struct task_struct *p, struct siginfo *info,
>+ int sig, u32 secid)
>+{
>+ smack_t *tsp = smk_of_task(p);
>+ int rc;
>+
>+ /*
>+ * Sending a signal requires that the sender
>+ * can write the receiver.
>+ */
>+ rc = smk_curacc(tsp, MAY_WRITE);
>+
>+ return rc;
>+}
>+
>+static int smack_sb_statfs(struct dentry *dentry)
>+{
>+ struct superblock_smack *sbp;
>+
>+ if (dentry == NULL || dentry->d_sb == NULL ||
>+ dentry->d_sb->s_security == NULL)
>+ return 0;
>+
>+ sbp = dentry->d_sb->s_security;
>+
>+ return smk_curacc(&sbp->smk_floor, MAY_READ);
>+}
>+/*
>+ * Inode hooks
>+ */
>+static int smack_inode_alloc_security(struct inode *inode)
>+{
>+ smack_t *csp = smk_of_task(current);
>+
>+ inode->i_security = new_inode_smack(*csp);
>+ if (inode->i_security == NULL)
>+ return -ENOMEM;
>+ return 0;
>+}
Tabz.
>+static int smack_inode_init_security(struct inode *inode, struct inode *dir,
>+ char **name, void **value, size_t *len)
>+{
>+ smack_t *isp = smk_of_inode(inode);
>+
>+ if (name && (*name = kstrdup(XATTR_SMACK_SUFFIX, GFP_KERNEL)) == NULL)
>+ return -ENOMEM;
>+
>+ if (value && (*value = kstrdup((char *)isp, GFP_KERNEL)) == NULL)
>+ return -ENOMEM;
Try not to do big assignments in if.
>+static const char *smack_inode_xattr_getsuffix(void)
>+{
>+ return XATTR_SMACK_SUFFIX;
>+}
inline it, or opencode.
>+static int smack_inode_setsecurity(struct inode *inode, const char *name,
>+ const void *value, size_t size, int flags)
>+{
>+ smack_t smack;
>+ smack_t *isp = smk_of_inode(inode);
>+ struct socket_smack *ssp;
>+ struct socket *sock;
>+ struct super_block *sbp;
>+ struct inode *ip = (struct inode *)inode;
This cast is really pointless.
>+static int smack_inode_listsecurity(struct inode *inode, char *buffer,
>+ size_t buffer_size)
Whitespace broken.
[attention span ran out]
Nice.
Jan
--
-
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/
