[apparmor] [PATCH 5/7] Refactor query_label into a base raw fn, and fn built on, top

[Seth Arnold]

Hello, there's two cosmetic issues and one potential bug in this patch.

On Fri, Feb 10, 2017 at 12:52:53PM -0800, John Johansen wrote:
>  /**
>   * aa_query_label - query the access(es) of a label

This is still the old function name.

>   * @mask: permission bits to query
>   * @query: binary query string, must be offset by AA_QUERY_CMD_LABEL_SIZE
>   * @size: size of the query string must include AA_QUERY_CMD_LABEL_SIZE
> - * @allowed: upon successful return, will be 1 if query is allowed and 0 if not
> - * @audited: upon successful return, will be 1 if query should be audited and 0
> - *           if not
> + * @perms: Return: perms for given query
>   *
>   * Returns: 0 on success else -1 and sets errno. If -1 is returned and errno is
>   *          ENOENT, the subject label in the query string is unknown to the
>   *          kernel.
>   */
> -int query_label(uint32_t mask, char *query, size_t size, int *allowed,
> -		int *audited)
> +static int query_label_raw(char *query, size_t size, aa_perms_t *perms)
>  {
>  	char buf[QUERY_LABEL_REPLY_LEN];
> -	uint32_t allow, deny, audit, quiet;
>  	int ret;
>  
> -	if (!mask) {
> -		errno = EINVAL;
> -		return -1;
> -	}
> -
>  	ret = aa_query_cmd(AA_QUERY_CMD_LABEL, AA_QUERY_CMD_LABEL_SIZE, query,
>  			   size, buf, QUERY_LABEL_REPLY_LEN);
>  	if (ret != QUERY_LABEL_REPLY_LEN) {
> @@ -924,16 +919,46 @@ int query_label(uint32_t mask, char *query, size_t size, int *allowed,
>  			  "deny 0x%8"  SCNx32 "\n"
>  			  "audit 0x%8" SCNx32 "\n"
>  			  "quiet 0x%8" SCNx32 "\n",
> -		     &allow, &deny, &audit, &quiet);
> +		     &perms->allow, &perms->deny, &perms->audit, &perms->quiet);
>  	if (ret != 4) {
>  		errno = EPROTONOSUPPORT;
>  		return -1;
>  	}
>  
> -	*allowed = mask & ~(allow & ~deny) ? 0 : 1;
> -	if (!(*allowed))
> -		audit = 0xFFFFFFFF;
> -	*audited = mask & ~(audit & ~quiet) ? 0 : 1;
> +	return 0;
> +}
> +
> +/**
> + * aa_query_label - query the access(es) of a label

The aa_ in this function name doesn't match the implementation.

> + * @mask: permission bits to query
> + * @query: binary query string, must be offset by AA_QUERY_CMD_LABEL_SIZE
> + * @size: size of the query string must include AA_QUERY_CMD_LABEL_SIZE
> + * @allowed: upon successful return, will be 1 if query is allowed and 0 if not
> + * @audited: upon successful return, will be 1 if query should be audited and 0
> + *           if not
> + *
> + * Returns: 0 on success else -1 and sets errno. If -1 is returned and errno is
> + *          ENOENT, the subject label in the query string is unknown to the
> + *          kernel.
> + */
> +int query_label(uint32_t mask, char *query, size_t size, int *allowed,
> +		int *audited)
> +{
> +	aa_perms_t perms;
> +	int ret;
> +
> +	if (!mask) {
> +		errno = EINVAL;
> +		return -1;
> +	}
> +
> +	ret = query_label_raw(query, size, &perms);
> +	if (ret == 0) {
> +		*allowed = mask & ~(perms.allow & ~perms.deny) ? 0 : 1;
> +		if (!(*allowed))
> +			perms.audit = 0xFFFFFFFF;
> +		*audited = mask & ~(perms.audit & ~perms.quiet) ? 0 : 1;
> +	}
>  
>  	return 0;

Should this be 'return ret' instead? If query_label_raw() returns an error,
the query_label() call will still return success.

>  }

Thanks
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20170216/fffb722a/attachment.pgp>