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.