On Mon, Mar 9, 2020 at 7:58 PM Casey Schaufler <casey(a)schaufler-ca.com> wrote:
On 3/6/2020 2:01 PM, Paul Moore wrote:
> On Fri, Feb 21, 2020 at 7:04 PM Casey Schaufler <casey(a)schaufler-ca.com>
wrote:
>> Change the secid parameter of security_audit_rule_match
>> to a lsmblob structure pointer. Pass the entry from the
>> lsmblob structure for the approprite slot to the LSM hook.
>>
>> Change the users of security_audit_rule_match to use the
>> lsmblob instead of a u32. In some cases this requires a
>> temporary conversion using lsmblob_init() that will go
>> away when other interfaces get converted.
>>
>> Reviewed-by: Kees Cook <keescook(a)chromium.org>
>> Reviewed-by: John Johansen <john.johansen(a)canonical.com>
>> Acked-by: Stephen Smalley <sds(a)tycho.nsa.gov>
>> Signed-off-by: Casey Schaufler <casey(a)schaufler-ca.com>
>> ---
>> include/linux/security.h | 7 ++++---
>> kernel/auditfilter.c | 6 ++++--
>> kernel/auditsc.c | 14 ++++++++++----
>> security/integrity/ima/ima.h | 4 ++--
>> security/integrity/ima/ima_policy.c | 7 +++++--
>> security/security.c | 8 +++++---
>> 6 files changed, 30 insertions(+), 16 deletions(-)
> ...
>
>> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
>> index 3a44abf4fced..509eb21eff7f 100644
>> --- a/kernel/auditfilter.c
>> +++ b/kernel/auditfilter.c
>> @@ -1327,6 +1327,7 @@ int audit_filter(int msgtype, unsigned int listtype)
>> struct audit_field *f = &e->rule.fields[i];
>> pid_t pid;
>> u32 sid;
>> + struct lsmblob blob;
>>
>> switch (f->type) {
>> case AUDIT_PID:
>> @@ -1357,8 +1358,9 @@ int audit_filter(int msgtype, unsigned int listtype)
>> case AUDIT_SUBJ_CLR:
>> if (f->lsm_isset) {
>> security_task_getsecid(current,
&sid);
>> - result = security_audit_rule_match(sid,
>> - f->type, f->op,
>> + lsmblob_init(&blob, sid);
>> + result = security_audit_rule_match(
>> + &blob, f->type,
f->op,
>> f->lsm_rules);
> Unless I'm mistaken this patch is almost exclusively the following pattern:
>
> lsmblob_init(blob, sid);
> security_audit_rule_match(blob, ...);
>
> ... which means we are assigning every array member in @blob the same
> value of "sid" and then sending that into the LSM where each LSM is
> going to then have to index into that array, to all get the same
> value, and then do their match. I'm assuming this will make more
> sense as I progress through the rest of the patchset, but right now it
> seems like we could get by just fine with a u32 here.
When I do the next version I'll put considerably more effort into
explaining the scaffolding strategy. I'm definitely in that area where
the advantage of keeping patches small and the advantage of making a
change obvious are at odds. This will apply to the next few patches in
the series as well.
Yes, it's a hard line to walk. If it helps any, I try to go by two
guiding principles when writing or reviewing a patch:
* each patch must have some standalone value
* each patch must make sense in the context of itself and the code
which preceded it
... which is where the hand-wavy "trust me it's coming" scaffolding
approach tends to fall apart. I'm sure others have more developed
ideas on this, but that is the basis of my comments.
--
paul moore
www.paul-moore.com