Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <30faaa83-bdb3-69b8-1fa9-7c1f4f011045@canonical.com>
Date:   Wed, 5 Jul 2023 11:02:26 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH v1] security: remove duplicate logical judgments in return
To:     Minjie Du <duminjie@vivo.com>, Paul Moore <paul@paul-moore.com>,
        James Morris <jmorris@namei.org>,
        "Serge E. Hallyn" <serge@hallyn.com>,
        Nick Terrell <terrelln@fb.com>,
        "moderated list:APPARMOR SECURITY MODULE" <apparmor@lists.ubuntu.com>,
        "open list:SECURITY SUBSYSTEM" 
        <linux-security-module@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Cc:     opensource.kernel@vivo.com
References: <20230704085653.6443-1-duminjie@vivo.com>
Content-Language: en-US
From:   John Johansen <john.johansen@canonical.com>
Organization: Canonical
In-Reply-To: <20230704085653.6443-1-duminjie@vivo.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 7/4/23 01:56, Minjie Du wrote:
> Fix: delate duplicate logical judgments:
> aa_unpack_u32(e, &perm->allow, NULL);
> Please check this. Thank you!
> 
> Signed-off-by: Minjie Du <duminjie@vivo.com>

NAK, it is a bug but not in a way that we can do this, which will break the unpack. The first entry is reserved, and for the moment should be skipped. Double loading to
&perm->allow, effectively does that but was not what was intended either. There is a patch coming that loads the first entry to a tmp variable and does a check that it is 0.

> ---
>   security/apparmor/policy_unpack.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
> index 694fb7a09..2069adf0a 100644
> --- a/security/apparmor/policy_unpack.c
> +++ b/security/apparmor/policy_unpack.c
> @@ -646,7 +646,6 @@ static bool unpack_perm(struct aa_ext *e, u32 version, struct aa_perms *perm)
>   		return false;
>   
>   	return	aa_unpack_u32(e, &perm->allow, NULL) &&
> -		aa_unpack_u32(e, &perm->allow, NULL) &&
>   		aa_unpack_u32(e, &perm->deny, NULL) &&
>   		aa_unpack_u32(e, &perm->subtree, NULL) &&
>   		aa_unpack_u32(e, &perm->cond, NULL) &&