netdev - Re: [RFC PATCH v2 06/18] netlabel: Add support for creating a CALIPSO protocol domain mapping.

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Sun, 07 Feb 2016 14:56:01 -0500
From:	Paul Moore <pmoore@...hat.com>
To:	Huw Davies <huw@...eweavers.com>
Cc:	netdev@...r.kernel.org, linux-security-module@...r.kernel.org,
	selinux@...ho.nsa.gov
Subject: Re: [RFC PATCH v2 06/18] netlabel: Add support for creating a CALIPSO protocol domain mapping.

On Friday, January 08, 2016 09:52:42 AM Huw Davies wrote:
> This extends the NLBL_MGMT_C_ADD and NLBL_MGMT_C_ADDDEF commands
> to accept CALIPSO protocol DOIs.
> 
> Signed-off-by: Huw Davies <huw@...eweavers.com>

...

> @@ -300,6 +311,11 @@ static int netlbl_domhsh_validate(const struct
> netlbl_dom_map *entry) entry->def.cipso == NULL)
>  			return -EINVAL;
>  		break;

In netlbl_domhsh_validate() you should also check to ensure that entry-
>def.calipso is NULL in the NETLBL_NLTYPE_UNLABELED case similar to what we do 
for CIPSO.

> @@ -233,6 +252,9 @@ static int netlbl_mgmt_add_common(struct genl_info
> *info, map->list.valid = 1;
>  		map->def.type = entry->def.type;
> 
> +		if (calipso)
> +			map->def.calipso = calipso;
> +

This is another nit-picky comment, and I'll only mention it this one time, but 
there are a number of places in this patchset where I think you are adding 
undesirable vertical whitespace.  In the if-block above, the whitespace above 
the "if (calipso)" line is inconsistent with other similar parts of the 
function (e.g. the CIPSO based code) and doesn't make the code any easier to 
read.

I mention this not because such things are grounds for NACK'ing a patch, but 
just something to make note of and fix if you happen to be updating the 
patch(set) anyway.

>  		ret_val = netlbl_af6list_add(&map->list, &addrmap->list6);
>  		if (ret_val != 0) {
>  			kfree(map);

-- 
paul moore
security @ redhat