[PATCH ghak73 V2] audit: re-structure audit field valid checks

[Paul Moore]

On Wed, May 8, 2019 at 12:43 PM Richard Guy Briggs <rgb at redhat.com> wrote:
> Multiple checks were being done in one switch case statement that
> started to cause some redundancies and awkward exceptions.  Separate the
> valid field and op check from the select valid values checks.
>
> Enforce the elimination of meaningless bitwise and greater/lessthan
> checks on string fields and other fields with unrelated scalar values.
>
> Honour the operator for WATCH, DIR, PERM, FILETYPE fields as is done in
> the EXE field.
>
> Please see the github issue
> https://github.com/linux-audit/audit-kernel/issues/73
>
> Signed-off-by: Richard Guy Briggs <rgb at redhat.com>
> ---
> Changelog:
> v2:
> - address WATCH, DIR, FILETYPE, PERM lack of op checking
> - touch up switch statement formatting
>
>  kernel/auditfilter.c | 48 ++++++++++++++++++++++++++++++------------------
>  kernel/auditsc.c     | 18 +++++++++++++++---
>  2 files changed, 45 insertions(+), 21 deletions(-)

...

> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 5371b59bde36..4bd0ec60a0e8 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -601,12 +601,20 @@ static int audit_filter_rules(struct task_struct *tsk,
>                         }
>                         break;
>                 case AUDIT_WATCH:
> -                       if (name)
> -                               result = audit_watch_compare(rule->watch, name->ino, name->dev);
> +                       if (name) {
> +                               result = audit_watch_compare(rule->watch,
> +                                                            name->ino,
> +                                                            name->dev);
> +                               if (f->op == Audit_not_equal)
> +                                       result = !result;
> +                       }
>                         break;
>                 case AUDIT_DIR:
> -                       if (ctx)
> +                       if (ctx) {
>                                 result = match_tree_refs(ctx, rule->tree);
> +                               if (f->op == Audit_not_equal)
> +                                       result = !result;
> +                       }
>                         break;
>                 case AUDIT_LOGINUID:
>                         result = audit_uid_comparator(audit_get_loginuid(tsk),
> @@ -684,9 +692,13 @@ static int audit_filter_rules(struct task_struct *tsk,
>                         break;
>                 case AUDIT_PERM:
>                         result = audit_match_perm(ctx, f->val);
> +                       if (f->op == Audit_not_equal)
> +                               result = !result;
>                         break;
>                 case AUDIT_FILETYPE:
>                         result = audit_match_filetype(ctx, f->val);
> +                       if (f->op == Audit_not_equal)
> +                               result = !result;
>                         break;
>                 case AUDIT_FIELD_COMPARE:
>                         result = audit_field_compare(tsk, cred, f, ctx, name);

Other than the fact that Ondrej suggested these changes during review
of this patch's first iteration, is there a reason why you thought it
best to include the audit_filter_rules() changes in with the
audit_field_valid() changes?  I ask because the audit_field_valid()
changes are mostly cosmetic in nature where the audit_filter_rules()
changes actually affect the behavior of the code and there is no
strong connection between the two changes.  It seems like we would be
better off if you split the changes into two patches.

-- 
paul moore
www.paul-moore.com