Linux-Integrity Archive on lore.kernel.org
 help / color / Atom feed
* [bug report] evm: Check also if *tfm is an error pointer in init_desc()
@ 2020-05-12 10:48 Dan Carpenter
  2020-05-12 11:31 ` Roberto Sassu
  0 siblings, 1 reply; 12+ messages in thread
From: Dan Carpenter @ 2020-05-12 10:48 UTC (permalink / raw)
  To: roberto.sassu; +Cc: linux-integrity, linux-security-module

Hello Roberto Sassu,

The patch 53de3b080d5e: "evm: Check also if *tfm is an error pointer
in init_desc()" from Apr 27, 2020, leads to the following static
checker warning:

	security/integrity/evm/evm_crypto.c:119 init_desc()
	error: '*tfm' dereferencing possible ERR_PTR()

security/integrity/evm/evm_crypto.c
    89  
    90                  tfm = &evm_tfm[hash_algo];
    91                  algo = hash_algo_name[hash_algo];
    92          }
    93  
    94          if (IS_ERR_OR_NULL(*tfm)) {

This used to be a "if (!*tfm)" check.

    95                  mutex_lock(&mutex);
    96                  if (*tfm)
    97                          goto out;

Then we test again with the lock held.  But in the new code if "*tfm"
is an error pointer then we jump directly to the unlock and crash on the
next line.  I can't see how the commit would fix anything.

    98                  *tfm = crypto_alloc_shash(algo, 0, CRYPTO_NOLOAD);
    99                  if (IS_ERR(*tfm)) {
   100                          rc = PTR_ERR(*tfm);
   101                          pr_err("Can not allocate %s (reason: %ld)\n", algo, rc);
   102                          *tfm = NULL;
   103                          mutex_unlock(&mutex);
   104                          return ERR_PTR(rc);
   105                  }
   106                  if (type == EVM_XATTR_HMAC) {
   107                          rc = crypto_shash_setkey(*tfm, evmkey, evmkey_len);
   108                          if (rc) {
   109                                  crypto_free_shash(*tfm);
   110                                  *tfm = NULL;
   111                                  mutex_unlock(&mutex);
   112                                  return ERR_PTR(rc);
   113                          }
   114                  }
   115  out:
   116                  mutex_unlock(&mutex);
   117          }
   118  
   119          desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(*tfm),
                                                                     ^^^^
I don't understand how using *tfm outside of a lock is safe at all
anyway.

   120                          GFP_KERNEL);
   121          if (!desc)
   122                  return ERR_PTR(-ENOMEM);
   123  

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [bug report] evm: Check also if *tfm is an error pointer in init_desc()
  2020-05-12 10:48 [bug report] evm: Check also if *tfm is an error pointer in init_desc() Dan Carpenter
@ 2020-05-12 11:31 ` Roberto Sassu
  2020-05-12 12:34   ` Dan Carpenter
  0 siblings, 1 reply; 12+ messages in thread
From: Roberto Sassu @ 2020-05-12 11:31 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: linux-integrity, linux-security-module, Silviu Vlasceanu

> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Tuesday, May 12, 2020 12:48 PM
> 
> Hello Roberto Sassu,
> 
> The patch 53de3b080d5e: "evm: Check also if *tfm is an error pointer
> in init_desc()" from Apr 27, 2020, leads to the following static
> checker warning:
> 
> 	security/integrity/evm/evm_crypto.c:119 init_desc()
> 	error: '*tfm' dereferencing possible ERR_PTR()
> 
> security/integrity/evm/evm_crypto.c
>     89
>     90                  tfm = &evm_tfm[hash_algo];
>     91                  algo = hash_algo_name[hash_algo];
>     92          }
>     93
>     94          if (IS_ERR_OR_NULL(*tfm)) {
> 
> This used to be a "if (!*tfm)" check.
> 
>     95                  mutex_lock(&mutex);
>     96                  if (*tfm)
>     97                          goto out;
> 
> Then we test again with the lock held.  But in the new code if "*tfm"
> is an error pointer then we jump directly to the unlock and crash on the
> next line.  I can't see how the commit would fix anything.

Hello Dan

you are right. The fix should be applied in both places.

if (!IS_ERR_OR_NULL(*tfm))
	goto out;

>     98                  *tfm = crypto_alloc_shash(algo, 0, CRYPTO_NOLOAD);
>     99                  if (IS_ERR(*tfm)) {
>    100                          rc = PTR_ERR(*tfm);
>    101                          pr_err("Can not allocate %s (reason: %ld)\n", algo, rc);
>    102                          *tfm = NULL;
>    103                          mutex_unlock(&mutex);
>    104                          return ERR_PTR(rc);
>    105                  }
>    106                  if (type == EVM_XATTR_HMAC) {
>    107                          rc = crypto_shash_setkey(*tfm, evmkey, evmkey_len);
>    108                          if (rc) {
>    109                                  crypto_free_shash(*tfm);
>    110                                  *tfm = NULL;
>    111                                  mutex_unlock(&mutex);
>    112                                  return ERR_PTR(rc);
>    113                          }
>    114                  }
>    115  out:
>    116                  mutex_unlock(&mutex);
>    117          }
>    118
>    119          desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(*tfm),
>                                                                      ^^^^
> I don't understand how using *tfm outside of a lock is safe at all
> anyway.

I think the purpose of the mutex is just to  prevent two concurrent
allocations. Later, it should not be a problem, as *tfm is never freed.

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli


>    120                          GFP_KERNEL);
>    121          if (!desc)
>    122                  return ERR_PTR(-ENOMEM);
>    123
> 
> regards,
> dan carpenter

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [bug report] evm: Check also if *tfm is an error pointer in init_desc()
  2020-05-12 11:31 ` Roberto Sassu
@ 2020-05-12 12:34   ` Dan Carpenter
  2020-05-12 12:45     ` Roberto Sassu
  0 siblings, 1 reply; 12+ messages in thread
From: Dan Carpenter @ 2020-05-12 12:34 UTC (permalink / raw)
  To: Roberto Sassu; +Cc: linux-integrity, linux-security-module, Silviu Vlasceanu

On Tue, May 12, 2020 at 11:31:53AM +0000, Roberto Sassu wrote:
> > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > Sent: Tuesday, May 12, 2020 12:48 PM
> > 
> > Hello Roberto Sassu,
> > 
> > The patch 53de3b080d5e: "evm: Check also if *tfm is an error pointer
> > in init_desc()" from Apr 27, 2020, leads to the following static
> > checker warning:
> > 
> > 	security/integrity/evm/evm_crypto.c:119 init_desc()
> > 	error: '*tfm' dereferencing possible ERR_PTR()
> > 
> > security/integrity/evm/evm_crypto.c
> >     89
> >     90                  tfm = &evm_tfm[hash_algo];
> >     91                  algo = hash_algo_name[hash_algo];
> >     92          }
> >     93
> >     94          if (IS_ERR_OR_NULL(*tfm)) {
> > 
> > This used to be a "if (!*tfm)" check.
> > 
> >     95                  mutex_lock(&mutex);
> >     96                  if (*tfm)
> >     97                          goto out;
> > 
> > Then we test again with the lock held.  But in the new code if "*tfm"
> > is an error pointer then we jump directly to the unlock and crash on the
> > next line.  I can't see how the commit would fix anything.
> 
> Hello Dan
> 
> you are right. The fix should be applied in both places.
> 
> if (!IS_ERR_OR_NULL(*tfm))
> 	goto out;

No.  I was wrong.

> 
> >     98                  *tfm = crypto_alloc_shash(algo, 0, CRYPTO_NOLOAD);
> >     99                  if (IS_ERR(*tfm)) {
> >    100                          rc = PTR_ERR(*tfm);
> >    101                          pr_err("Can not allocate %s (reason: %ld)\n", algo, rc);
> >    102                          *tfm = NULL;
> >    103                          mutex_unlock(&mutex);
> >    104                          return ERR_PTR(rc);
> >    105                  }
> >    106                  if (type == EVM_XATTR_HMAC) {
> >    107                          rc = crypto_shash_setkey(*tfm, evmkey, evmkey_len);
> >    108                          if (rc) {
> >    109                                  crypto_free_shash(*tfm);
> >    110                                  *tfm = NULL;
> >    111                                  mutex_unlock(&mutex);
> >    112                                  return ERR_PTR(rc);
> >    113                          }
> >    114                  }
> >    115  out:
> >    116                  mutex_unlock(&mutex);
> >    117          }
> >    118
> >    119          desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(*tfm),
> >                                                                      ^^^^
> > I don't understand how using *tfm outside of a lock is safe at all
> > anyway.
> 
> I think the purpose of the mutex is just to  prevent two concurrent
> allocations. Later, it should not be a problem, as *tfm is never freed.
> 

Actually by the time we take the lock then *tfm is either valid or NULL
so this code works.  It's confusing though.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [bug report] evm: Check also if *tfm is an error pointer in init_desc()
  2020-05-12 12:34   ` Dan Carpenter
@ 2020-05-12 12:45     ` Roberto Sassu
  2020-05-12 13:04       ` Dan Carpenter
  0 siblings, 1 reply; 12+ messages in thread
From: Roberto Sassu @ 2020-05-12 12:45 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: linux-integrity, linux-security-module, Silviu Vlasceanu

> From: owner-linux-security-module@vger.kernel.org [mailto:owner-linux-
> security-module@vger.kernel.org] On Behalf Of Dan Carpenter
> Sent: Tuesday, May 12, 2020 2:34 PM
> On Tue, May 12, 2020 at 11:31:53AM +0000, Roberto Sassu wrote:
> > > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > > Sent: Tuesday, May 12, 2020 12:48 PM
> > >
> > > Hello Roberto Sassu,
> > >
> > > The patch 53de3b080d5e: "evm: Check also if *tfm is an error pointer
> > > in init_desc()" from Apr 27, 2020, leads to the following static
> > > checker warning:
> > >
> > > 	security/integrity/evm/evm_crypto.c:119 init_desc()
> > > 	error: '*tfm' dereferencing possible ERR_PTR()
> > >
> > > security/integrity/evm/evm_crypto.c
> > >     89
> > >     90                  tfm = &evm_tfm[hash_algo];
> > >     91                  algo = hash_algo_name[hash_algo];
> > >     92          }
> > >     93
> > >     94          if (IS_ERR_OR_NULL(*tfm)) {
> > >
> > > This used to be a "if (!*tfm)" check.
> > >
> > >     95                  mutex_lock(&mutex);
> > >     96                  if (*tfm)
> > >     97                          goto out;
> > >
> > > Then we test again with the lock held.  But in the new code if "*tfm"
> > > is an error pointer then we jump directly to the unlock and crash on the
> > > next line.  I can't see how the commit would fix anything.
> >
> > Hello Dan
> >
> > you are right. The fix should be applied in both places.
> >
> > if (!IS_ERR_OR_NULL(*tfm))
> > 	goto out;
> 
> No.  I was wrong.
> 
> >
> > >     98                  *tfm = crypto_alloc_shash(algo, 0, CRYPTO_NOLOAD);
> > >     99                  if (IS_ERR(*tfm)) {
> > >    100                          rc = PTR_ERR(*tfm);
> > >    101                          pr_err("Can not allocate %s (reason: %ld)\n", algo, rc);
> > >    102                          *tfm = NULL;
> > >    103                          mutex_unlock(&mutex);
> > >    104                          return ERR_PTR(rc);
> > >    105                  }
> > >    106                  if (type == EVM_XATTR_HMAC) {
> > >    107                          rc = crypto_shash_setkey(*tfm, evmkey,
> evmkey_len);
> > >    108                          if (rc) {
> > >    109                                  crypto_free_shash(*tfm);
> > >    110                                  *tfm = NULL;
> > >    111                                  mutex_unlock(&mutex);
> > >    112                                  return ERR_PTR(rc);
> > >    113                          }
> > >    114                  }
> > >    115  out:
> > >    116                  mutex_unlock(&mutex);
> > >    117          }
> > >    118
> > >    119          desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(*tfm),
> > >                                                                      ^^^^
> > > I don't understand how using *tfm outside of a lock is safe at all
> > > anyway.
> >
> > I think the purpose of the mutex is just to  prevent two concurrent
> > allocations. Later, it should not be a problem, as *tfm is never freed.
> >
> 
> Actually by the time we take the lock then *tfm is either valid or NULL
> so this code works.  It's confusing though.

static inline bool __must_check IS_ERR_OR_NULL(__force const void *ptr)
{
        return unlikely(!ptr) || IS_ERR_VALUE((unsigned long)ptr);
}

CPU#1			CPU#2
			*tfm = crypto_alloc_shash(algo, 0, CRYPTO_NOLOAD);
unlikely(!ptr)
			*tfm = NULL;
IS_ERR_VALUE((unsigned long)ptr);

desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(*tfm),

Could this happen?

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [bug report] evm: Check also if *tfm is an error pointer in init_desc()
  2020-05-12 12:45     ` Roberto Sassu
@ 2020-05-12 13:04       ` Dan Carpenter
  2020-05-12 13:08         ` Roberto Sassu
  2020-05-12 13:19         ` [PATCH] evm: Fix a small race " Dan Carpenter
  0 siblings, 2 replies; 12+ messages in thread
From: Dan Carpenter @ 2020-05-12 13:04 UTC (permalink / raw)
  To: Roberto Sassu; +Cc: linux-integrity, linux-security-module, Silviu Vlasceanu

On Tue, May 12, 2020 at 12:45:06PM +0000, Roberto Sassu wrote:
> > From: owner-linux-security-module@vger.kernel.org [mailto:owner-linux-
> > security-module@vger.kernel.org] On Behalf Of Dan Carpenter
> > Sent: Tuesday, May 12, 2020 2:34 PM
> > On Tue, May 12, 2020 at 11:31:53AM +0000, Roberto Sassu wrote:
> > > > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > > > Sent: Tuesday, May 12, 2020 12:48 PM
> > > >
> > > > Hello Roberto Sassu,
> > > >
> > > > The patch 53de3b080d5e: "evm: Check also if *tfm is an error pointer
> > > > in init_desc()" from Apr 27, 2020, leads to the following static
> > > > checker warning:
> > > >
> > > > 	security/integrity/evm/evm_crypto.c:119 init_desc()
> > > > 	error: '*tfm' dereferencing possible ERR_PTR()
> > > >
> > > > security/integrity/evm/evm_crypto.c
> > > >     89
> > > >     90                  tfm = &evm_tfm[hash_algo];
> > > >     91                  algo = hash_algo_name[hash_algo];
> > > >     92          }
> > > >     93
> > > >     94          if (IS_ERR_OR_NULL(*tfm)) {
> > > >
> > > > This used to be a "if (!*tfm)" check.
> > > >
> > > >     95                  mutex_lock(&mutex);
> > > >     96                  if (*tfm)
> > > >     97                          goto out;
> > > >
> > > > Then we test again with the lock held.  But in the new code if "*tfm"
> > > > is an error pointer then we jump directly to the unlock and crash on the
> > > > next line.  I can't see how the commit would fix anything.
> > >
> > > Hello Dan
> > >
> > > you are right. The fix should be applied in both places.
> > >
> > > if (!IS_ERR_OR_NULL(*tfm))
> > > 	goto out;
> > 
> > No.  I was wrong.
> > 
> > >
> > > >     98                  *tfm = crypto_alloc_shash(algo, 0, CRYPTO_NOLOAD);
> > > >     99                  if (IS_ERR(*tfm)) {
> > > >    100                          rc = PTR_ERR(*tfm);
> > > >    101                          pr_err("Can not allocate %s (reason: %ld)\n", algo, rc);
> > > >    102                          *tfm = NULL;
> > > >    103                          mutex_unlock(&mutex);
> > > >    104                          return ERR_PTR(rc);
> > > >    105                  }
> > > >    106                  if (type == EVM_XATTR_HMAC) {
> > > >    107                          rc = crypto_shash_setkey(*tfm, evmkey,
> > evmkey_len);
> > > >    108                          if (rc) {
> > > >    109                                  crypto_free_shash(*tfm);
> > > >    110                                  *tfm = NULL;
> > > >    111                                  mutex_unlock(&mutex);
> > > >    112                                  return ERR_PTR(rc);
> > > >    113                          }
> > > >    114                  }
> > > >    115  out:
> > > >    116                  mutex_unlock(&mutex);
> > > >    117          }
> > > >    118
> > > >    119          desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(*tfm),
> > > >                                                                      ^^^^
> > > > I don't understand how using *tfm outside of a lock is safe at all
> > > > anyway.
> > >
> > > I think the purpose of the mutex is just to  prevent two concurrent
> > > allocations. Later, it should not be a problem, as *tfm is never freed.
> > >
> > 
> > Actually by the time we take the lock then *tfm is either valid or NULL
> > so this code works.  It's confusing though.
> 
> static inline bool __must_check IS_ERR_OR_NULL(__force const void *ptr)
> {
>         return unlikely(!ptr) || IS_ERR_VALUE((unsigned long)ptr);
> }
> 
> CPU#1			CPU#2
> 			*tfm = crypto_alloc_shash(algo, 0, CRYPTO_NOLOAD);
> unlikely(!ptr)
> 			*tfm = NULL;
> IS_ERR_VALUE((unsigned long)ptr);
> 
> desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(*tfm),
> 
> Could this happen?

Yeah.  Huh.  That's true.  Good eyes.

I feel like this would be more clear as well if we used a temporary
variable instead of working directly on "*tfm".

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [bug report] evm: Check also if *tfm is an error pointer in init_desc()
  2020-05-12 13:04       ` Dan Carpenter
@ 2020-05-12 13:08         ` Roberto Sassu
  2020-05-12 13:19         ` [PATCH] evm: Fix a small race " Dan Carpenter
  1 sibling, 0 replies; 12+ messages in thread
From: Roberto Sassu @ 2020-05-12 13:08 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: linux-integrity, linux-security-module, Silviu Vlasceanu

> From: owner-linux-security-module@vger.kernel.org [mailto:owner-linux-
> security-module@vger.kernel.org] On Behalf Of Dan Carpenter
> Sent: Tuesday, May 12, 2020 3:04 PM
> On Tue, May 12, 2020 at 12:45:06PM +0000, Roberto Sassu wrote:
> > > From: owner-linux-security-module@vger.kernel.org [mailto:owner-
> linux-
> > > security-module@vger.kernel.org] On Behalf Of Dan Carpenter
> > > Sent: Tuesday, May 12, 2020 2:34 PM
> > > On Tue, May 12, 2020 at 11:31:53AM +0000, Roberto Sassu wrote:
> > > > > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > > > > Sent: Tuesday, May 12, 2020 12:48 PM
> > > > >
> > > > > Hello Roberto Sassu,
> > > > >
> > > > > The patch 53de3b080d5e: "evm: Check also if *tfm is an error pointer
> > > > > in init_desc()" from Apr 27, 2020, leads to the following static
> > > > > checker warning:
> > > > >
> > > > > 	security/integrity/evm/evm_crypto.c:119 init_desc()
> > > > > 	error: '*tfm' dereferencing possible ERR_PTR()
> > > > >
> > > > > security/integrity/evm/evm_crypto.c
> > > > >     89
> > > > >     90                  tfm = &evm_tfm[hash_algo];
> > > > >     91                  algo = hash_algo_name[hash_algo];
> > > > >     92          }
> > > > >     93
> > > > >     94          if (IS_ERR_OR_NULL(*tfm)) {
> > > > >
> > > > > This used to be a "if (!*tfm)" check.
> > > > >
> > > > >     95                  mutex_lock(&mutex);
> > > > >     96                  if (*tfm)
> > > > >     97                          goto out;
> > > > >
> > > > > Then we test again with the lock held.  But in the new code if "*tfm"
> > > > > is an error pointer then we jump directly to the unlock and crash on
> the
> > > > > next line.  I can't see how the commit would fix anything.
> > > >
> > > > Hello Dan
> > > >
> > > > you are right. The fix should be applied in both places.
> > > >
> > > > if (!IS_ERR_OR_NULL(*tfm))
> > > > 	goto out;
> > >
> > > No.  I was wrong.
> > >
> > > >
> > > > >     98                  *tfm = crypto_alloc_shash(algo, 0, CRYPTO_NOLOAD);
> > > > >     99                  if (IS_ERR(*tfm)) {
> > > > >    100                          rc = PTR_ERR(*tfm);
> > > > >    101                          pr_err("Can not allocate %s (reason: %ld)\n", algo,
> rc);
> > > > >    102                          *tfm = NULL;
> > > > >    103                          mutex_unlock(&mutex);
> > > > >    104                          return ERR_PTR(rc);
> > > > >    105                  }
> > > > >    106                  if (type == EVM_XATTR_HMAC) {
> > > > >    107                          rc = crypto_shash_setkey(*tfm, evmkey,
> > > evmkey_len);
> > > > >    108                          if (rc) {
> > > > >    109                                  crypto_free_shash(*tfm);
> > > > >    110                                  *tfm = NULL;
> > > > >    111                                  mutex_unlock(&mutex);
> > > > >    112                                  return ERR_PTR(rc);
> > > > >    113                          }
> > > > >    114                  }
> > > > >    115  out:
> > > > >    116                  mutex_unlock(&mutex);
> > > > >    117          }
> > > > >    118
> > > > >    119          desc = kmalloc(sizeof(*desc) +
> crypto_shash_descsize(*tfm),
> > > > >                                                                      ^^^^
> > > > > I don't understand how using *tfm outside of a lock is safe at all
> > > > > anyway.
> > > >
> > > > I think the purpose of the mutex is just to  prevent two concurrent
> > > > allocations. Later, it should not be a problem, as *tfm is never freed.
> > > >
> > >
> > > Actually by the time we take the lock then *tfm is either valid or NULL
> > > so this code works.  It's confusing though.
> >
> > static inline bool __must_check IS_ERR_OR_NULL(__force const void *ptr)
> > {
> >         return unlikely(!ptr) || IS_ERR_VALUE((unsigned long)ptr);
> > }
> >
> > CPU#1			CPU#2
> > 			*tfm = crypto_alloc_shash(algo, 0,
> CRYPTO_NOLOAD);
> > unlikely(!ptr)
> > 			*tfm = NULL;
> > IS_ERR_VALUE((unsigned long)ptr);
> >
> > desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(*tfm),
> >
> > Could this happen?
> 
> Yeah.  Huh.  That's true.  Good eyes.
> 
> I feel like this would be more clear as well if we used a temporary
> variable instead of working directly on "*tfm".

Yes, seems better. I will send a new version of the patch.

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH] evm: Fix a small race in init_desc()
  2020-05-12 13:04       ` Dan Carpenter
  2020-05-12 13:08         ` Roberto Sassu
@ 2020-05-12 13:19         ` Dan Carpenter
  2020-05-12 13:43           ` Roberto Sassu
  1 sibling, 1 reply; 12+ messages in thread
From: Dan Carpenter @ 2020-05-12 13:19 UTC (permalink / raw)
  To: Mimi Zohar, Roberto Sassu
  Cc: James Morris, Serge E. Hallyn, linux-integrity,
	linux-security-module, kernel-janitors

The IS_ERR_OR_NULL() function has two conditions and if we got really
unlucky we could hit a race where "ptr" started as an error pointer and
then was set to NULL.  Both conditions would be false even though the
pointer at the end was NULL.

This patch fixes the problem by ensuring that "*tfm" can only be NULL
or valid.  I have introduced a "tmp_tfm" variable to make that work.  I
also reversed a condition and pulled the code in one tab.

Reported-by: Roberto Sassu <roberto.sassu@huawei.com>
Fixes: 53de3b080d5e: "evm: Check also if *tfm is an error pointer in init_desc()"
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I just had a go at the fix.  I'm not super wedded to this solution, but
I hoped it was nice.

 security/integrity/evm/evm_crypto.c | 44 ++++++++++++++---------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index 5d3789edab71f..168c3b78ac47b 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -73,7 +73,7 @@ static struct shash_desc *init_desc(char type, uint8_t hash_algo)
 {
 	long rc;
 	const char *algo;
-	struct crypto_shash **tfm;
+	struct crypto_shash **tfm, *tmp_tfm;
 	struct shash_desc *desc;
 
 	if (type == EVM_XATTR_HMAC) {
@@ -91,31 +91,31 @@ static struct shash_desc *init_desc(char type, uint8_t hash_algo)
 		algo = hash_algo_name[hash_algo];
 	}
 
-	if (IS_ERR_OR_NULL(*tfm)) {
-		mutex_lock(&mutex);
-		if (*tfm)
-			goto out;
-		*tfm = crypto_alloc_shash(algo, 0, CRYPTO_NOLOAD);
-		if (IS_ERR(*tfm)) {
-			rc = PTR_ERR(*tfm);
-			pr_err("Can not allocate %s (reason: %ld)\n", algo, rc);
-			*tfm = NULL;
+	if (*tfm)
+		goto alloc;
+	mutex_lock(&mutex);
+	if (*tfm)
+		goto unlock;
+
+	tmp_tfm = crypto_alloc_shash(algo, 0, CRYPTO_NOLOAD);
+	if (IS_ERR(tmp_tfm)) {
+		pr_err("Can not allocate %s (reason: %ld)\n", algo,
+		       PTR_ERR(tmp_tfm));
+		mutex_unlock(&mutex);
+		return ERR_CAST(tmp_tfm);
+	}
+	if (type == EVM_XATTR_HMAC) {
+		rc = crypto_shash_setkey(tmp_tfm, evmkey, evmkey_len);
+		if (rc) {
+			crypto_free_shash(tmp_tfm);
 			mutex_unlock(&mutex);
 			return ERR_PTR(rc);
 		}
-		if (type == EVM_XATTR_HMAC) {
-			rc = crypto_shash_setkey(*tfm, evmkey, evmkey_len);
-			if (rc) {
-				crypto_free_shash(*tfm);
-				*tfm = NULL;
-				mutex_unlock(&mutex);
-				return ERR_PTR(rc);
-			}
-		}
-out:
-		mutex_unlock(&mutex);
 	}
-
+	*tfm = tmp_tfm;
+unlock:
+	mutex_unlock(&mutex);
+alloc:
 	desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(*tfm),
 			GFP_KERNEL);
 	if (!desc)
-- 
2.26.2


^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH] evm: Fix a small race in init_desc()
  2020-05-12 13:19         ` [PATCH] evm: Fix a small race " Dan Carpenter
@ 2020-05-12 13:43           ` Roberto Sassu
  2020-05-12 17:47             ` [PATCH v2] " Dan Carpenter
  0 siblings, 1 reply; 12+ messages in thread
From: Roberto Sassu @ 2020-05-12 13:43 UTC (permalink / raw)
  To: Dan Carpenter, Mimi Zohar
  Cc: James Morris, Serge E. Hallyn, linux-integrity,
	linux-security-module, kernel-janitors, Silviu Vlasceanu

> From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity-
> owner@vger.kernel.org] On Behalf Of Dan Carpenter
> Sent: Tuesday, May 12, 2020 3:19 PM
> Subject: [PATCH] evm: Fix a small race in init_desc()
> 
> The IS_ERR_OR_NULL() function has two conditions and if we got really
> unlucky we could hit a race where "ptr" started as an error pointer and
> then was set to NULL.  Both conditions would be false even though the
> pointer at the end was NULL.
> 
> This patch fixes the problem by ensuring that "*tfm" can only be NULL
> or valid.  I have introduced a "tmp_tfm" variable to make that work.  I
> also reversed a condition and pulled the code in one tab.
> 
> Reported-by: Roberto Sassu <roberto.sassu@huawei.com>
> Fixes: 53de3b080d5e: "evm: Check also if *tfm is an error pointer in
> init_desc()"
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> I just had a go at the fix.  I'm not super wedded to this solution, but
> I hoped it was nice.

Thanks. I think you can merge both patches in one, as the first one
is not yet pulled.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli


>  security/integrity/evm/evm_crypto.c | 44 ++++++++++++++---------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/security/integrity/evm/evm_crypto.c
> b/security/integrity/evm/evm_crypto.c
> index 5d3789edab71f..168c3b78ac47b 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -73,7 +73,7 @@ static struct shash_desc *init_desc(char type, uint8_t
> hash_algo)
>  {
>  	long rc;
>  	const char *algo;
> -	struct crypto_shash **tfm;
> +	struct crypto_shash **tfm, *tmp_tfm;
>  	struct shash_desc *desc;
> 
>  	if (type == EVM_XATTR_HMAC) {
> @@ -91,31 +91,31 @@ static struct shash_desc *init_desc(char type, uint8_t
> hash_algo)
>  		algo = hash_algo_name[hash_algo];
>  	}
> 
> -	if (IS_ERR_OR_NULL(*tfm)) {
> -		mutex_lock(&mutex);
> -		if (*tfm)
> -			goto out;
> -		*tfm = crypto_alloc_shash(algo, 0, CRYPTO_NOLOAD);
> -		if (IS_ERR(*tfm)) {
> -			rc = PTR_ERR(*tfm);
> -			pr_err("Can not allocate %s (reason: %ld)\n", algo,
> rc);
> -			*tfm = NULL;
> +	if (*tfm)
> +		goto alloc;
> +	mutex_lock(&mutex);
> +	if (*tfm)
> +		goto unlock;
> +
> +	tmp_tfm = crypto_alloc_shash(algo, 0, CRYPTO_NOLOAD);
> +	if (IS_ERR(tmp_tfm)) {
> +		pr_err("Can not allocate %s (reason: %ld)\n", algo,
> +		       PTR_ERR(tmp_tfm));
> +		mutex_unlock(&mutex);
> +		return ERR_CAST(tmp_tfm);
> +	}
> +	if (type == EVM_XATTR_HMAC) {
> +		rc = crypto_shash_setkey(tmp_tfm, evmkey, evmkey_len);
> +		if (rc) {
> +			crypto_free_shash(tmp_tfm);
>  			mutex_unlock(&mutex);
>  			return ERR_PTR(rc);
>  		}
> -		if (type == EVM_XATTR_HMAC) {
> -			rc = crypto_shash_setkey(*tfm, evmkey,
> evmkey_len);
> -			if (rc) {
> -				crypto_free_shash(*tfm);
> -				*tfm = NULL;
> -				mutex_unlock(&mutex);
> -				return ERR_PTR(rc);
> -			}
> -		}
> -out:
> -		mutex_unlock(&mutex);
>  	}
> -
> +	*tfm = tmp_tfm;
> +unlock:
> +	mutex_unlock(&mutex);
> +alloc:
>  	desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(*tfm),
>  			GFP_KERNEL);
>  	if (!desc)
> --
> 2.26.2


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2] evm: Fix a small race in init_desc()
  2020-05-12 13:43           ` Roberto Sassu
@ 2020-05-12 17:47             ` Dan Carpenter
  2020-05-14  6:47               ` Roberto Sassu
  0 siblings, 1 reply; 12+ messages in thread
From: Dan Carpenter @ 2020-05-12 17:47 UTC (permalink / raw)
  To: Mimi Zohar, Roberto Sassu, Krzysztof Struczynski
  Cc: James Morris, Serge E. Hallyn, Dmitry Kasatkin, linux-integrity,
	linux-security-module, kernel-janitors

This patch avoids a kernel panic due to accessing an error pointer set by
crypto_alloc_shash(). It occurs especially when there are many files that
require an unsupported algorithm, as it would increase the likelihood of
the following race condition.

Imagine we have two threads and in the first thread crypto_alloc_shash()
fails and returns an error pointer.

		*tfm = crypto_alloc_shash(algo, 0, CRYPTO_NOLOAD);
		if (IS_ERR(*tfm)) {
			rc = PTR_ERR(*tfm); <--- FIRST THREAD HERE!
			pr_err("Can not allocate %s (reason: %ld)\n", algo, rc);
			*tfm = NULL;

And the second thread is here:

	if (*tfm == NULL) {  <--- SECOND THREAD HERE!
		mutex_lock(&mutex);
		if (*tfm)
			goto out;

Since "*tfm" is non-NULL, we assume that it is valid and that leads to
a crash when it dereferences "*tfm".

	desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(*tfm),
                                                             ^^^^

This patch fixes the problem by introducing a temporary "tmp_tfm" and
only setting "*tfm" at the very end after everything has succeeded.  The
other change is that I reversed the initial "if (!*tfm) {" condition and
pull the code in one indent level.

Cc: stable@vger.kernel.org
Fixes: d46eb3699502b ("evm: crypto hash replaced by shash")
Reported-by: Roberto Sassu <roberto.sassu@huawei.com>
Reported-by: Krzysztof Struczynski <krzysztof.struczynski@huawei.com>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: I folded mine patch together with Roberto's

 security/integrity/evm/evm_crypto.c | 44 ++++++++++++++---------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index 35682852ddea9..c9f7206591b30 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -73,7 +73,7 @@ static struct shash_desc *init_desc(char type, uint8_t hash_algo)
 {
 	long rc;
 	const char *algo;
-	struct crypto_shash **tfm;
+	struct crypto_shash **tfm, *tmp_tfm;
 	struct shash_desc *desc;
 
 	if (type == EVM_XATTR_HMAC) {
@@ -91,31 +91,31 @@ static struct shash_desc *init_desc(char type, uint8_t hash_algo)
 		algo = hash_algo_name[hash_algo];
 	}
 
-	if (*tfm == NULL) {
-		mutex_lock(&mutex);
-		if (*tfm)
-			goto out;
-		*tfm = crypto_alloc_shash(algo, 0, CRYPTO_NOLOAD);
-		if (IS_ERR(*tfm)) {
-			rc = PTR_ERR(*tfm);
-			pr_err("Can not allocate %s (reason: %ld)\n", algo, rc);
-			*tfm = NULL;
+	if (*tfm)
+		goto alloc;
+	mutex_lock(&mutex);
+	if (*tfm)
+		goto unlock;
+
+	tmp_tfm = crypto_alloc_shash(algo, 0, CRYPTO_NOLOAD);
+	if (IS_ERR(tmp_tfm)) {
+		pr_err("Can not allocate %s (reason: %ld)\n", algo,
+		       PTR_ERR(tmp_tfm));
+		mutex_unlock(&mutex);
+		return ERR_CAST(tmp_tfm);
+	}
+	if (type == EVM_XATTR_HMAC) {
+		rc = crypto_shash_setkey(tmp_tfm, evmkey, evmkey_len);
+		if (rc) {
+			crypto_free_shash(tmp_tfm);
 			mutex_unlock(&mutex);
 			return ERR_PTR(rc);
 		}
-		if (type == EVM_XATTR_HMAC) {
-			rc = crypto_shash_setkey(*tfm, evmkey, evmkey_len);
-			if (rc) {
-				crypto_free_shash(*tfm);
-				*tfm = NULL;
-				mutex_unlock(&mutex);
-				return ERR_PTR(rc);
-			}
-		}
-out:
-		mutex_unlock(&mutex);
 	}
-
+	*tfm = tmp_tfm;
+unlock:
+	mutex_unlock(&mutex);
+alloc:
 	desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(*tfm),
 			GFP_KERNEL);
 	if (!desc)
-- 
2.26.2


^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH v2] evm: Fix a small race in init_desc()
  2020-05-12 17:47             ` [PATCH v2] " Dan Carpenter
@ 2020-05-14  6:47               ` Roberto Sassu
  2020-05-14  7:11                 ` Krzysztof Struczynski
  0 siblings, 1 reply; 12+ messages in thread
From: Roberto Sassu @ 2020-05-14  6:47 UTC (permalink / raw)
  To: Dan Carpenter, Mimi Zohar, Krzysztof Struczynski
  Cc: James Morris, Serge E. Hallyn, Dmitry Kasatkin, linux-integrity,
	linux-security-module, kernel-janitors

> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Tuesday, May 12, 2020 7:47 PM
> This patch avoids a kernel panic due to accessing an error pointer set by
> crypto_alloc_shash(). It occurs especially when there are many files that
> require an unsupported algorithm, as it would increase the likelihood of
> the following race condition.
> 
> Imagine we have two threads and in the first thread crypto_alloc_shash()
> fails and returns an error pointer.
> 
> 		*tfm = crypto_alloc_shash(algo, 0, CRYPTO_NOLOAD);
> 		if (IS_ERR(*tfm)) {
> 			rc = PTR_ERR(*tfm); <--- FIRST THREAD HERE!
> 			pr_err("Can not allocate %s (reason: %ld)\n", algo,
> rc);
> 			*tfm = NULL;
> 
> And the second thread is here:
> 
> 	if (*tfm == NULL) {  <--- SECOND THREAD HERE!
> 		mutex_lock(&mutex);
> 		if (*tfm)
> 			goto out;
> 
> Since "*tfm" is non-NULL, we assume that it is valid and that leads to
> a crash when it dereferences "*tfm".
> 
> 	desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(*tfm),
>                                                              ^^^^
> 
> This patch fixes the problem by introducing a temporary "tmp_tfm" and
> only setting "*tfm" at the very end after everything has succeeded.  The
> other change is that I reversed the initial "if (!*tfm) {" condition and
> pull the code in one indent level.
> 
> Cc: stable@vger.kernel.org
> Fixes: d46eb3699502b ("evm: crypto hash replaced by shash")
> Reported-by: Roberto Sassu <roberto.sassu@huawei.com>
> Reported-by: Krzysztof Struczynski <krzysztof.struczynski@huawei.com>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Acked-by: Roberto Sassu <roberto.sassu@huawei.com>

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli


> ---
> v2: I folded mine patch together with Roberto's
> 
>  security/integrity/evm/evm_crypto.c | 44 ++++++++++++++---------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/security/integrity/evm/evm_crypto.c
> b/security/integrity/evm/evm_crypto.c
> index 35682852ddea9..c9f7206591b30 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -73,7 +73,7 @@ static struct shash_desc *init_desc(char type, uint8_t
> hash_algo)
>  {
>  	long rc;
>  	const char *algo;
> -	struct crypto_shash **tfm;
> +	struct crypto_shash **tfm, *tmp_tfm;
>  	struct shash_desc *desc;
> 
>  	if (type == EVM_XATTR_HMAC) {
> @@ -91,31 +91,31 @@ static struct shash_desc *init_desc(char type, uint8_t
> hash_algo)
>  		algo = hash_algo_name[hash_algo];
>  	}
> 
> -	if (*tfm == NULL) {
> -		mutex_lock(&mutex);
> -		if (*tfm)
> -			goto out;
> -		*tfm = crypto_alloc_shash(algo, 0, CRYPTO_NOLOAD);
> -		if (IS_ERR(*tfm)) {
> -			rc = PTR_ERR(*tfm);
> -			pr_err("Can not allocate %s (reason: %ld)\n", algo,
> rc);
> -			*tfm = NULL;
> +	if (*tfm)
> +		goto alloc;
> +	mutex_lock(&mutex);
> +	if (*tfm)
> +		goto unlock;
> +
> +	tmp_tfm = crypto_alloc_shash(algo, 0, CRYPTO_NOLOAD);
> +	if (IS_ERR(tmp_tfm)) {
> +		pr_err("Can not allocate %s (reason: %ld)\n", algo,
> +		       PTR_ERR(tmp_tfm));
> +		mutex_unlock(&mutex);
> +		return ERR_CAST(tmp_tfm);
> +	}
> +	if (type == EVM_XATTR_HMAC) {
> +		rc = crypto_shash_setkey(tmp_tfm, evmkey, evmkey_len);
> +		if (rc) {
> +			crypto_free_shash(tmp_tfm);
>  			mutex_unlock(&mutex);
>  			return ERR_PTR(rc);
>  		}
> -		if (type == EVM_XATTR_HMAC) {
> -			rc = crypto_shash_setkey(*tfm, evmkey,
> evmkey_len);
> -			if (rc) {
> -				crypto_free_shash(*tfm);
> -				*tfm = NULL;
> -				mutex_unlock(&mutex);
> -				return ERR_PTR(rc);
> -			}
> -		}
> -out:
> -		mutex_unlock(&mutex);
>  	}
> -
> +	*tfm = tmp_tfm;
> +unlock:
> +	mutex_unlock(&mutex);
> +alloc:
>  	desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(*tfm),
>  			GFP_KERNEL);
>  	if (!desc)
> --
> 2.26.2


^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH v2] evm: Fix a small race in init_desc()
  2020-05-14  6:47               ` Roberto Sassu
@ 2020-05-14  7:11                 ` Krzysztof Struczynski
  2020-05-14 18:21                   ` Mimi Zohar
  0 siblings, 1 reply; 12+ messages in thread
From: Krzysztof Struczynski @ 2020-05-14  7:11 UTC (permalink / raw)
  To: Roberto Sassu, Dan Carpenter, Mimi Zohar
  Cc: James Morris, Serge E. Hallyn, Dmitry Kasatkin, linux-integrity,
	linux-security-module, kernel-janitors


> -----Original Message-----
> From: Roberto Sassu
> Sent: Thursday, May 14, 2020 8:47 AM
> To: Dan Carpenter <dan.carpenter@oracle.com>; Mimi Zohar
> <zohar@linux.ibm.com>; Krzysztof Struczynski
> <krzysztof.struczynski@huawei.com>
> Cc: James Morris <jmorris@namei.org>; Serge E. Hallyn <serge@hallyn.com>;
> Dmitry Kasatkin <dmitry.kasatkin@nokia.com>; linux-
> integrity@vger.kernel.org; linux-security-module@vger.kernel.org; kernel-
> janitors@vger.kernel.org
> Subject: RE: [PATCH v2] evm: Fix a small race in init_desc()
> 
> > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > Sent: Tuesday, May 12, 2020 7:47 PM
> > This patch avoids a kernel panic due to accessing an error pointer set
> > by crypto_alloc_shash(). It occurs especially when there are many
> > files that require an unsupported algorithm, as it would increase the
> > likelihood of the following race condition.
> >
> > Imagine we have two threads and in the first thread
> > crypto_alloc_shash() fails and returns an error pointer.
> >
> > 		*tfm = crypto_alloc_shash(algo, 0, CRYPTO_NOLOAD);
> > 		if (IS_ERR(*tfm)) {
> > 			rc = PTR_ERR(*tfm); <--- FIRST THREAD HERE!
> > 			pr_err("Can not allocate %s (reason: %ld)\n", algo, rc);
> > 			*tfm = NULL;
> >
> > And the second thread is here:
> >
> > 	if (*tfm == NULL) {  <--- SECOND THREAD HERE!
> > 		mutex_lock(&mutex);
> > 		if (*tfm)
> > 			goto out;
> >
> > Since "*tfm" is non-NULL, we assume that it is valid and that leads to
> > a crash when it dereferences "*tfm".
> >
> > 	desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(*tfm),
> >                                                              ^^^^
> >
> > This patch fixes the problem by introducing a temporary "tmp_tfm" and
> > only setting "*tfm" at the very end after everything has succeeded.
> > The other change is that I reversed the initial "if (!*tfm) {"
> > condition and pull the code in one indent level.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: d46eb3699502b ("evm: crypto hash replaced by shash")
> > Reported-by: Roberto Sassu <roberto.sassu@huawei.com>
> > Reported-by: Krzysztof Struczynski <krzysztof.struczynski@huawei.com>
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> Acked-by: Roberto Sassu <roberto.sassu@huawei.com>

Acked-by: Krzysztof Struczynski <krzysztof.struczynski@huawei.com>

Krzysztof
> 
> Roberto
> 
> HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li
> Peng, Li Jian, Shi Yanli
> 
> 
> > ---
> > v2: I folded mine patch together with Roberto's
> >
> >  security/integrity/evm/evm_crypto.c | 44
> > ++++++++++++++---------------
> >  1 file changed, 22 insertions(+), 22 deletions(-)
> >
> > diff --git a/security/integrity/evm/evm_crypto.c
> > b/security/integrity/evm/evm_crypto.c
> > index 35682852ddea9..c9f7206591b30 100644
> > --- a/security/integrity/evm/evm_crypto.c
> > +++ b/security/integrity/evm/evm_crypto.c
> > @@ -73,7 +73,7 @@ static struct shash_desc *init_desc(char type,
> > uint8_t
> > hash_algo)
> >  {
> >  	long rc;
> >  	const char *algo;
> > -	struct crypto_shash **tfm;
> > +	struct crypto_shash **tfm, *tmp_tfm;
> >  	struct shash_desc *desc;
> >
> >  	if (type == EVM_XATTR_HMAC) {
> > @@ -91,31 +91,31 @@ static struct shash_desc *init_desc(char type,
> > uint8_t
> > hash_algo)
> >  		algo = hash_algo_name[hash_algo];
> >  	}
> >
> > -	if (*tfm == NULL) {
> > -		mutex_lock(&mutex);
> > -		if (*tfm)
> > -			goto out;
> > -		*tfm = crypto_alloc_shash(algo, 0, CRYPTO_NOLOAD);
> > -		if (IS_ERR(*tfm)) {
> > -			rc = PTR_ERR(*tfm);
> > -			pr_err("Can not allocate %s (reason: %ld)\n", algo,
> > rc);
> > -			*tfm = NULL;
> > +	if (*tfm)
> > +		goto alloc;
> > +	mutex_lock(&mutex);
> > +	if (*tfm)
> > +		goto unlock;
> > +
> > +	tmp_tfm = crypto_alloc_shash(algo, 0, CRYPTO_NOLOAD);
> > +	if (IS_ERR(tmp_tfm)) {
> > +		pr_err("Can not allocate %s (reason: %ld)\n", algo,
> > +		       PTR_ERR(tmp_tfm));
> > +		mutex_unlock(&mutex);
> > +		return ERR_CAST(tmp_tfm);
> > +	}
> > +	if (type == EVM_XATTR_HMAC) {
> > +		rc = crypto_shash_setkey(tmp_tfm, evmkey, evmkey_len);
> > +		if (rc) {
> > +			crypto_free_shash(tmp_tfm);
> >  			mutex_unlock(&mutex);
> >  			return ERR_PTR(rc);
> >  		}
> > -		if (type == EVM_XATTR_HMAC) {
> > -			rc = crypto_shash_setkey(*tfm, evmkey,
> > evmkey_len);
> > -			if (rc) {
> > -				crypto_free_shash(*tfm);
> > -				*tfm = NULL;
> > -				mutex_unlock(&mutex);
> > -				return ERR_PTR(rc);
> > -			}
> > -		}
> > -out:
> > -		mutex_unlock(&mutex);
> >  	}
> > -
> > +	*tfm = tmp_tfm;
> > +unlock:
> > +	mutex_unlock(&mutex);
> > +alloc:
> >  	desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(*tfm),
> >  			GFP_KERNEL);
> >  	if (!desc)
> > --
> > 2.26.2


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] evm: Fix a small race in init_desc()
  2020-05-14  7:11                 ` Krzysztof Struczynski
@ 2020-05-14 18:21                   ` Mimi Zohar
  0 siblings, 0 replies; 12+ messages in thread
From: Mimi Zohar @ 2020-05-14 18:21 UTC (permalink / raw)
  To: Krzysztof Struczynski, Roberto Sassu, Dan Carpenter
  Cc: James Morris, Serge E. Hallyn, Dmitry Kasatkin, linux-integrity,
	linux-security-module, kernel-janitors

On Thu, 2020-05-14 at 07:11 +0000, Krzysztof Struczynski wrote:
> > > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > > This patch avoids a kernel panic due to accessing an error pointer set
> > > by crypto_alloc_shash(). It occurs especially when there are many
> > > files that require an unsupported algorithm, as it would increase the
> > > likelihood of the following race condition.
> > >
> > > Imagine we have two threads and in the first thread
> > > crypto_alloc_shash() fails and returns an error pointer.
> > >
> > > 		*tfm = crypto_alloc_shash(algo, 0, CRYPTO_NOLOAD);
> > > 		if (IS_ERR(*tfm)) {
> > > 			rc = PTR_ERR(*tfm); <--- FIRST THREAD HERE!
> > > 			pr_err("Can not allocate %s (reason: %ld)\n", algo, rc);
> > > 			*tfm = NULL;
> > >
> > > And the second thread is here:
> > >
> > > 	if (*tfm == NULL) {  <--- SECOND THREAD HERE!
> > > 		mutex_lock(&mutex);
> > > 		if (*tfm)
> > > 			goto out;
> > >
> > > Since "*tfm" is non-NULL, we assume that it is valid and that leads to
> > > a crash when it dereferences "*tfm".
> > >
> > > 	desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(*tfm),
> > >                                                              ^^^^
> > >
> > > This patch fixes the problem by introducing a temporary "tmp_tfm" and
> > > only setting "*tfm" at the very end after everything has succeeded.
> > > The other change is that I reversed the initial "if (!*tfm) {"
> > > condition and pull the code in one indent level.
> > >
> > > Cc: stable@vger.kernel.org
> > > Fixes: d46eb3699502b ("evm: crypto hash replaced by shash")
> > > Reported-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > Reported-by: Krzysztof Struczynski <krzysztof.struczynski@huawei.com>
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > Acked-by: Roberto Sassu <roberto.sassu@huawei.com>
> 
> Acked-by: Krzysztof Struczynski <krzysztof.struczynski@huawei.com>

Thanks, Roberto and Krzysztof.

This patch is now queued in the "fixes" branch.

Mimi

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, back to index

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-12 10:48 [bug report] evm: Check also if *tfm is an error pointer in init_desc() Dan Carpenter
2020-05-12 11:31 ` Roberto Sassu
2020-05-12 12:34   ` Dan Carpenter
2020-05-12 12:45     ` Roberto Sassu
2020-05-12 13:04       ` Dan Carpenter
2020-05-12 13:08         ` Roberto Sassu
2020-05-12 13:19         ` [PATCH] evm: Fix a small race " Dan Carpenter
2020-05-12 13:43           ` Roberto Sassu
2020-05-12 17:47             ` [PATCH v2] " Dan Carpenter
2020-05-14  6:47               ` Roberto Sassu
2020-05-14  7:11                 ` Krzysztof Struczynski
2020-05-14 18:21                   ` Mimi Zohar

Linux-Integrity Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-integrity/0 linux-integrity/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-integrity linux-integrity/ https://lore.kernel.org/linux-integrity \
		linux-integrity@vger.kernel.org
	public-inbox-index linux-integrity

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-integrity


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git