Message-ID: <20230929.Zausiachi5Eu@digikod.net>
Date:   Fri, 29 Sep 2023 18:04:00 +0200
From:   Mickaël Salaün <mic@...ikod.net>
To:     Jeff Xu <jeffxu@...gle.com>
Cc:     Eric Paris <eparis@...hat.com>, James Morris <jmorris@...ei.org>,
        Paul Moore <paul@...l-moore.com>,
        "Serge E . Hallyn" <serge@...lyn.com>,
        Ben Scarlato <akhna@...gle.com>,
        Günther Noack <gnoack@...gle.com>,
        Jorge Lucangeli Obes <jorgelo@...gle.com>,
        Konstantin Meskhidze <konstantin.meskhidze@...wei.com>,
        Shervin Oloumi <enlightened@...gle.com>, audit@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-security-module@...r.kernel.org
Subject: Re: [RFC PATCH v1 5/7] landlock: Log file-related requests

On Thu, Sep 28, 2023 at 01:04:01PM -0700, Jeff Xu wrote:
> On Thu, Sep 28, 2023 at 8:16 AM Mickaël Salaün <mic@...ikod.net> wrote:
> >
> > On Tue, Sep 26, 2023 at 02:19:51PM -0700, Jeff Xu wrote:
> > > On Tue, Sep 26, 2023 at 6:35 AM Mickaël Salaün <mic@...ikod.net> wrote:
> > > >
> > > > 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@...ikod.net> wrote:
> > > > > >
> > > > > > Add audit support for mkdir, mknod, symlink, unlink, rmdir, truncate,
> > > > > > and open requests.
> > > > > >
> > > > > > Signed-off-by: Mickaël Salaün <mic@...ikod.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),
> > >
> > > "grep -A 2 landlock_log_request" would serve the same purpose though.
> >
> > Yes, there is always a way to find a pattern, and the best tool might be
> > Coccinelle, but I think it's harder to miss with such tail calls.
> >
> > >
> > > > * 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.
> > > >
> > >
> > > What are tail calls and what is the benefit of this code pattern ?
> > > i.e. pass the return value into landlock_log_request() and make it a
> > > single point of setting return value for all landlock hooks.
> >
> > landlock_log_request() should only be called at the end of LSM hooks.
> > Tail calls is basically when you call a function at the end of the
> > caller. This enables replacing "call" with "jmp" and save stack space.
> > landlock_log_request() can fit with this pattern (if not using the
> > caller's stack, which is not currently the case). See this tail call
> > optimization example: https://godbolt.org/z/r88ofcW6x
> >
> Thanks for giving the context of the tail call.
> Compile options are controlled by makefile, and can be customized. In
> the past, I have had different projects setting different O levels for
> various reasons, including disable optimization completely. Individual
> Compiler implementation  also matters, gcc vs clang, etc. I think the
> tail call is probably not essential to the discussion.
> 
> > I find it less error prone to not duplicate the error code (once for
> > landlock_log_request and another for the caller's returned value). I
> > also don't really see the pro of using a variable only to share this
> > value. In ptrace.c, an "err" variable is used to check if the error is 0
> > or not, but that is handled differently for most hooks.
> >
> > Makeing landlock_log_request() return a value also encourages us (thanks
> > to compiler warnings) to use this value and keep the error handling
> > consistent (especially for future code).
> >
> One general assumption about logging function is that it is not part
> of the main business logic, i.e. if the logging statement is
> removed/commented out, it doesn't have side effects to the main
> business logic. This is probably why most log functions return void.

I get it. We need to be careful not to add blind spots though. If audit
is not compiled, the inline function call will just be removed.
Otherwise, logging or not depends on the audit framework and the runtime
configuration.

Another thing to keep in mind is that for now, if the log failed somehow
(e.g. because of not enough memory), it will not impact the decision
(either accept or deny). However, I guess we may want to be able to
control this behavior is some cases one day, and in this case the log
function needs to return an error.

> 
> > Another feature that I'd like to add is to support a "permissive mode",
> > that would enable to quickly see the impact of a sandbox without
> > applying it for real. This might change some landlock_log_request()
> > calls, so we'll see what fits best.
> >
> It is an excellent feature to have.
> To implement that, I guess there will be a common function as a switch
> to allow/deny, and logging the decision, depending on the permissive
> setting.

Permissive mode will be per domain/sandbox, so it will add complexity to
the current logging mechanism, but that is worth it.

> From that point, preparing the code towards that goal makes sense.
> 
> > >
> > > > 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).
> When there is more business logic to landlock_log_request, it is
> probably better to rename the function. Most devs might assume the log
> function does nothing but logging.  Having some meaningful name, e.g.
> check_permissive_and_audit_log,  will help with readability.

As for the permissive mode, this would be per domain.

I'd like to keep the audit.h functions with the same prefix.
What about landlock_log_result()?

> 
> Thanks!
> -Jeff

Powered by blists - more mailing lists