[GIT] More security subsystem fixes

[GIT] More security subsystem fixes

[James Morris]

Linus,

Please pull these fixes for 3.3.


The following changes since commit f59e842fc0871cd5baa213dc32e0ce8e5aaf4758:
  Linus Torvalds (1):
        Merge branch 'for-next-merge' of git://git.kernel.org/.../nab/target-pending

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git for-linus

David Howells (2):
      MPILIB: Add a missing ENOMEM check
      KEYS: Permit key_serial() to be called with a const key pointer

Mimi Zohar (2):
      ima: fix cred sparse warning
      keys: fix user_defined key sparse messages

 include/linux/key.h                 |    2 +-
 lib/mpi/mpicoder.c                  |    2 ++
 security/integrity/ima/ima_policy.c |    3 ++-
 security/keys/user_defined.c        |    6 +++---
 4 files changed, 8 insertions(+), 5 deletions(-)

---

commit 456a8167e94b66f406c27400a46a707b870452b0
Author: David Howells <dhowells@redhat.com>
Date:   Wed Jan 18 10:04:29 2012 +0000

    KEYS: Permit key_serial() to be called with a const key pointer
    
    Permit key_serial() to be called with a const key pointer.
    
    Signed-off-by: David Howells <dhowells@redhat.com>
    Signed-off-by: James Morris <jmorris@namei.org>

diff --git a/include/linux/key.h b/include/linux/key.h
index bfc014c..5253471 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -271,7 +271,7 @@ extern int keyring_add_key(struct key *keyring,
 
 extern struct key *key_lookup(key_serial_t id);
 
-static inline key_serial_t key_serial(struct key *key)
+static inline key_serial_t key_serial(const struct key *key)
 {
 	return key ? key->serial : 0;
 }

commit f6b24579d099ebb67f39cd7924a72a7eec0ce6ae
Author: Mimi Zohar <zohar@linux.vnet.ibm.com>
Date:   Wed Jan 18 10:03:14 2012 +0000

    keys: fix user_defined key sparse messages
    
    Replace the rcu_assign_pointer() calls with rcu_assign_keypointer().
    
    Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
    Signed-off-by: David Howells <dhowells@redhat.com>
    Signed-off-by: James Morris <jmorris@namei.org>

diff --git a/security/keys/user_defined.c b/security/keys/user_defined.c
index 69ff52c..2aee3c5 100644
--- a/security/keys/user_defined.c
+++ b/security/keys/user_defined.c
@@ -59,7 +59,7 @@ int user_instantiate(struct key *key, const void *data, size_t datalen)
 	/* attach the data */
 	upayload->datalen = datalen;
 	memcpy(upayload->data, data, datalen);
-	rcu_assign_pointer(key->payload.data, upayload);
+	rcu_assign_keypointer(key, upayload);
 	ret = 0;
 
 error:
@@ -98,7 +98,7 @@ int user_update(struct key *key, const void *data, size_t datalen)
 	if (ret == 0) {
 		/* attach the new data, displacing the old */
 		zap = key->payload.data;
-		rcu_assign_pointer(key->payload.data, upayload);
+		rcu_assign_keypointer(key, upayload);
 		key->expiry = 0;
 	}
 
@@ -133,7 +133,7 @@ void user_revoke(struct key *key)
 	key_payload_reserve(key, 0);
 
 	if (upayload) {
-		rcu_assign_pointer(key->payload.data, NULL);
+		rcu_assign_keypointer(key, NULL);
 		kfree_rcu(upayload, rcu);
 	}
 }

commit 3db59dd93309710c40aaf1571c607cb0feef3ecb
Author: Mimi Zohar <zohar@linux.vnet.ibm.com>
Date:   Tue Jan 17 22:11:28 2012 -0500

    ima: fix cred sparse warning
    
    Fix ima_policy.c sparse "warning: dereference of noderef expression"
    message, by accessing cred->uid using current_cred().
    
    Changelog v1:
    - Change __cred to just cred (based on David Howell's comment)
    
    Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
    Signed-off-by: James Morris <jmorris@namei.org>

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index d661afb..d45061d 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -99,6 +99,7 @@ static bool ima_match_rules(struct ima_measure_rule_entry *rule,
 			    struct inode *inode, enum ima_hooks func, int mask)
 {
 	struct task_struct *tsk = current;
+	const struct cred *cred = current_cred();
 	int i;
 
 	if ((rule->flags & IMA_FUNC) && rule->func != func)
@@ -108,7 +109,7 @@ static bool ima_match_rules(struct ima_measure_rule_entry *rule,
 	if ((rule->flags & IMA_FSMAGIC)
 	    && rule->fsmagic != inode->i_sb->s_magic)
 		return false;
-	if ((rule->flags & IMA_UID) && rule->uid != tsk->cred->uid)
+	if ((rule->flags & IMA_UID) && rule->uid != cred->uid)
 		return false;
 	for (i = 0; i < MAX_LSM_RULES; i++) {
 		int rc = 0;

commit 4bf1924c008dffdc154f82507b4052e49263a6f4
Author: David Howells <dhowells@redhat.com>
Date:   Wed Jan 18 10:03:54 2012 +0000

    MPILIB: Add a missing ENOMEM check
    
    Add a missing ENOMEM check.
    
    Signed-off-by: David Howells <dhowells@redhat.com>
    Acked-by: Mimi Zohar <zohar@us.ibm.com>
    Signed-off-by: James Morris <jmorris@namei.org>

diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c
index fe84bb9..716802b 100644
--- a/lib/mpi/mpicoder.c
+++ b/lib/mpi/mpicoder.c
@@ -255,6 +255,8 @@ void *mpi_get_buffer(MPI a, unsigned *nbytes, int *sign)
 	if (!n)
 		n++;		/* avoid zero length allocation */
 	p = buffer = kmalloc(n, GFP_KERNEL);
+	if (!p)
+		return NULL;
 
 	for (i = a->nlimbs - 1; i >= 0; i--) {
 		alimb = a->d[i];

Re: [GIT] More security subsystem fixes

[from-linux-security-module]

James Morris wrote:
>       MPILIB: Add a missing ENOMEM check
Maybe one more (shown below) with some random comments.



        for (i = a->nlimbs - 1; i >= 0; i--) {
                alimb = a->d[i];
#if BYTES_PER_MPI_LIMB == 4
                *p++ = alimb >> 24;
                *p++ = alimb >> 16;
                *p++ = alimb >> 8;
                *p++ = alimb;
#elif BYTES_PER_MPI_LIMB == 8
                *p++ = alimb >> 56;
                *p++ = alimb >> 48;
                *p++ = alimb >> 40;
                *p++ = alimb >> 32;
                *p++ = alimb >> 24;
                *p++ = alimb >> 16;
                *p++ = alimb >> 8;
                *p++ = alimb;
#else
#error please implement for this limb size.
#endif

Are such large bitshift guaranteed to work?



MPI do_encode_md(const void *sha_buffer, unsigned nbits)
{
(...snipped...)
        a = mpi_alloc((nframe + BYTES_PER_MPI_LIMB - 1) / BYTES_PER_MPI_LIMB);
	// Missing a != NULL check here.
        mpi_set_buffer(a, frame, nframe, 0);
        kfree(frame);
(...snipped...)
}

Is (nframe + BYTES_PER_MPI_LIMB - 1) / BYTES_PER_MPI_LIMB * sizeof(mpi_limb_t)
guaranteed not to overflow? Is nlimbs guaranteed to be not 0?

MPI mpi_alloc(unsigned nlimbs)
{
	mpi_alloc_limb_space(nlimbs)
	{
		mpi_ptr_t mpi_alloc_limb_space(unsigned nlimbs)
		{
		        size_t len = nlimbs * sizeof(mpi_limb_t);
		
		        return kmalloc(len, GFP_KERNEL);
		}
	}
}

There are several kmalloc(nlimbs * sizeof(mpi_limb_t)) calls.
Are they guaranteed not to overflow?

int mpi_resize(MPI a, unsigned nlimbs)
{
        void *p;

        if (nlimbs <= a->alloced)
                return 0;       /* no need to do it */

        if (a->d) {
		// Can use krealloc()?
                p = kmalloc(nlimbs * sizeof(mpi_limb_t), GFP_KERNEL);
                if (!p)
                        return -ENOMEM;
                memcpy(p, a->d, a->alloced * sizeof(mpi_limb_t));
                kfree(a->d);
                a->d = p;
        } else {
                a->d = kzalloc(nlimbs * sizeof(mpi_limb_t), GFP_KERNEL);
                if (!a->d)
                        return -ENOMEM;
        }
        a->alloced = nlimbs;
        return 0;
}

Roughly browsing, it seems to me that this module assumes that kmalloc()
returns NULL if requested size was 0. It is OK for userspace, but I think we
should introduce and use a version of kmalloc() that returns NULL if requested
size was 0 (so that we can catch 0 byte allocation unless overflow).

Re: [GIT] More security subsystem fixes

[Tetsuo Handa]

Tetsuo Handa wrote:
> James Morris wrote:
> >       MPILIB: Add a missing ENOMEM check
> Maybe one more (shown below) with some random comments.

Looked a bit more.



In lib/mpi/mpih-div.c:

mpi_limb_t
mpihelp_divrem(mpi_ptr_t qp, mpi_size_t qextra_limbs,
               mpi_ptr_t np, mpi_size_t nsize, mpi_ptr_t dp, mpi_size_t dsize)
{
        mpi_limb_t most_significant_q_limb = 0;

        switch (dsize) {
        case 0:
                /* We are asked to divide by zero, so go ahead and do it!  (To make
                   the compiler not remove this statement, return the value.)  */
                return 1 / dsize;

What's this? Division by 0 in the kernel is no good.



In lib/mpi/mpicoder.c:

MPI do_encode_md(const void *sha_buffer, unsigned nbits)
{
(...snipped...)
        n = 0;
        frame[n++] = 0;
        frame[n++] = 1;         /* block type */
        i = nframe - SHA1_DIGEST_LENGTH - asnlen - 3;

        if (i <= 1) {
                pr_info("MPI: message digest encoding failed\n");
                kfree(frame);
                return a;
        }

        memset(frame + n, 0xff, i);
        n += i;
        frame[n++] = 0;
        memcpy(frame + n, &asn, asnlen);
        n += asnlen;
        memcpy(frame + n, sha_buffer, SHA1_DIGEST_LENGTH);
        n += SHA1_DIGEST_LENGTH;

        i = nframe;
        fr_pt = frame;

        if (n != nframe) {

What's this? i = nframe - SHA1_DIGEST_LENGTH - asnlen - 3; equals
nframe = i + SHA1_DIGEST_LENGTH + asnlen + 3; and this should be always true.
Also, i = nframe; seems to make no sense because i is no longer used.

                printk
                    ("MPI: message digest encoding failed, frame length is wrong\n");
                kfree(frame);
                return a;
        }

        a = mpi_alloc((nframe + BYTES_PER_MPI_LIMB - 1) / BYTES_PER_MPI_LIMB);

Missing a != NULL check.

        mpi_set_buffer(a, frame, nframe, 0);
        kfree(frame);

        return a;
}



In lib/mpi/mpi-pow.c:

int mpi_powm(MPI res, MPI base, MPI exp, MPI mod)
{
(...snipped...)
        if (!msize)
                msize = 1 / msize;      /* provoke a signal */
(...snipped...)
}

Division by 0.



In lib/mpi/mpi-div.c:

int mpi_tdiv_q_2exp(MPI w, MPI u, unsigned count)
{
(...snipped...)
        usize = u->nlimbs;
        limb_cnt = count / BITS_PER_MPI_LIMB;
        wsize = usize - limb_cnt;
        if (limb_cnt >= usize)
                w->nlimbs = 0;
        else {
                mpi_ptr_t wp;
                mpi_ptr_t up;

                if (RESIZE_IF_NEEDED(w, wsize) < 0)
                        return -ENOMEM;
                wp = w->d;
                up = u->d;

                count %= BITS_PER_MPI_LIMB;
                if (count) {
                        mpihelp_rshift(wp, up + limb_cnt, wsize, count);
                        wsize -= !wp[wsize - 1];
(...snipped...)
}

Is wsize > 0 guaranteed?



Fixes (if any) will be needed to RHEL6's 2.6.32-220.2.1.el6 kernel as well.

Re: [GIT] More security subsystem fixes

[James Morris]

On Thu, 19 Jan 2012, Tetsuo Handa wrote:

> mpi_limb_t
> mpihelp_divrem(mpi_ptr_t qp, mpi_size_t qextra_limbs,
>                mpi_ptr_t np, mpi_size_t nsize, mpi_ptr_t dp, mpi_size_t dsize)
> {
>         mpi_limb_t most_significant_q_limb = 0;
> 
>         switch (dsize) {
>         case 0:
>                 /* We are asked to divide by zero, so go ahead and do it!  (To make
>                    the compiler not remove this statement, return the value.)  */
>                 return 1 / dsize;
> 
> What's this? Division by 0 in the kernel is no good.

David,

Do you know if this is triggerable from userland?


- James
-- 
James Morris
<jmorris@namei.org>

Re: [GIT] More security subsystem fixes

[Kasatkin, Dmitry]

On Thu, Jan 19, 2012 at 4:21 PM, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> Tetsuo Handa wrote:
>> James Morris wrote:
>> >       MPILIB: Add a missing ENOMEM check
>> Maybe one more (shown below) with some random comments.
>
> Looked a bit more.
>
>
>
> In lib/mpi/mpih-div.c:
>
> mpi_limb_t
> mpihelp_divrem(mpi_ptr_t qp, mpi_size_t qextra_limbs,
>               mpi_ptr_t np, mpi_size_t nsize, mpi_ptr_t dp, mpi_size_t dsize)
> {
>        mpi_limb_t most_significant_q_limb = 0;
>
>        switch (dsize) {
>        case 0:
>                /* We are asked to divide by zero, so go ahead and do it!  (To make
>                   the compiler not remove this statement, return the value.)  */
>                return 1 / dsize;
>
> What's this? Division by 0 in the kernel is no good.
>
>
>
> In lib/mpi/mpicoder.c:
>
> MPI do_encode_md(const void *sha_buffer, unsigned nbits)
> {
> (...snipped...)
>        n = 0;
>        frame[n++] = 0;
>        frame[n++] = 1;         /* block type */
>        i = nframe - SHA1_DIGEST_LENGTH - asnlen - 3;
>
>        if (i <= 1) {
>                pr_info("MPI: message digest encoding failed\n");
>                kfree(frame);
>                return a;
>        }
>
>        memset(frame + n, 0xff, i);
>        n += i;
>        frame[n++] = 0;
>        memcpy(frame + n, &asn, asnlen);
>        n += asnlen;
>        memcpy(frame + n, sha_buffer, SHA1_DIGEST_LENGTH);
>        n += SHA1_DIGEST_LENGTH;
>
>        i = nframe;
>        fr_pt = frame;
>
>        if (n != nframe) {
>
> What's this? i = nframe - SHA1_DIGEST_LENGTH - asnlen - 3; equals
> nframe = i + SHA1_DIGEST_LENGTH + asnlen + 3; and this should be always true.

In fact n always equals to nframe, and this check is always false...


> Also, i = nframe; seems to make no sense because i is no longer used.
>
>                printk
>                    ("MPI: message digest encoding failed, frame length is wrong\n");
>                kfree(frame);
>                return a;
>        }
>
>        a = mpi_alloc((nframe + BYTES_PER_MPI_LIMB - 1) / BYTES_PER_MPI_LIMB);
>
> Missing a != NULL check.
>
>        mpi_set_buffer(a, frame, nframe, 0);
>        kfree(frame);
>
>        return a;
> }
>
>
>
> In lib/mpi/mpi-pow.c:
>
> int mpi_powm(MPI res, MPI base, MPI exp, MPI mod)
> {
> (...snipped...)
>        if (!msize)
>                msize = 1 / msize;      /* provoke a signal */
> (...snipped...)
> }
>
> Division by 0.
>
>
>
> In lib/mpi/mpi-div.c:
>
> int mpi_tdiv_q_2exp(MPI w, MPI u, unsigned count)
> {
> (...snipped...)
>        usize = u->nlimbs;
>        limb_cnt = count / BITS_PER_MPI_LIMB;
>        wsize = usize - limb_cnt;
>        if (limb_cnt >= usize)
>                w->nlimbs = 0;
>        else {
>                mpi_ptr_t wp;
>                mpi_ptr_t up;
>
>                if (RESIZE_IF_NEEDED(w, wsize) < 0)
>                        return -ENOMEM;
>                wp = w->d;
>                up = u->d;
>
>                count %= BITS_PER_MPI_LIMB;
>                if (count) {
>                        mpihelp_rshift(wp, up + limb_cnt, wsize, count);
>                        wsize -= !wp[wsize - 1];
> (...snipped...)
> }
>
> Is wsize > 0 guaranteed?
>
Should be, because of the check:
        if (limb_cnt >= usize)
                w->nlimbs = 0;
       else...


>
>
> Fixes (if any) will be needed to RHEL6's 2.6.32-220.2.1.el6 kernel as well.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html