From 4227c333f65cddc6c2f048e5b67cfe796b9df9a6 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Tue, 23 May 2017 03:25:14 -0700 Subject: [PATCH] apparmor: Move path lookup to using preallocated buffers Dynamically allocating buffers is problematic and is an extra layer that is a potntial point of failure and can slow down mediation. Change path lookup to use the preallocated per cpu buffers. Signed-off-by: John Johansen --- security/apparmor/domain.c | 8 ++- security/apparmor/file.c | 13 ++-- security/apparmor/include/path.h | 4 +- security/apparmor/path.c | 114 +++++++++++-------------------- 4 files changed, 53 insertions(+), 86 deletions(-) diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c index c92fd0e7b33c..ab8f23cdccff 100644 --- a/security/apparmor/domain.c +++ b/security/apparmor/domain.c @@ -357,6 +357,9 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm) AA_BUG(!ctx); profile = aa_get_newest_profile(ctx->profile); + + /* buffer freed below, name is pointer into buffer */ + get_buffers(buffer); /* * get the namespace from the replacement profile as replacement * can change the namespace @@ -364,8 +367,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm) ns = profile->ns; state = profile->file.start; - /* buffer freed below, name is pointer into buffer */ - error = aa_path_name(&bprm->file->f_path, profile->path_flags, &buffer, + error = aa_path_name(&bprm->file->f_path, profile->path_flags, buffer, &name, &info, profile->disconnected); if (error) { if (unconfined(profile) || @@ -515,7 +517,7 @@ audit: cleanup: aa_put_profile(new_profile); aa_put_profile(profile); - kfree(buffer); + put_buffers(buffer); return error; } diff --git a/security/apparmor/file.c b/security/apparmor/file.c index 83d43ac72134..22be62f0fc73 100644 --- a/security/apparmor/file.c +++ b/security/apparmor/file.c @@ -285,7 +285,8 @@ int aa_path_perm(const char *op, struct aa_profile *profile, int error; flags |= profile->path_flags | (S_ISDIR(cond->mode) ? PATH_IS_DIR : 0); - error = aa_path_name(path, flags, &buffer, &name, &info, + get_buffers(buffer); + error = aa_path_name(path, flags, buffer, &name, &info, profile->disconnected); if (error) { if (error == -ENOENT && is_deleted(path->dentry)) { @@ -304,7 +305,7 @@ int aa_path_perm(const char *op, struct aa_profile *profile, } error = aa_audit_file(profile, &perms, op, request, name, NULL, cond->uid, info, error); - kfree(buffer); + put_buffers(buffer); return error; } @@ -363,16 +364,17 @@ int aa_path_link(struct aa_profile *profile, struct dentry *old_dentry, unsigned int state; int error; + get_buffers(buffer, buffer2); lperms = nullperms; /* buffer freed below, lname is pointer in buffer */ - error = aa_path_name(&link, profile->path_flags, &buffer, &lname, + error = aa_path_name(&link, profile->path_flags, buffer, &lname, &info, profile->disconnected); if (error) goto audit; /* buffer2 freed below, tname is pointer in buffer2 */ - error = aa_path_name(&target, profile->path_flags, &buffer2, &tname, + error = aa_path_name(&target, profile->path_flags, buffer2, &tname, &info, profile->disconnected); if (error) goto audit; @@ -432,8 +434,7 @@ done_tests: audit: error = aa_audit_file(profile, &lperms, OP_LINK, request, lname, tname, cond.uid, info, error); - kfree(buffer); - kfree(buffer2); + put_buffers(buffer, buffer2); return error; } diff --git a/security/apparmor/include/path.h b/security/apparmor/include/path.h index 78e4909dcc6a..05fb3305671e 100644 --- a/security/apparmor/include/path.h +++ b/security/apparmor/include/path.h @@ -23,10 +23,10 @@ enum path_flags { PATH_CHROOT_NSCONNECT = 0x10, /* connect paths that are at ns root */ PATH_DELEGATE_DELETED = 0x08000, /* delegate deleted files */ - PATH_MEDIATE_DELETED = 0x10000, /* mediate deleted paths */ + PATH_MEDIATE_DELETED = 0x10000, /* mediate deleted paths */ }; -int aa_path_name(const struct path *path, int flags, char **buffer, +int aa_path_name(const struct path *path, int flags, char *buffer, const char **name, const char **info, const char *disconnected); diff --git a/security/apparmor/path.c b/security/apparmor/path.c index 9490f8e89630..9d5de1d05be4 100644 --- a/security/apparmor/path.c +++ b/security/apparmor/path.c @@ -79,7 +79,6 @@ static int disconnect(const struct path *path, char *buf, char **name, * d_namespace_path - lookup a name associated with a given path * @path: path to lookup (NOT NULL) * @buf: buffer to store path to (NOT NULL) - * @buflen: length of @buf * @name: Returns - pointer for start of path name with in @buf (NOT NULL) * @flags: flags controlling path lookup * @disconnected: string to prefix to disconnected paths @@ -90,12 +89,14 @@ static int disconnect(const struct path *path, char *buf, char **name, * When no error the path name is returned in @name which points to * to a position in @buf */ -static int d_namespace_path(const struct path *path, char *buf, int buflen, - char **name, int flags, const char *disconnected) +static int d_namespace_path(const struct path *path, char *buf, char **name, + int flags, const char *disconnected) { char *res; int error = 0; int connected = 1; + int isdir = (flags & PATH_IS_DIR) ? 1 : 0; + int buflen = aa_g_path_max - isdir; if (path->mnt->mnt_flags & MNT_INTERNAL) { /* it's not mounted anywhere */ @@ -110,10 +111,12 @@ static int d_namespace_path(const struct path *path, char *buf, int buflen, /* TODO: convert over to using a per namespace * control instead of hard coded /proc */ - return prepend(name, *name - buf, "/proc", 5); + error = prepend(name, *name - buf, "/proc", 5); + goto out; } else - return disconnect(path, buf, name, flags, - disconnected); + error = disconnect(path, buf, name, flags, + disconnected); + goto out; } /* resolve paths relative to chroot?*/ @@ -132,8 +135,11 @@ static int d_namespace_path(const struct path *path, char *buf, int buflen, * be returned. */ if (!res || IS_ERR(res)) { - if (PTR_ERR(res) == -ENAMETOOLONG) - return -ENAMETOOLONG; + if (PTR_ERR(res) == -ENAMETOOLONG) { + error = -ENAMETOOLONG; + *name = buf; + goto out; + } connected = 0; res = dentry_path_raw(path->dentry, buf, buflen); if (IS_ERR(res)) { @@ -146,6 +152,9 @@ static int d_namespace_path(const struct path *path, char *buf, int buflen, *name = res; + if (!connected) + error = disconnect(path, buf, name, flags, disconnected); + /* Handle two cases: * 1. A deleted dentry && profile is not allowing mediation of deleted * 2. On some filesystems, newly allocated dentries appear to the @@ -153,62 +162,27 @@ static int d_namespace_path(const struct path *path, char *buf, int buflen, * allocated. */ if (d_unlinked(path->dentry) && d_is_positive(path->dentry) && - !(flags & PATH_MEDIATE_DELETED)) { + !(flags & (PATH_MEDIATE_DELETED | PATH_DELEGATE_DELETED))) { error = -ENOENT; goto out; } - if (!connected) - error = disconnect(path, buf, name, flags, disconnected); - out: - return error; -} - -/** - * get_name_to_buffer - get the pathname to a buffer ensure dir / is appended - * @path: path to get name for (NOT NULL) - * @flags: flags controlling path lookup - * @buffer: buffer to put name in (NOT NULL) - * @size: size of buffer - * @name: Returns - contains position of path name in @buffer (NOT NULL) - * - * Returns: %0 else error on failure - */ -static int get_name_to_buffer(const struct path *path, int flags, char *buffer, - int size, char **name, const char **info, - const char *disconnected) -{ - int adjust = (flags & PATH_IS_DIR) ? 1 : 0; - int error = d_namespace_path(path, buffer, size - adjust, name, flags, - disconnected); - - if (!error && (flags & PATH_IS_DIR) && (*name)[1] != '\0') - /* - * Append "/" to the pathname. The root directory is a special - * case; it already ends in slash. - */ - strcpy(&buffer[size - 2], "/"); - - if (info && error) { - if (error == -ENOENT) - *info = "Failed name lookup - deleted entry"; - else if (error == -EACCES) - *info = "Failed name lookup - disconnected path"; - else if (error == -ENAMETOOLONG) - *info = "Failed name lookup - name too long"; - else - *info = "Failed name lookup"; - } + /* + * Append "/" to the pathname. The root directory is a special + * case; it already ends in slash. + */ + if (!error && isdir && ((*name)[1] != '\0' || (*name)[0] != '/')) + strcpy(&buf[aa_g_path_max - 2], "/"); return error; } /** - * aa_path_name - compute the pathname of a file + * aa_path_name - get the pathname to a buffer ensure dir / is appended * @path: path the file (NOT NULL) * @flags: flags controlling path name generation - * @buffer: buffer that aa_get_name() allocated (NOT NULL) + * @buffer: buffer to put name in (NOT NULL) * @name: Returns - the generated path name if !error (NOT NULL) * @info: Returns - information on why the path lookup failed (MAYBE NULL) * @disconnected: string to prepend to disconnected paths @@ -224,33 +198,23 @@ static int get_name_to_buffer(const struct path *path, int flags, char *buffer, * * Returns: %0 else error code if could retrieve name */ -int aa_path_name(const struct path *path, int flags, char **buffer, +int aa_path_name(const struct path *path, int flags, char *buffer, const char **name, const char **info, const char *disconnected) { - char *buf, *str = NULL; - int size = 256; - int error; - - *name = NULL; - *buffer = NULL; - for (;;) { - /* freed by caller */ - buf = kmalloc(size, GFP_KERNEL); - if (!buf) - return -ENOMEM; + char *str = NULL; + int error = d_namespace_path(path, buffer, &str, flags, disconnected); - error = get_name_to_buffer(path, flags, buf, size, &str, info, - disconnected); - if (error != -ENAMETOOLONG) - break; - - kfree(buf); - size <<= 1; - if (size > aa_g_path_max) - return -ENAMETOOLONG; - *info = NULL; + if (info && error) { + if (error == -ENOENT) + *info = "Failed name lookup - deleted entry"; + else if (error == -EACCES) + *info = "Failed name lookup - disconnected path"; + else if (error == -ENAMETOOLONG) + *info = "Failed name lookup - name too long"; + else + *info = "Failed name lookup"; } - *buffer = buf; + *name = str; return error; -- 2.20.1