On Mon, Mar 9, 2020 at 7:58 PM Casey Schaufler <[email protected]> wrote: > On 3/6/2020 2:01 PM, Paul Moore wrote: > > On Fri, Feb 21, 2020 at 7:04 PM Casey Schaufler <[email protected]> > > 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 <[email protected]> > >> Reviewed-by: John Johansen <[email protected]> > >> Acked-by: Stephen Smalley <[email protected]> > >> Signed-off-by: Casey Schaufler <[email protected]> > >> --- > >> 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 -- Linux-audit mailing list [email protected] https://www.redhat.com/mailman/listinfo/linux-audit
