--- Jan Engelhardt <jengelh@computergmbh.de> wrote:

quoted text
> 
> 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 :)

It's almost worth considering the change for the joke. Almost.

quoted text
> >+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?

Smack uses netlabel to communicate label information even locally.
Without this only processes running with the ambient label would
be able to use sockets.

quoted text
> >+	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.

Yup. Fixed.
 
quoted text
> >+++ 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 :=

Added.

quoted text
> >+++ 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.

I've hit a few places.

quoted text
> >+	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).

Err, yeah. Okay.

quoted text
> >+	if (sub == SMK_HAT && ((request & MAY_ANYREAD) == request))
> >+		return 0;
> >+
> >+	if (obj == SMK_FLOOR && ((request & MAY_ANYREAD) == request))
> >+		return 0;
> 
> Redundant parentheses, be gone.

Done.

quoted text
> 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.

Ok.

quoted text
> >+	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.

Small string stuck in a large integer. The only operation is
comparison. Big or little endian the string will come out right,
and that's what's important.

quoted text
> >+		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?

I have to look into that for the configuration values. I don't
think I can do it for the symlinks.

quoted text
> >+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.

It's pretty inconsistant, but that's what I prefer, too. I'll do it.

quoted text
> >+static struct smk_cipso_entry smack_cipso_floor = {
> >+        .smk_next = NULL,
> >+        .smk_smack = SMK_FLOOR,
> >+        .smk_level = 0,
> >+        .smk_catset = 0LL,
> >+};
> 
> const me.

This is the correct tinitial value, but could be changed.

quoted text
> >+/*
> >+ * 'ssssssss oooooooo mmmm\n__PLACEHOLDER__12_' 
> >+ */
> 
> 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)

Yes. "s == o" vs "strcmp(s,o) == 0".

quoted text
> >+	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__.

Changed, although it's used extensively elsewhere.

quoted text
> Not sure what they've got for __LINE__.

No clue. Leaving it as is.

quoted text
> >+		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.

Makes it a little cleaner. Thank you.

quoted text
> >+static char *smk_digit(char *cp)
> >+{
> >+	for (; *cp != '__PLACEHOLDER__16_'; cp++)
> >+		if (*cp >= '0' && *cp <= '9')
> >+			return cp;
> >+
> >+	return NULL;
> >+}
> 
> strpbrk again.

Thank you again.

quoted text
> >+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.

No problem. Changed.

quoted text
> >+	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?]

Better.

quoted text
> >+	*(data + count) = '__PLACEHOLDER__19_';
> 
> data[count] = '__PLACEHOLDER__19_';

Yeah, it's clearer.


quoted text
> >+	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()..

Spiffy. Used.

quoted text
> >+	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.

Constant strings all fit.

quoted text
> >+	for (i = 0; i < NCHOICES; i++)
> >+		if ((strcmp(bound, netlbl_choices[i].o_name) == 0) ||
> >+		    (strcmp(bound, netlbl_choices[i].o_alias) == 0)) {
> 
> redundant ()

Removed.


quoted text
> >+#define SMK_TMPPATH_SIZE        32
> >+#define SMK_TMPPATH_ROOT        "/moldy/"
> 
> What is going to be mold?

/tmp -> /smack/tmp -> /moldy/<yoursmacklabelhere>

quoted text
> >+	if (slp == NULL) {
> >+		printk("%s:%d failed\n", __FUNCTION__, __LINE__);
> 
> KERN_<level> is missing. (Check other places too)

printks in excess. Cleanup and correction in order.

quoted text
> 
> >+#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.

Worth looking into.
 
quoted text
> >+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...

the others?

quoted text
> >+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.

Yup. Fixed.

quoted text
> >+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)

That was a little excessive, wasn't it?

quoted text
> >+
> >+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.

Fixxxed.

 
quoted text
> >+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.

Fixed.

quoted text
> >+static const char *smack_inode_xattr_getsuffix(void)
> >+{
> >+	return XATTR_SMACK_SUFFIX;
> >+}
> 
> inline it, or opencode.

Better yet, no one is using the hook. nuked.

quoted text
> >+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.

The variable is really pointless. Repaired.

quoted text
> >+static int smack_inode_listsecurity(struct inode *inode, char *buffer,
> >+				    size_t buffer_size)
> 
> Whitespace broken.

Fixed

quoted text
> 
> [attention span ran out]

It's a summer Saturday. Thank you for your helpful comments.
 
quoted text
> Nice.

Thank you.


Casey Schaufler
casey@schaufler-ca.com
-
unsubscribe notice
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/