Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <cover.1652730821.git.rgb@redhat.com> <81264e038b7b1e0d8fd8bafb25452fb777cd664a.1652730821.git.rgb@redhat.com>
In-Reply-To: <81264e038b7b1e0d8fd8bafb25452fb777cd664a.1652730821.git.rgb@redhat.com>
From:   Paul Moore <paul@paul-moore.com>
Date:   Mon, 16 May 2022 21:42:55 -0400
Message-ID: <CAHC9VhSZNbQoFfStWp96G18_pdEtV1orKRvQ0reXfD7L4TiUHA@mail.gmail.com>
Subject: Re: [PATCH v3 3/3] fanotify: Allow audit to use the full permission
 event response
To:     Richard Guy Briggs <rgb@redhat.com>
Cc:     Linux-Audit Mailing List <linux-audit@redhat.com>,
        LKML <linux-kernel@vger.kernel.org>,
        linux-fsdevel@vger.kernel.org, Eric Paris <eparis@parisplace.org>,
        Steve Grubb <sgrubb@redhat.com>, Jan Kara <jack@suse.cz>,
        Amir Goldstein <amir73il@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, May 16, 2022 at 4:22 PM Richard Guy Briggs <rgb@redhat.com> wrote:
>
> This patch passes the full value so that the audit function can use all
> of it. The audit function was updated to log the additional information in
> the AUDIT_FANOTIFY record. The following is an example of the new record
> format:
>
> type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_ctx=17
>
> Suggested-by: Steve Grubb <sgrubb@redhat.com>
> Link: https://lore.kernel.org/r/3075502.aeNJFYEL58@x2
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  fs/notify/fanotify/fanotify.c |  4 +++-
>  include/linux/audit.h         |  9 +++++----
>  kernel/auditsc.c              | 18 +++++++++++++++---
>  3 files changed, 23 insertions(+), 8 deletions(-)

...

> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 6973be0bf6c9..cb93c6ed07cd 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2893,10 +2894,21 @@ void __audit_log_kern_module(char *name)
>         context->type = AUDIT_KERN_MODULE;
>  }
>
> -void __audit_fanotify(u32 response)
> +void __audit_fanotify(u32 response, u32 type, union fanotify_response_extra *info)
>  {
> -       audit_log(audit_context(), GFP_KERNEL,
> -               AUDIT_FANOTIFY, "resp=%u", response);
> +       switch (type) {
> +       case FAN_RESPONSE_INFO_AUDIT_RULE:
> +               audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> +                         "resp=%u fan_type=%u fan_ctx=%u",
> +                         response, type, info->audit_rule);
> +               break;
> +       case FAN_RESPONSE_INFO_NONE:
> +       default:
> +               audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> +                         "resp=%u fan_type=%u fan_ctx=?",
> +                         response, type);
> +               break;
> +       }
>  }

Two things:

* Instead of "fan_ctx=", would it make sense to call it "fan_extra="
to better match the UAPI struct?  I don't feel strongly either way,
but it did occur to me just now while looking at the code so I thought
I would mention it.
* I'm also wondering if there is a way to be a bit proactive about
future proofing this field.  Since we already hex encode some fields
with "bad" characters, would it make sense to hex encode this field
too?  Not for the "bad" character reason, but more as a way of
marshalling the fanotify_response_extra union into an audit record.  I
can't see far enough into the future to know if this would be a good
idea or not, but like the other point above, it popped into my head
while looking at the code so I thought I would put it in the email :)

--
paul-moore.com