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

Attachment: signature.asc
Description: PGP signature

-- 
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/apparmor

Reply via email to