On Thu, Sep 28, 2023 at 8:16 AM Mickaël Salaün [off-list ref] wrote:

On Tue, Sep 26, 2023 at 02:19:51PM -0700, Jeff Xu wrote:

quoted

On Tue, Sep 26, 2023 at 6:35 AM Mickaël Salaün [off-list ref] wrote:

quoted

On Mon, Sep 25, 2023 at 06:26:28PM -0700, Jeff Xu wrote:

quoted

On Wed, Sep 20, 2023 at 11:17 PM Mickaël Salaün [off-list ref] wrote:

quoted

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(-)

quoted

quoted

+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;
+}

quoted

quoted

@@ -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.

quoted

quoted

* 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.

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.
From that point, preparing the code towards that goal makes sense.

quoted

quoted

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.

Thanks!
-Jeff