Before this change, pathname lookup for umount permission checks was performed inside profile_umount(). This mixed object-specific path information with profile-specific path configuration.
PATH_IS_DIR describes the object being unmounted, while path_flags and disconnected belong to the current profile. Keeping these inputs together inside profile_umount() unnecessarily coupled pathname resolution with the permission check. An existing TODO in profile_umount() already called for lifting this lookup. Separate pathname resolution from the mount permission check. Calculate PATH_IS_DIR once in aa_umount(), since it is specific to the object. Then, for each profile, combine it with profile->path_flags when calling aa_path_name(), using profile->disconnected from the current profile. Move this per-profile pathname lookup into umount_path_perm() and pass the resolved pathname to profile_umount(). Keep profile_umount() focused on checking the resolved pathname against the mount policy: DFA matching, permission lookup, profile mode handling, and the final AA_MAY_UMOUNT check. Signed-off-by: Murilo Duarte M de Almeida <[email protected]> --- security/apparmor/mount.c | 42 +++++++++++++++++++++++++-------------- 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/security/apparmor/mount.c b/security/apparmor/mount.c index 4ed7b9136beb..c7dfdae95747 100644 --- a/security/apparmor/mount.c +++ b/security/apparmor/mount.c @@ -541,13 +541,32 @@ int aa_new_mount(const struct cred *subj_cred, struct aa_label *label, return error; } -static int profile_umount(struct aa_profile *profile, const struct path *path, - char *buffer, struct apparmor_audit_data *ad) +static int profile_umount(struct aa_profile *profile, const char *name, + struct apparmor_audit_data *ad) { + + AA_BUG(!profile); + AA_BUG(!name); + struct aa_ruleset *rules = profile->label.rules[0]; struct aa_perms perms = { }; - const char *name = NULL; aa_state_t state; + + state = aa_dfa_match(rules->policy->dfa, + rules->policy->start[AA_CLASS_MOUNT], + name); + perms = *aa_lookup_perms(rules->policy, state); + + aa_apply_modes_to_perms(profile, &perms); + return aa_check_perms(profile, &perms, AA_MAY_UMOUNT, ad, audit_cb); +} + +static int umount_path_perm(struct aa_profile *profile, const struct path *path, + int flags, char *buffer, + struct apparmor_audit_data *ad) +{ + struct aa_ruleset *rules = profile->label.rules[0]; + const char *name = NULL; int error; AA_BUG(!profile); @@ -556,23 +575,14 @@ static int profile_umount(struct aa_profile *profile, const struct path *path, if (!RULE_MEDIATES(rules, AA_CLASS_MOUNT)) return 0; - /* TODO: lift path_name, need to separate profile path_flags from - * the lookup - */ - error = aa_path_name(path, path_flags(profile, path), buffer, &name, + error = aa_path_name(path, flags | profile->path_flags, buffer, &name, &ad->info, profile->disconnected); if (error) return aa_audit_perm_error(&profile->label, AA_MAY_UMOUNT, error, ad, audit_cb); ad->name = name; - state = aa_dfa_match(rules->policy->dfa, - rules->policy->start[AA_CLASS_MOUNT], - name); - perms = *aa_lookup_perms(rules->policy, state); - - aa_apply_modes_to_perms(profile, &perms); - return aa_check_perms(profile, &perms, AA_MAY_UMOUNT, ad, audit_cb); + return profile_umount(profile, name, ad); } int aa_umount(const struct cred *subj_cred, struct aa_label *label, @@ -587,12 +597,14 @@ int aa_umount(const struct cred *subj_cred, struct aa_label *label, AA_BUG(!label); AA_BUG(!mnt); + flags = S_ISDIR(path.dentry->d_inode->i_mode) ? PATH_IS_DIR : 0; + buffer = aa_get_buffer(false); if (!buffer) return -ENOMEM; error = fn_for_each(label, profile, - profile_umount(profile, &path, buffer, &ad)); + umount_path_perm(profile, &path, flags, buffer, &ad)); aa_put_buffer(buffer); return error; -- 2.43.0
