cap: fix issues with waserror and memory leaks
authorBarret Rhoden <brho@cs.berkeley.edu>
Thu, 2 May 2019 01:19:11 +0000 (21:19 -0400)
committerBarret Rhoden <brho@cs.berkeley.edu>
Thu, 2 May 2019 01:19:11 +0000 (21:19 -0400)
The rule is that if you set a variable after a waserror(), that variable
needs to be volatile.  Otherwise the compiler may lose it.

It's not enough for the access inside the waserror() to be volatile,
since a write further down in the function may be stuck in a register.

Until we can get the compiler to realize a variable will be accessed in
both sides of the waserror() and flush it to the stack before calling
another function (which could throw), then we're out of luck.

To deal with it, the easiest thing in this case is to just do the
allocations first.

On a similar note, it looks like p was being leaked if there was an
error later in capwrite().  We just free it regardless (which may be a
bug), so we might as well free it right away.  It's already yanked out
of its data structure and just leaked.

Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>
kern/drivers/dev/capability.c

index 4e648fe..ac92de5 100644 (file)
@@ -200,9 +200,8 @@ static size_t capread(struct chan *c, void *va, size_t n, off64_t m)
 static size_t capwrite(struct chan *c, void *va, size_t n, off64_t m)
 {
        struct Caphash *p;
-       char *cp;
+       char *cp, *hashstr;
        uint8_t hash[Hashlen + 1] = {0};
-       char *hashstr = NULL;
        char *key, *from, *to;
        int ret;
        ERRSTACK(1);
@@ -222,13 +221,13 @@ static size_t capwrite(struct chan *c, void *va, size_t n, off64_t m)
        case Quse:
                /* copy key to avoid a fault in hmac_xx and so we can enforce
                 * null termination. */
-               cp = NULL;
+               cp = kzmalloc(n + 1, MEM_WAIT);
+               hashstr = kzmalloc(sizeof(hash), MEM_WAIT);
                if (waserror()) {
                        kfree(cp);
                        kfree(hashstr);
                        nexterror();
                }
-               cp = kzmalloc(n + 1, MEM_WAIT);
                memmove(cp, va, n);
                cp[n] = 0;
 
@@ -244,7 +243,6 @@ static size_t capwrite(struct chan *c, void *va, size_t n, off64_t m)
                        error(EINVAL, "HMAC failed");
 
                // Convert to ASCII text
-               hashstr = (char *)kzmalloc(sizeof(hash), MEM_WAIT);
                ret = __hashstr(hashstr, hash, VB2_SHA256_DIGEST_SIZE);
                if (ret != VB2_SHA256_DIGEST_SIZE)
                        error(EINVAL, "hash is wrong length");
@@ -252,9 +250,7 @@ static size_t capwrite(struct chan *c, void *va, size_t n, off64_t m)
                p = remcap((uint8_t *)hashstr);
                if (p == NULL)
                        error(EINVAL, "invalid capability %s@%s", from, key);
-
-               kfree(hashstr);
-               hashstr = NULL;
+               kfree(p);
 
                /* if a from user is supplied, make sure it matches */
                to = strchr(from, '@');
@@ -275,8 +271,8 @@ static size_t capwrite(struct chan *c, void *va, size_t n, off64_t m)
                proc_set_username(current, to);
                //up->basepri = PriNormal;
 
-               kfree(p);
                kfree(cp);
+               kfree(hashstr);
                poperror();
                break;