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