| Message ID | 20230921061641.273654-6-mic@digikod.net (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | Landlock audit support | expand |
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(-) > > diff --git a/security/landlock/audit.c b/security/landlock/audit.c > index d9589d07e126..148fc0fafef4 100644 > --- a/security/landlock/audit.c > +++ b/security/landlock/audit.c > @@ -14,6 +14,25 @@ > > atomic64_t ruleset_and_domain_counter = ATOMIC64_INIT(0); > > +static const char *op_to_string(enum landlock_operation operation) > +{ > + const char *const desc[] = { > + [0] = "", > + [LANDLOCK_OP_MKDIR] = "mkdir", > + [LANDLOCK_OP_MKNOD] = "mknod", > + [LANDLOCK_OP_SYMLINK] = "symlink", > + [LANDLOCK_OP_UNLINK] = "unlink", > + [LANDLOCK_OP_RMDIR] = "rmdir", > + [LANDLOCK_OP_TRUNCATE] = "truncate", > + [LANDLOCK_OP_OPEN] = "open", > + }; > + > + if (WARN_ON_ONCE(operation < 0 || operation > ARRAY_SIZE(desc))) > + return "unknown"; > + > + return desc[operation]; > +} > + > #define BIT_INDEX(bit) HWEIGHT(bit - 1) > > static void log_accesses(struct audit_buffer *const ab, > @@ -141,3 +160,98 @@ void landlock_log_release_ruleset(const struct landlock_ruleset *const ruleset) > audit_log_format(ab, "op=release-%s %s=%llu", name, name, id); > audit_log_end(ab); > } > + > +/* Update request.youngest_domain and request.missing_access */ > +static void > +update_request(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]) > +{ > + const unsigned long access_req = access_request; > + unsigned long access_bit; > + long youngest_denied_layer = -1; > + const struct landlock_hierarchy *node = domain->hierarchy; > + size_t i; > + > + WARN_ON_ONCE(request->youngest_domain); > + WARN_ON_ONCE(request->missing_access); > + > + if (WARN_ON_ONCE(!access_request)) > + return; > + > + if (WARN_ON_ONCE(!layer_masks)) > + return; > + > + for_each_set_bit(access_bit, &access_req, ARRAY_SIZE(*layer_masks)) { > + long domain_layer; > + > + if (!(*layer_masks)[access_bit]) > + continue; > + > + domain_layer = __fls((*layer_masks)[access_bit]); > + > + /* > + * Gets the access rights that are missing from > + * the youngest (i.e. closest) domain. > + */ > + if (domain_layer == youngest_denied_layer) { > + request->missing_access |= BIT_ULL(access_bit); > + } else if (domain_layer > youngest_denied_layer) { > + youngest_denied_layer = domain_layer; > + request->missing_access = BIT_ULL(access_bit); > + } > + } > + > + WARN_ON_ONCE(!request->missing_access); > + WARN_ON_ONCE(youngest_denied_layer < 0); > + > + /* Gets the nearest domain ID that denies request.missing_access */ > + for (i = domain->num_layers - youngest_denied_layer - 1; i > 0; i--) > + node = node->parent; > + request->youngest_domain = node->id; > +} > + > +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; > +} > diff --git a/security/landlock/audit.h b/security/landlock/audit.h > index bc17dc8ca6f1..8edc68b98fca 100644 > --- a/security/landlock/audit.h > +++ b/security/landlock/audit.h > @@ -13,6 +13,23 @@ > > #include "ruleset.h" > > +enum landlock_operation { > + LANDLOCK_OP_MKDIR = 1, > + LANDLOCK_OP_MKNOD, > + LANDLOCK_OP_SYMLINK, > + LANDLOCK_OP_UNLINK, > + LANDLOCK_OP_RMDIR, > + LANDLOCK_OP_TRUNCATE, > + LANDLOCK_OP_OPEN, > +}; > + > +struct landlock_request { > + const enum landlock_operation operation; > + access_mask_t missing_access; > + u64 youngest_domain; > + struct common_audit_data audit; > +}; > + > #ifdef CONFIG_AUDIT > > void landlock_log_create_ruleset(struct landlock_ruleset *const ruleset); > @@ -20,6 +37,12 @@ void landlock_log_restrict_self(struct landlock_ruleset *const domain, > struct landlock_ruleset *const ruleset); > void landlock_log_release_ruleset(const struct landlock_ruleset *const ruleset); > > +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]); > + > #else /* CONFIG_AUDIT */ > > static inline void > @@ -38,6 +61,15 @@ landlock_log_release_ruleset(const struct landlock_ruleset *const ruleset) > { > } > > +static inline 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]) > +{ > + return error; > +} > + > #endif /* CONFIG_AUDIT */ > > #endif /* _SECURITY_LANDLOCK_AUDIT_H */ > diff --git a/security/landlock/fs.c b/security/landlock/fs.c > index 978e325d8708..104dfb2abc32 100644 > --- a/security/landlock/fs.c > +++ b/security/landlock/fs.c > @@ -18,6 +18,7 @@ > #include <linux/kernel.h> > #include <linux/limits.h> > #include <linux/list.h> > +#include <linux/lsm_audit.h> > #include <linux/lsm_hooks.h> > #include <linux/mount.h> > #include <linux/namei.h> > @@ -30,6 +31,7 @@ > #include <linux/workqueue.h> > #include <uapi/linux/landlock.h> > > +#include "audit.h" > #include "common.h" > #include "cred.h" > #include "fs.h" > @@ -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. -Jeff > } > > static inline access_mask_t get_mode_access(const umode_t mode) > @@ -1097,6 +1103,7 @@ static int hook_path_link(struct dentry *const old_dentry, > const struct path *const new_dir, > struct dentry *const new_dentry) > { > + // TODO: Implement fine-grained audit > return current_check_refer_path(old_dentry, new_dir, new_dentry, false, > false); > } > @@ -1115,38 +1122,67 @@ static int hook_path_rename(const struct path *const old_dir, > static int hook_path_mkdir(const struct path *const dir, > struct dentry *const dentry, const umode_t mode) > { > - return current_check_access_path(dir, LANDLOCK_ACCESS_FS_MAKE_DIR); > + struct landlock_request request = { > + .operation = LANDLOCK_OP_MKDIR, > + }; > + > + return current_check_access_path(dir, LANDLOCK_ACCESS_FS_MAKE_DIR, > + &request); > } > > static int hook_path_mknod(const struct path *const dir, > struct dentry *const dentry, const umode_t mode, > const unsigned int dev) > { > - return current_check_access_path(dir, get_mode_access(mode)); > + struct landlock_request request = { > + .operation = LANDLOCK_OP_MKNOD, > + }; > + > + return current_check_access_path(dir, get_mode_access(mode), &request); > } > > static int hook_path_symlink(const struct path *const dir, > struct dentry *const dentry, > const char *const old_name) > { > - return current_check_access_path(dir, LANDLOCK_ACCESS_FS_MAKE_SYM); > + struct landlock_request request = { > + .operation = LANDLOCK_OP_SYMLINK, > + }; > + > + return current_check_access_path(dir, LANDLOCK_ACCESS_FS_MAKE_SYM, > + &request); > } > > static int hook_path_unlink(const struct path *const dir, > struct dentry *const dentry) > { > - return current_check_access_path(dir, LANDLOCK_ACCESS_FS_REMOVE_FILE); > + struct landlock_request request = { > + .operation = LANDLOCK_OP_UNLINK, > + }; > + > + return current_check_access_path(dir, LANDLOCK_ACCESS_FS_REMOVE_FILE, > + &request); > } > > static int hook_path_rmdir(const struct path *const dir, > struct dentry *const dentry) > { > - return current_check_access_path(dir, LANDLOCK_ACCESS_FS_REMOVE_DIR); > + struct landlock_request request = { > + .operation = LANDLOCK_OP_RMDIR, > + }; > + > + return current_check_access_path(dir, LANDLOCK_ACCESS_FS_REMOVE_DIR, > + &request); > } > > static int hook_path_truncate(const struct path *const path) > { > - return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE); > + struct landlock_request request = { > + .operation = LANDLOCK_OP_TRUNCATE, > + }; > + > + return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE, > + &request); > } > > /* File hooks */ > @@ -1199,6 +1235,13 @@ static int hook_file_open(struct file *const file) > const access_mask_t optional_access = LANDLOCK_ACCESS_FS_TRUNCATE; > const struct landlock_ruleset *const dom = > landlock_get_current_domain(); > + struct landlock_request request = { > + .operation = LANDLOCK_OP_OPEN, > + .audit = { > + .type = LSM_AUDIT_DATA_PATH, > + .u.path = file->f_path, > + }, > + }; > > if (!dom) > return 0; > @@ -1249,7 +1292,8 @@ static int hook_file_open(struct file *const file) > if ((open_access_request & allowed_access) == open_access_request) > return 0; > > - return -EACCES; > + return landlock_log_request(-EACCES, &request, dom, open_access_request, > + &layer_masks); > } > > static int hook_file_truncate(struct file *const file) > -- > 2.42.0 >
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).
On Tue, Sep 26, 2023 at 6:35 AM Mickaël Salaün <mic@digikod.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@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), "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).
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@digikod.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@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), > > "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 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). 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. > > > 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).
On Thu, Sep 28, 2023 at 8:16 AM Mickaël Salaün <mic@digikod.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@digikod.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@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), > > > > "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
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@digikod.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@digikod.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@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), > > > > > > "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
On Fri, Sep 29, 2023 at 9:04 AM Mickaël Salaün <mic@digikod.net> wrote: > > 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@digikod.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@digikod.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@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), > > > > > > > > "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. > That is really nice to have. > I'd like to keep the audit.h functions with the same prefix. > What about landlock_log_result()? > I'm fine with keeping landlock_log_request in this version since it is only temporary, or landlock_log_result() is better. In the future version, landlock_log_result() still doesn't quite say it might override the result, so maybe extract the audit logging out of that at the time. > > > > Thanks! > > -Jeff
On Thu, Sep 21, 2023 at 2:17 AM 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(-) > > diff --git a/security/landlock/audit.c b/security/landlock/audit.c > index d9589d07e126..148fc0fafef4 100644 > --- a/security/landlock/audit.c > +++ b/security/landlock/audit.c > @@ -14,6 +14,25 @@ > > atomic64_t ruleset_and_domain_counter = ATOMIC64_INIT(0); > > +static const char *op_to_string(enum landlock_operation operation) > +{ > + const char *const desc[] = { > + [0] = "", > + [LANDLOCK_OP_MKDIR] = "mkdir", > + [LANDLOCK_OP_MKNOD] = "mknod", > + [LANDLOCK_OP_SYMLINK] = "symlink", > + [LANDLOCK_OP_UNLINK] = "unlink", > + [LANDLOCK_OP_RMDIR] = "rmdir", > + [LANDLOCK_OP_TRUNCATE] = "truncate", > + [LANDLOCK_OP_OPEN] = "open", > + }; If you're going to be using a single AUDIT_LANDLOCK record type, do you want to somehow encode that the above are access/permission requests in the "op=" field name? > +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); > +} See my previous comments about record format consistency. -- paul-moore.com
On Wed, Dec 20, 2023 at 04:22:33PM -0500, Paul Moore wrote: > On Thu, Sep 21, 2023 at 2:17 AM 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(-) > > > > diff --git a/security/landlock/audit.c b/security/landlock/audit.c > > index d9589d07e126..148fc0fafef4 100644 > > --- a/security/landlock/audit.c > > +++ b/security/landlock/audit.c > > @@ -14,6 +14,25 @@ > > > > atomic64_t ruleset_and_domain_counter = ATOMIC64_INIT(0); > > > > +static const char *op_to_string(enum landlock_operation operation) > > +{ > > + const char *const desc[] = { > > + [0] = "", > > + [LANDLOCK_OP_MKDIR] = "mkdir", > > + [LANDLOCK_OP_MKNOD] = "mknod", > > + [LANDLOCK_OP_SYMLINK] = "symlink", > > + [LANDLOCK_OP_UNLINK] = "unlink", > > + [LANDLOCK_OP_RMDIR] = "rmdir", > > + [LANDLOCK_OP_TRUNCATE] = "truncate", > > + [LANDLOCK_OP_OPEN] = "open", > > + }; > > If you're going to be using a single AUDIT_LANDLOCK record type, do > you want to somehow encode that the above are access/permission > requests in the "op=" field name? I'll use several audit record types, one for a denial and others for the related kernel objects. See my other reply. > > > +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); > > +} > > See my previous comments about record format consistency. right > > -- > paul-moore.com >
diff --git a/security/landlock/audit.c b/security/landlock/audit.c index d9589d07e126..148fc0fafef4 100644 --- a/security/landlock/audit.c +++ b/security/landlock/audit.c @@ -14,6 +14,25 @@ atomic64_t ruleset_and_domain_counter = ATOMIC64_INIT(0); +static const char *op_to_string(enum landlock_operation operation) +{ + const char *const desc[] = { + [0] = "", + [LANDLOCK_OP_MKDIR] = "mkdir", + [LANDLOCK_OP_MKNOD] = "mknod", + [LANDLOCK_OP_SYMLINK] = "symlink", + [LANDLOCK_OP_UNLINK] = "unlink", + [LANDLOCK_OP_RMDIR] = "rmdir", + [LANDLOCK_OP_TRUNCATE] = "truncate", + [LANDLOCK_OP_OPEN] = "open", + }; + + if (WARN_ON_ONCE(operation < 0 || operation > ARRAY_SIZE(desc))) + return "unknown"; + + return desc[operation]; +} + #define BIT_INDEX(bit) HWEIGHT(bit - 1) static void log_accesses(struct audit_buffer *const ab, @@ -141,3 +160,98 @@ void landlock_log_release_ruleset(const struct landlock_ruleset *const ruleset) audit_log_format(ab, "op=release-%s %s=%llu", name, name, id); audit_log_end(ab); } + +/* Update request.youngest_domain and request.missing_access */ +static void +update_request(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]) +{ + const unsigned long access_req = access_request; + unsigned long access_bit; + long youngest_denied_layer = -1; + const struct landlock_hierarchy *node = domain->hierarchy; + size_t i; + + WARN_ON_ONCE(request->youngest_domain); + WARN_ON_ONCE(request->missing_access); + + if (WARN_ON_ONCE(!access_request)) + return; + + if (WARN_ON_ONCE(!layer_masks)) + return; + + for_each_set_bit(access_bit, &access_req, ARRAY_SIZE(*layer_masks)) { + long domain_layer; + + if (!(*layer_masks)[access_bit]) + continue; + + domain_layer = __fls((*layer_masks)[access_bit]); + + /* + * Gets the access rights that are missing from + * the youngest (i.e. closest) domain. + */ + if (domain_layer == youngest_denied_layer) { + request->missing_access |= BIT_ULL(access_bit); + } else if (domain_layer > youngest_denied_layer) { + youngest_denied_layer = domain_layer; + request->missing_access = BIT_ULL(access_bit); + } + } + + WARN_ON_ONCE(!request->missing_access); + WARN_ON_ONCE(youngest_denied_layer < 0); + + /* Gets the nearest domain ID that denies request.missing_access */ + for (i = domain->num_layers - youngest_denied_layer - 1; i > 0; i--) + node = node->parent; + request->youngest_domain = node->id; +} + +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; +} diff --git a/security/landlock/audit.h b/security/landlock/audit.h index bc17dc8ca6f1..8edc68b98fca 100644 --- a/security/landlock/audit.h +++ b/security/landlock/audit.h @@ -13,6 +13,23 @@ #include "ruleset.h" +enum landlock_operation { + LANDLOCK_OP_MKDIR = 1, + LANDLOCK_OP_MKNOD, + LANDLOCK_OP_SYMLINK, + LANDLOCK_OP_UNLINK, + LANDLOCK_OP_RMDIR, + LANDLOCK_OP_TRUNCATE, + LANDLOCK_OP_OPEN, +}; + +struct landlock_request { + const enum landlock_operation operation; + access_mask_t missing_access; + u64 youngest_domain; + struct common_audit_data audit; +}; + #ifdef CONFIG_AUDIT void landlock_log_create_ruleset(struct landlock_ruleset *const ruleset); @@ -20,6 +37,12 @@ void landlock_log_restrict_self(struct landlock_ruleset *const domain, struct landlock_ruleset *const ruleset); void landlock_log_release_ruleset(const struct landlock_ruleset *const ruleset); +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]); + #else /* CONFIG_AUDIT */ static inline void @@ -38,6 +61,15 @@ landlock_log_release_ruleset(const struct landlock_ruleset *const ruleset) { } +static inline 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]) +{ + return error; +} + #endif /* CONFIG_AUDIT */ #endif /* _SECURITY_LANDLOCK_AUDIT_H */ diff --git a/security/landlock/fs.c b/security/landlock/fs.c index 978e325d8708..104dfb2abc32 100644 --- a/security/landlock/fs.c +++ b/security/landlock/fs.c @@ -18,6 +18,7 @@ #include <linux/kernel.h> #include <linux/limits.h> #include <linux/list.h> +#include <linux/lsm_audit.h> #include <linux/lsm_hooks.h> #include <linux/mount.h> #include <linux/namei.h> @@ -30,6 +31,7 @@ #include <linux/workqueue.h> #include <uapi/linux/landlock.h> +#include "audit.h" #include "common.h" #include "cred.h" #include "fs.h" @@ -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); } static inline access_mask_t get_mode_access(const umode_t mode) @@ -1097,6 +1103,7 @@ static int hook_path_link(struct dentry *const old_dentry, const struct path *const new_dir, struct dentry *const new_dentry) { + // TODO: Implement fine-grained audit return current_check_refer_path(old_dentry, new_dir, new_dentry, false, false); } @@ -1115,38 +1122,67 @@ static int hook_path_rename(const struct path *const old_dir, static int hook_path_mkdir(const struct path *const dir, struct dentry *const dentry, const umode_t mode) { - return current_check_access_path(dir, LANDLOCK_ACCESS_FS_MAKE_DIR); + struct landlock_request request = { + .operation = LANDLOCK_OP_MKDIR, + }; + + return current_check_access_path(dir, LANDLOCK_ACCESS_FS_MAKE_DIR, + &request); } static int hook_path_mknod(const struct path *const dir, struct dentry *const dentry, const umode_t mode, const unsigned int dev) { - return current_check_access_path(dir, get_mode_access(mode)); + struct landlock_request request = { + .operation = LANDLOCK_OP_MKNOD, + }; + + return current_check_access_path(dir, get_mode_access(mode), &request); } static int hook_path_symlink(const struct path *const dir, struct dentry *const dentry, const char *const old_name) { - return current_check_access_path(dir, LANDLOCK_ACCESS_FS_MAKE_SYM); + struct landlock_request request = { + .operation = LANDLOCK_OP_SYMLINK, + }; + + return current_check_access_path(dir, LANDLOCK_ACCESS_FS_MAKE_SYM, + &request); } static int hook_path_unlink(const struct path *const dir, struct dentry *const dentry) { - return current_check_access_path(dir, LANDLOCK_ACCESS_FS_REMOVE_FILE); + struct landlock_request request = { + .operation = LANDLOCK_OP_UNLINK, + }; + + return current_check_access_path(dir, LANDLOCK_ACCESS_FS_REMOVE_FILE, + &request); } static int hook_path_rmdir(const struct path *const dir, struct dentry *const dentry) { - return current_check_access_path(dir, LANDLOCK_ACCESS_FS_REMOVE_DIR); + struct landlock_request request = { + .operation = LANDLOCK_OP_RMDIR, + }; + + return current_check_access_path(dir, LANDLOCK_ACCESS_FS_REMOVE_DIR, + &request); } static int hook_path_truncate(const struct path *const path) { - return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE); + struct landlock_request request = { + .operation = LANDLOCK_OP_TRUNCATE, + }; + + return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE, + &request); } /* File hooks */ @@ -1199,6 +1235,13 @@ static int hook_file_open(struct file *const file) const access_mask_t optional_access = LANDLOCK_ACCESS_FS_TRUNCATE; const struct landlock_ruleset *const dom = landlock_get_current_domain(); + struct landlock_request request = { + .operation = LANDLOCK_OP_OPEN, + .audit = { + .type = LSM_AUDIT_DATA_PATH, + .u.path = file->f_path, + }, + }; if (!dom) return 0; @@ -1249,7 +1292,8 @@ static int hook_file_open(struct file *const file) if ((open_access_request & allowed_access) == open_access_request) return 0; - return -EACCES; + return landlock_log_request(-EACCES, &request, dom, open_access_request, + &layer_masks); } static int hook_file_truncate(struct file *const file)
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(-)