Re: [PATCH v2 3/4] device_cgroup: make may_access() stronger

[Tejun Heo <tj@kernel.org>]

Hello,

On Thu, Jan 24, 2013 at 02:50:00PM -0500, aris@redhat.com wrote:
> In order to revalidate local exceptions for the hierarchy change propagation,
> make may_access() stronger.

It would be nice to explain what "stronger" actually means.

> --- github.orig/security/device_cgroup.c	2013-01-24 10:40:46.384253615 -0500
> +++ github/security/device_cgroup.c	2013-01-24 10:41:07.513567697 -0500
> @@ -353,13 +353,15 @@ 	return 0;
>   *		won't have more privileges than its parent or to
>   *		verify if a certain access is allowed.
>   * @dev_cgroup: dev cgroup to be tested against
> + * @behavior: behavior of the exception

Should come after @refex?

>   * @refex: new exception
>   */
> -static int may_access(struct dev_cgroup *dev_cgroup,
> -		      struct dev_exception_item *refex)
> +static bool may_access(struct dev_cgroup *dev_cgroup,
> +		       struct dev_exception_item *refex,
> +		       enum devcg_behavior behavior)
>  {
>  	struct dev_exception_item *ex;
> -	bool match = false;
> +	int match = false;
>  
>  	rcu_lockdep_assert(rcu_read_lock_held() ||
>  			   lockdep_is_held(&devcgroup_mutex),
> @@ -380,18 +382,28 @@ 		if (ex->minor != ~0 && ex->minor != re
>  		break;
>  	}
>  
> -	/*
> -	 * In two cases we'll consider this new exception valid:
> -	 * - the dev cgroup has its default policy to allow + exception list:
> -	 *   the new exception should *not* match any of the exceptions
> -	 *   (behavior == DEVCG_DEFAULT_ALLOW, !match)
> -	 * - the dev cgroup has its default policy to deny + exception list:
> -	 *   the new exception *should* match the exceptions
> -	 *   (behavior == DEVCG_DEFAULT_DENY, match)
> -	 */
> -	if ((dev_cgroup->behavior == DEVCG_DEFAULT_DENY) == match)
> -		return 1;
> -	return 0;
> +	if (dev_cgroup->behavior == DEVCG_DEFAULT_ALLOW) {
> +		if (behavior == DEVCG_DEFAULT_ALLOW) {
> +			/* the exception will deny access to certain devices */
> +			return true;
> +		} else {
> +			/* the exception will allow access to certain devices */
> +			if (match)
> +				/*
> +				 * a new exception allowing access shouldn't
> +				 * match an parent's exception
> +				 */
> +				return false;
> +			return true;
> +		}
> +	} else {
> +		/* only behavior == DEVCG_DEFAULT_DENY allowed here */
> +		if (match)
> +			/* parent has an exception that matches the proposed */
> +			return true;
> +		else
> +			return false;

It would be nice if there were a separate patch to decompress the
logic separate from adding new logic.

Thanks.

-- 
tejun