Message-Id: <200902201111.10319.paul.moore@hp.com>
Date:	Fri, 20 Feb 2009 11:11:10 -0500
From:	Paul Moore <paul.moore@...com>
To:	etienne <etienne.basset@...ericable.fr>
Cc:	Casey Schaufler <casey@...aufler-ca.com>,
	"Linux-Kernel" <linux-kernel@...r.kernel.org>,
	linux-security-module@...r.kernel.org
Subject: Re: [PATCH] SMACK netlabel fixes v2

On Thursday 19 February 2009 06:22:11 pm etienne wrote:
> Hi Paul,
>
> thanks for your comments, please find below an updated version of the
> patch, I hope it is fine with you and Casey. (i didnt remove the 'mask_bits
> = (1<<31)' of your last comment :
> => i think it's much obvious that it represent binary 1000..00 than
> 0x80000000, but that may be just me :)

Fair enough, it was a pretty nit-picky comment :)

> I redid some basics tests, it seems i've not messed things too much :)
>
> For the moving to standard list, I guess I could try to do that; (even
> other smack list, yes want my bonus points! ;) ) After a quick list.h grep
> i think i'll have to implement a 'sorted insert' anyway. But please allow
> me to do that on top of my patch

That is fine, the patch below is still a good improvement, no need to hold it 
up for the list conversion.

Reviewed-by: Paul Moore <paul.moore@...com>

> --
>
> the following patch (against 2.6.29rc5) fixes a few issues in the
> smack/netlabel area : 1) smack_host_label disregard a "0.0.0.0/0 @" rule
> (or other label), preventing 'tagged' tasks to access Internet (many
> systems drop packets with IP options) 2) netmasks were not handled
> correctly, they were stored in a way _not equivalent_  to conversion to
> be32 (it was equivalent for /0, /8, /16, /24, /32 masks but not other
> masks) 3) smack_netlbladdr prefixes (IP/mask) were not consistent (mask&IP
> was not done), so there could have been different list entries for the same
> IP prefix; if those entries had different labels, well ... 4) they were not
> sorted
>
> 1) 2) 3) are bugs, 4) is a more cosmetic issue.
> The patch :
> -creates a new helper smk_netlbladdr_insert to insert a smk_netlbladdr,
> sorted by netmask length -use the new sorted nature of  smack_netlbladdrs
> list to simplify smack_host_label : the first match _will_ be the more
> specific
> -corrects endianness issues in smk_write_netlbladdr &  netlbladdr_seq_show
>
> The patch are "tested" so that they no crash the system; cat
> /smack/netlabel is now sorted and always consistent. Some basics ping tests
> to '@' and other label combination seems ok See   an extract of my tests
> bellow the patch
>
> regards,
> Etienne
>
> Signed-off-by: <etienne.basset@...ericable.fr>
> ---
>
>
> security/smack/smack_lsm.c |   43 +++++------------------------
>  security/smack/smackfs.c   |   64
> +++++++++++++++++++++++++++++++++---------- 2 files changed, 57
> insertions(+), 50 deletions(-)
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 0278bc0..e7ded13 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1498,58 +1498,31 @@ static int smack_socket_post_create(struct socket
> *sock, int family, * looks for host based access restrictions
>   *
>   * This version will only be appropriate for really small
> - * sets of single label hosts. Because of the masking
> - * it cannot shortcut out on the first match. There are
> - * numerious ways to address the problem, but none of them
> - * have been applied here.
> + * sets of single label hosts.
>   *
>   * Returns the label of the far end or NULL if it's not special.
>   */
>  static char *smack_host_label(struct sockaddr_in *sip)
>  {
>  	struct smk_netlbladdr *snp;
> -	char *bestlabel = NULL;
>  	struct in_addr *siap = &sip->sin_addr;
> -	struct in_addr *liap;
> -	struct in_addr *miap;
> -	struct in_addr bestmask;
>
>  	if (siap->s_addr == 0)
>  		return NULL;
>
> -	bestmask.s_addr = 0;
> -
>  	for (snp = smack_netlbladdrs; snp != NULL; snp = snp->smk_next) {
> -		liap = &snp->smk_host.sin_addr;
> -		miap = &snp->smk_mask;
> -		/*
> -		 * If the addresses match after applying the list entry mask
> -		 * the entry matches the address. If it doesn't move along to
> -		 * the next entry.
> -		 */
> -		if ((liap->s_addr & miap->s_addr) !=
> -		    (siap->s_addr & miap->s_addr))
> -			continue;
>  		/*
> -		 * If the list entry mask identifies a single address
> -		 * it can't get any more specific.
> +		 * we break after finding the first match because
> +		 * the list is sorted from longest to shortest mask
> +		 * so we have found the most specific match
>  		 */
> -		if (miap->s_addr == 0xffffffff)
> +		if ((&snp->smk_host.sin_addr)->s_addr  ==
> +			(siap->s_addr & (&snp->smk_mask)->s_addr)) {
>  			return snp->smk_label;
> -		/*
> -		 * If the list entry mask is less specific than the best
> -		 * already found this entry is uninteresting.
> -		 */
> -		if ((miap->s_addr | bestmask.s_addr) == bestmask.s_addr)
> -			continue;
> -		/*
> -		 * This is better than any entry found so far.
> -		 */
> -		bestmask.s_addr = miap->s_addr;
> -		bestlabel = snp->smk_label;
> +		}
>  	}
>
> -	return bestlabel;
> +	return NULL;
>  }
>
>  /**
> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> index 8e42800..51f0efc 100644
> --- a/security/smack/smackfs.c
> +++ b/security/smack/smackfs.c
> @@ -650,10 +650,6 @@ static void *netlbladdr_seq_next(struct seq_file *s,
> void *v, loff_t *pos)
>
>  	return skp;
>  }
> -/*
> -#define BEMASK	0x80000000
> -*/
> -#define BEMASK	0x00000001
>  #define BEBITS	(sizeof(__be32) * 8)
>
>  /*
> @@ -663,12 +659,10 @@ static int netlbladdr_seq_show(struct seq_file *s,
> void *v) {
>  	struct smk_netlbladdr *skp = (struct smk_netlbladdr *) v;
>  	unsigned char *hp = (char *) &skp->smk_host.sin_addr.s_addr;
> -	__be32 bebits;
> -	int maskn = 0;
> +	int maskn;
> +	u32 temp_mask = be32_to_cpu(skp->smk_mask.s_addr);
>
> -	for (bebits = BEMASK; bebits != 0; maskn++, bebits <<= 1)
> -		if ((skp->smk_mask.s_addr & bebits) == 0)
> -			break;
> +	for (maskn = 0; temp_mask; temp_mask <<= 1, maskn++);
>
>  	seq_printf(s, "%u.%u.%u.%u/%d %s\n",
>  		hp[0], hp[1], hp[2], hp[3], maskn, skp->smk_label);
> @@ -702,6 +696,42 @@ static int smk_open_netlbladdr(struct inode *inode,
> struct file *file) }
>
>  /**
> + * smk_netlbladdr_insert
> + * @new : netlabel to insert
> + *
> + * This helper insert netlabel in the smack_netlbladdrs list
> + * sorted by netmask length (longest to smallest)
> + */
> +static void smk_netlbladdr_insert(struct smk_netlbladdr *new)
> +{
> +	struct smk_netlbladdr *m;
> +
> +	if (smack_netlbladdrs == NULL) {
> +		smack_netlbladdrs = new;
> +		return;
> +	}
> +
> +	/* the comparison '>' is a bit hacky, but works */
> +	if (new->smk_mask.s_addr > smack_netlbladdrs->smk_mask.s_addr) {
> +		new->smk_next = smack_netlbladdrs;
> +		smack_netlbladdrs = new;
> +		return;
> +	}
> +	for (m = smack_netlbladdrs; m != NULL; m = m->smk_next) {
> +		if (m->smk_next == NULL) {
> +			m->smk_next = new;
> +			return;
> +		}
> +		if (new->smk_mask.s_addr > m->smk_next->smk_mask.s_addr) {
> +			new->smk_next = m->smk_next;
> +			m->smk_next = new;
> +			return;
> +		}
> +	}
> +}
> +
> +
> +/**
>   * smk_write_netlbladdr - write() for /smack/netlabel
>   * @filp: file pointer, not actually used
>   * @buf: where to get the data from
> @@ -724,8 +754,9 @@ static ssize_t smk_write_netlbladdr(struct file *file,
> const char __user *buf, struct netlbl_audit audit_info;
>  	struct in_addr mask;
>  	unsigned int m;
> -	__be32 bebits = BEMASK;
> +	u32 mask_bits = (1<<31);
>  	__be32 nsa;
> +	u32 temp_mask;
>
>  	/*
>  	 * Must have privilege.
> @@ -761,10 +792,13 @@ static ssize_t smk_write_netlbladdr(struct file
> *file, const char __user *buf, if (sp == NULL)
>  		return -EINVAL;
>
> -	for (mask.s_addr = 0; m > 0; m--) {
> -		mask.s_addr |= bebits;
> -		bebits <<= 1;
> +	for (temp_mask = 0; m > 0; m--) {
> +		temp_mask |= mask_bits;
> +		mask_bits >>= 1;
>  	}
> +	mask.s_addr = cpu_to_be32(temp_mask);
> +
> +	newname.sin_addr.s_addr &= mask.s_addr;
>  	/*
>  	 * Only allow one writer at a time. Writes should be
>  	 * quite rare and small in any case.
> @@ -772,6 +806,7 @@ static ssize_t smk_write_netlbladdr(struct file *file,
> const char __user *buf, mutex_lock(&smk_netlbladdr_lock);
>
>  	nsa = newname.sin_addr.s_addr;
> +	/* try to find if the prefix is already in the list */
>  	for (skp = smack_netlbladdrs; skp != NULL; skp = skp->smk_next)
>  		if (skp->smk_host.sin_addr.s_addr == nsa &&
>  		    skp->smk_mask.s_addr == mask.s_addr)
> @@ -787,9 +822,8 @@ static ssize_t smk_write_netlbladdr(struct file *file,
> const char __user *buf, rc = 0;
>  			skp->smk_host.sin_addr.s_addr = newname.sin_addr.s_addr;
>  			skp->smk_mask.s_addr = mask.s_addr;
> -			skp->smk_next = smack_netlbladdrs;
>  			skp->smk_label = sp;
> -			smack_netlbladdrs = skp;
> +			smk_netlbladdr_insert(skp);
>  		}
>  	} else {
>  		rc = netlbl_cfg_unlbl_static_del(&init_net, NULL,

-- 
paul moore
linux @ hp

--
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