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 <john.johansen@canonical.com>

diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index c92fd0e7b33c6be8824e57e8a9d93b952a15a20c..ab8f23cdccff9881bcc2da5a51f687f440fc1cd0 100644 (file)
--- 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 83d43ac72134a70be2b178e89f8ea38fea4031a4..22be62f0fc7336b435f1ccb7595930442367442f 100644 (file)
--- 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 78e4909dcc6a3152e6e57714b4683bc17fb0a123..05fb3305671e7123ee464d2a7cfd516deb3e1728 100644 (file)
--- 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 9490f8e896305d7343216ebca8db9ce756fc3059..9d5de1d05be438c8d1a1fbdece439107758a29c6 100644 (file)
--- 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;