[PATCH v15 03/23] LSM: Use lsmblob in security_audit_rule_match

[Paul Moore]

On Mon, Mar 9, 2020 at 7:58 PM Casey Schaufler <casey at 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 at 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 at chromium.org>
> >> Reviewed-by: John Johansen <john.johansen at canonical.com>
> >> Acked-by: Stephen Smalley <sds at tycho.nsa.gov>
> >> Signed-off-by: Casey Schaufler <casey at 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