Received-SPF: pass (google.com: domain of linux-kernel+bounces-94299-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <5c99f987-d359-4366-984f-fe36fcff601b@schaufler-ca.com>
Date: Wed, 6 Mar 2024 08:56:57 -0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v39 01/42] integrity: disassociate ima_filter_rule from
 security_audit_rule
To: Roberto Sassu <roberto.sassu@huaweicloud.com>, paul@paul-moore.com,
 linux-security-module@vger.kernel.org
Cc: jmorris@namei.org, serge@hallyn.com, keescook@chromium.org,
 john.johansen@canonical.com, penguin-kernel@i-love.sakura.ne.jp,
 stephen.smalley.work@gmail.com, linux-kernel@vger.kernel.org,
 mic@digikod.net, linux-integrity@vger.kernel.org,
 Casey Schaufler <casey@schaufler-ca.com>
References: <20231215221636.105680-1-casey@schaufler-ca.com>
 <20231215221636.105680-2-casey@schaufler-ca.com>
 <d4cbe23822f7fdac900d1ebd5da9865d8bb96977.camel@huaweicloud.com>
Content-Language: en-US
From: Casey Schaufler <casey@schaufler-ca.com>
In-Reply-To: <d4cbe23822f7fdac900d1ebd5da9865d8bb96977.camel@huaweicloud.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 3/6/2024 1:54 AM, Roberto Sassu wrote:
> On Fri, 2023-12-15 at 14:15 -0800, Casey Schaufler wrote:
>> Create real functions for the ima_filter_rule interfaces.
>> These replace #defines that obscure the reuse of audit
>> interfaces. The new functions are put in security.c because
>> they use security module registered hooks that we don't
>> want exported.
> Beginner question: what makes IMA special, that the audit subsystem
> does not need an AUDIT_LSM field to do the same that IMA would do?
>
> In other words, why can't we add the lsm_id parameter to
> security_audit_*() functions, so that IMA can just call those?

I have never liked the reuse of the audit filter functions, especially
the way that they're hidden behind #define. The assumption that the
two facilities (audit and IMA) are going to use them the same way, and
that they will never diverge in their requirements, has always seemed
questionable.

In most cases audit needs an lsmblob rather than a secid+lsm_id pair,
as there is information required about all the LSMs, not just the one
actively involved.

>
> Thanks
>
> Roberto
>
>> Acked-by: Paul Moore <paul@paul-moore.com>
>> Reviewed-by: John Johansen <john.johansen@canonical.com>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> To: Mimi Zohar <zohar@linux.ibm.com>
>> Cc: linux-integrity@vger.kernel.org
>> ---
>>  include/linux/security.h     | 24 ++++++++++++++++++++++++
>>  security/integrity/ima/ima.h | 26 --------------------------
>>  security/security.c          | 21 +++++++++++++++++++++
>>  3 files changed, 45 insertions(+), 26 deletions(-)
>>
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index 750130a7b9dd..4790508818ee 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -2009,6 +2009,30 @@ static inline void security_audit_rule_free(void *lsmrule)
>>  #endif /* CONFIG_SECURITY */
>>  #endif /* CONFIG_AUDIT */
>>  
>> +#if defined(CONFIG_IMA_LSM_RULES) && defined(CONFIG_SECURITY)
>> +int ima_filter_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule);
>> +int ima_filter_rule_match(u32 secid, u32 field, u32 op, void *lsmrule);
>> +void ima_filter_rule_free(void *lsmrule);
>> +
>> +#else
>> +
>> +static inline int ima_filter_rule_init(u32 field, u32 op, char *rulestr,
>> +					   void **lsmrule)
>> +{
>> +	return 0;
>> +}
>> +
>> +static inline int ima_filter_rule_match(u32 secid, u32 field, u32 op,
>> +					    void *lsmrule)
>> +{
>> +	return 0;
>> +}
>> +
>> +static inline void ima_filter_rule_free(void *lsmrule)
>> +{ }
>> +
>> +#endif /* defined(CONFIG_IMA_LSM_RULES) && defined(CONFIG_SECURITY) */
>> +
>>  #ifdef CONFIG_SECURITYFS
>>  
>>  extern struct dentry *securityfs_create_file(const char *name, umode_t mode,
>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>> index c29db699c996..560d6104de72 100644
>> --- a/security/integrity/ima/ima.h
>> +++ b/security/integrity/ima/ima.h
>> @@ -420,32 +420,6 @@ static inline void ima_free_modsig(struct modsig *modsig)
>>  }
>>  #endif /* CONFIG_IMA_APPRAISE_MODSIG */
>>  
>> -/* LSM based policy rules require audit */
>> -#ifdef CONFIG_IMA_LSM_RULES
>> -
>> -#define ima_filter_rule_init security_audit_rule_init
>> -#define ima_filter_rule_free security_audit_rule_free
>> -#define ima_filter_rule_match security_audit_rule_match
>> -
>> -#else
>> -
>> -static inline int ima_filter_rule_init(u32 field, u32 op, char *rulestr,
>> -				       void **lsmrule)
>> -{
>> -	return -EINVAL;
>> -}
>> -
>> -static inline void ima_filter_rule_free(void *lsmrule)
>> -{
>> -}
>> -
>> -static inline int ima_filter_rule_match(u32 secid, u32 field, u32 op,
>> -					void *lsmrule)
>> -{
>> -	return -EINVAL;
>> -}
>> -#endif /* CONFIG_IMA_LSM_RULES */
>> -
>>  #ifdef	CONFIG_IMA_READ_POLICY
>>  #define	POLICY_FILE_FLAGS	(S_IWUSR | S_IRUSR)
>>  #else
>> diff --git a/security/security.c b/security/security.c
>> index d7b15ea67c3f..8e5379a76369 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -5350,6 +5350,27 @@ int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
>>  }
>>  #endif /* CONFIG_AUDIT */
>>  
>> +#ifdef CONFIG_IMA_LSM_RULES
>> +/*
>> + * The integrity subsystem uses the same hooks as
>> + * the audit subsystem.
>> + */
>> +int ima_filter_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule)
>> +{
>> +	return call_int_hook(audit_rule_init, 0, field, op, rulestr, lsmrule);
>> +}
>> +
>> +void ima_filter_rule_free(void *lsmrule)
>> +{
>> +	call_void_hook(audit_rule_free, lsmrule);
>> +}
>> +
>> +int ima_filter_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
>> +{
>> +	return call_int_hook(audit_rule_match, 0, secid, field, op, lsmrule);
>> +}
>> +#endif /* CONFIG_IMA_LSM_RULES */
>> +
>>  #ifdef CONFIG_BPF_SYSCALL
>>  /**
>>   * security_bpf() - Check if the bpf syscall operation is allowed