On Thu, Sep 28, 2023 at 8:16 AM Mickaël Salaün <[email protected]> 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 <[email protected]> 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 <[email protected]> > > > > wrote: > > > > > > > > > > Add audit support for mkdir, mknod, symlink, unlink, rmdir, truncate, > > > > > and open requests. > > > > > > > > > > Signed-off-by: Mickaël Salaün <[email protected]> > > > > > --- > > > > > 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. > 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. > > > > > 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
