Message-Id: <200902191024.50198.paul.moore@hp.com>
Date:	Thu, 19 Feb 2009 10:24:50 -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

On Wednesday 18 February 2009 04:16:08 pm etienne wrote:
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 0278bc0..427595e 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1498,10 +1498,7 @@ 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.
>   */
> @@ -1512,41 +1509,22 @@ static char *smack_host_label(struct sockaddr_in
> *sip) 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;

Unless I'm mistaken the 'liap' and 'miap' variables are only used once inside 
the for loop, why not remove them and simply reference 'snp' directly?

>  		/*
> -		 * 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.
> -		 */
> -		if (miap->s_addr == 0xffffffff)
> -			return snp->smk_label;
> -		/*
> -		 * If the list entry mask is less specific than the best
> -		 * already found this entry is uninteresting.
> +		 * 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 | 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;
> +		if (liap->s_addr  == (siap->s_addr & miap->s_addr)) {
> +			bestlabel = snp->smk_label;
> +			break;

This is being a bit nit-picky, but why not get rid of 'bestlabel' completely 
and instead of breaking here simply reutn 'snp->smk_label'?  If we ever reach 
the end of the function (no match) just return NULL.

> +		}
>  	}
>
>  	return bestlabel;

...

> @@ -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;
> +	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 ( ; temp_mask; temp_mask <<= 1, maskn++);

More nit-picky stuff :)  Why not set 'maskn' to zero inside the for loop 
construct instead of at the top of the function?  There is less chance for 
error this way if someone else touches the code.

>  	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,40 @@ 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;

An empty line here might be nice.

> +	if (smack_netlbladdrs == NULL) {
> +		smack_netlbladdrs = new;
> +		return;
> +	}

I would prefer another one here, if that is okay with you.

> +	/** the comparison '>' is a bit hacky, but works */

Why start the comment with '/**', using a single '*' works just fine.

> +	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;
> +		}
> +	}

As Casey mentioned earlier (and has been brough up in the past), you should 
heavily consider using the list.h constructs, it would make life much easier 
here and elsewhere.

Bonus points if you convert the other Smack lists to using the list.h bits.

> +}

...

> +/**
>   * smk_write_netlbladdr - write() for /smack/netlabel
>   * @filp: file pointer, not actually used
>   * @buf: where to get the data from
> @@ -724,8 +752,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);

Why not just enter the value directly here?  It would be much clearer I think.

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