Re: [RFC PATCH v1 5/7] landlock: Log file-related requests

["Mickaël Salaün" <mic@digikod.net>]

On Mon, Sep 25, 2023 at 06:26:28PM -0700, Jeff Xu wrote:
> On Wed, Sep 20, 2023 at 11:17 PM Mickaël Salaün <mic@digikod.net> wrote:
> >
> > Add audit support for mkdir, mknod, symlink, unlink, rmdir, truncate,
> > and open requests.
> >
> > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > ---
> >  security/landlock/audit.c | 114 ++++++++++++++++++++++++++++++++++++++
> >  security/landlock/audit.h |  32 +++++++++++
> >  security/landlock/fs.c    |  62 ++++++++++++++++++---
> >  3 files changed, 199 insertions(+), 9 deletions(-)
> >

> > +static void
> > +log_request(const int error, struct landlock_request *const request,
> > +           const struct landlock_ruleset *const domain,
> > +           const access_mask_t access_request,
> > +           const layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS])
> > +{
> > +       struct audit_buffer *ab;
> > +
> > +       if (WARN_ON_ONCE(!error))
> > +               return;
> > +       if (WARN_ON_ONCE(!request))
> > +               return;
> > +       if (WARN_ON_ONCE(!domain || !domain->hierarchy))
> > +               return;
> > +
> > +       /* Uses GFP_ATOMIC to not sleep. */
> > +       ab = audit_log_start(audit_context(), GFP_ATOMIC | __GFP_NOWARN,
> > +                            AUDIT_LANDLOCK);
> > +       if (!ab)
> > +               return;
> > +
> > +       update_request(request, domain, access_request, layer_masks);
> > +
> > +       log_task(ab);
> > +       audit_log_format(ab, " domain=%llu op=%s errno=%d missing-fs-accesses=",
> > +                        request->youngest_domain,
> > +                        op_to_string(request->operation), -error);
> > +       log_accesses(ab, request->missing_access);
> > +       audit_log_lsm_data(ab, &request->audit);
> > +       audit_log_end(ab);
> > +}
> > +
> > +// TODO: Make it generic, not FS-centric.
> > +int landlock_log_request(
> > +       const int error, struct landlock_request *const request,
> > +       const struct landlock_ruleset *const domain,
> > +       const access_mask_t access_request,
> > +       const layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS])
> > +{
> > +       /* No need to log the access request, only the missing accesses. */
> > +       log_request(error, request, domain, access_request, layer_masks);
> > +       return error;
> > +}

> > @@ -636,7 +638,8 @@ static bool is_access_to_paths_allowed(
> >  }
> >
> >  static int current_check_access_path(const struct path *const path,
> > -                                    access_mask_t access_request)
> > +                                    access_mask_t access_request,
> > +                                    struct landlock_request *const request)
> >  {
> >         const struct landlock_ruleset *const dom =
> >                 landlock_get_current_domain();
> > @@ -650,7 +653,10 @@ static int current_check_access_path(const struct path *const path,
> >                                        NULL, 0, NULL, NULL))
> >                 return 0;
> >
> > -       return -EACCES;
> > +       request->audit.type = LSM_AUDIT_DATA_PATH;
> > +       request->audit.u.path = *path;
> > +       return landlock_log_request(-EACCES, request, dom, access_request,
> > +                                   &layer_masks);
> 
> It might be more readable to let landlock_log_request return void.
> Then the code will look like below.
> 
> landlock_log_request(-EACCES, request, dom, access_request,  &layer_masks);
> return -EACCES;
> 
> The allow/deny logic will be in this function, i.e. reader
> doesn't need to check landlock_log_request's implementation to find
> out it never returns 0.

I did that in an early version of this patch, but I finally choose to write
'return lanlock_log_request();` for mainly two reasons:
* to help not forget to call this function at any non-zero return values
  (which can easily be checked with grep),
* to do tail calls.

I guess compiler should be smart enough to do tail calls with a variable
set indirection, but I'd like to check that.

To make it easier to read (and to not forget returning the error), the
landlock_log_request() calls a void log_request() helper, and returns
the error itself. It is then easy to review and know what's happening
without reading log_request().

I'd like the compiler to check itself that every LSM hook returned
values are either 0 or comming from landlock_log_request() but I think
it's not possible right now. Coccinelle might help here though.

BTW, in a next version, we might have landlock_log_request() called even
for allowed requests (i.e. returned value of 0).