[PATCH RFC] cred: Add WARN to detect wrong use of get/put_cred

[PATCH RFC] cred: Add WARN to detect wrong use of get/put_cred

[Xiaoming Ni]

Cred release and usage check code flow:
	1. put_cred()
		if (atomic_dec_and_test(&(cred)->usage))
			__put_cred(cred);

	2. __put_cred()
		BUG_ON(atomic_read(&cred->usage) != 0);
		call_rcu(&cred->rcu, put_cred_rcu);

	3. put_cred_rcu()
		if (atomic_read(&cred->usage) != 0)
			panic("CRED: put_cred_rcu() sees %p with usage %d\n",
			       cred, atomic_read(&cred->usage));
		kmem_cache_free(cred_jar, cred);

If panic is triggered on put_cred_rcu(), there are two possibilities
	1. Call get_cred() after __put_cred(), usage > 0
	2. Call put_cred() after __put_cred(), usage < 0
Since put_cred_rcu is an asynchronous behavior, it is no longer the first
scene when panic, there is no information about the murderer in the panic
call stack...

So, add WARN() in get_cred()/put_cred(), and pray to catch the murderer
at the first scene.

Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
---
 include/linux/cred.h | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/include/linux/cred.h b/include/linux/cred.h
index 18639c0..c00d5a1 100644
--- a/include/linux/cred.h
+++ b/include/linux/cred.h
@@ -224,11 +224,16 @@ static inline bool cap_ambient_invariant_ok(const struct cred *cred)
  *
  * Get a reference on the specified set of new credentials.  The caller must
  * release the reference.
+ *
+ * Initialize usage to 1 during cred resource allocation,
+ * so when calling get_cred, usage cannot be 0.
  */
 static inline struct cred *get_new_cred(struct cred *cred)
 {
-	atomic_inc(&cred->usage);
-	return cred;
+	if (atomic_inc_not_zero(&cred->usage))
+		return cred;
+	WARN(1, "get_new_cred after __put_cred");
+	return NULL;
 }
 
 /**
@@ -280,11 +285,14 @@ static inline const struct cred *get_cred_rcu(const struct cred *cred)
 static inline void put_cred(const struct cred *_cred)
 {
 	struct cred *cred = (struct cred *) _cred;
+	int usage;
 
 	if (cred) {
 		validate_creds(cred);
-		if (atomic_dec_and_test(&(cred)->usage))
+		usage = atomic_dec_return(&(cred)->usage);
+		if (usage == 0)
 			__put_cred(cred);
+		WARN(usage < 0, "put_cred after __put_cred");
 	}
 }
 
-- 
1.8.5.6

RE: [PATCH RFC] cred: Add WARN to detect wrong use of get/put_cred

[David Laight]

From: Xiaoming Ni
> Sent: 12 June 2020 11:28
> Cred release and usage check code flow:
> 	1. put_cred()
> 		if (atomic_dec_and_test(&(cred)->usage))
> 			__put_cred(cred);
> 
> 	2. __put_cred()
> 		BUG_ON(atomic_read(&cred->usage) != 0);
> 		call_rcu(&cred->rcu, put_cred_rcu);
> 
> 	3. put_cred_rcu()
> 		if (atomic_read(&cred->usage) != 0)
> 			panic("CRED: put_cred_rcu() sees %p with usage %d\n",
> 			       cred, atomic_read(&cred->usage));
> 		kmem_cache_free(cred_jar, cred);
> 
> If panic is triggered on put_cred_rcu(), there are two possibilities
> 	1. Call get_cred() after __put_cred(), usage > 0
> 	2. Call put_cred() after __put_cred(), usage < 0
> Since put_cred_rcu is an asynchronous behavior, it is no longer the first
> scene when panic, there is no information about the murderer in the panic
> call stack...
> 
> So, add WARN() in get_cred()/put_cred(), and pray to catch the murderer
> at the first scene.
> 
> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
> ---
>  include/linux/cred.h | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/cred.h b/include/linux/cred.h
> index 18639c0..c00d5a1 100644
> --- a/include/linux/cred.h
> +++ b/include/linux/cred.h
> @@ -224,11 +224,16 @@ static inline bool cap_ambient_invariant_ok(const struct cred *cred)
>   *
>   * Get a reference on the specified set of new credentials.  The caller must
>   * release the reference.
> + *
> + * Initialize usage to 1 during cred resource allocation,
> + * so when calling get_cred, usage cannot be 0.
>   */
>  static inline struct cred *get_new_cred(struct cred *cred)
>  {
> -	atomic_inc(&cred->usage);
> -	return cred;
> +	if (atomic_inc_not_zero(&cred->usage))
> +		return cred;
> +	WARN(1, "get_new_cred after __put_cred");
> +	return NULL;
>  }
> 
>  /**
> @@ -280,11 +285,14 @@ static inline const struct cred *get_cred_rcu(const struct cred *cred)
>  static inline void put_cred(const struct cred *_cred)
>  {
>  	struct cred *cred = (struct cred *) _cred;
> +	int usage;
> 
>  	if (cred) {
>  		validate_creds(cred);
> -		if (atomic_dec_and_test(&(cred)->usage))
> +		usage = atomic_dec_return(&(cred)->usage);
> +		if (usage == 0)
>  			__put_cred(cred);
> +		WARN(usage < 0, "put_cred after __put_cred");
>  	}
>  }

You really don't want to add WARN() to static inline functions.
It will bloat horribly.
It might be possible to the message into a called function.

One thing I've thought about for reference counts is for the
code that allocates and frees the item to add a big number
and code that only borrows a reference just adds 1.
If the counter is large enough you can separately detect
double frees and missing frees for the two different types
of allocation.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH RFC] cred: Add WARN to detect wrong use of get/put_cred

[Eric Dumazet]

On Fri, Jun 12, 2020 at 3:28 AM Xiaoming Ni <nixiaoming@huawei.com> wrote:
>
> Cred release and usage check code flow:
>         1. put_cred()
>                 if (atomic_dec_and_test(&(cred)->usage))
>                         __put_cred(cred);
>
>         2. __put_cred()
>                 BUG_ON(atomic_read(&cred->usage) != 0);
>                 call_rcu(&cred->rcu, put_cred_rcu);
>
>         3. put_cred_rcu()
>                 if (atomic_read(&cred->usage) != 0)
>                         panic("CRED: put_cred_rcu() sees %p with usage %d\n",
>                                cred, atomic_read(&cred->usage));
>                 kmem_cache_free(cred_jar, cred);
>
> If panic is triggered on put_cred_rcu(), there are two possibilities
>         1. Call get_cred() after __put_cred(), usage > 0
>         2. Call put_cred() after __put_cred(), usage < 0
> Since put_cred_rcu is an asynchronous behavior, it is no longer the first
> scene when panic, there is no information about the murderer in the panic
> call stack...
>
> So, add WARN() in get_cred()/put_cred(), and pray to catch the murderer
> at the first scene.
>
> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
> ---


It seems you reinvented refcount_t ?

Re: [PATCH RFC] cred: Add WARN to detect wrong use of get/put_cred

[Peter Zijlstra]

On Fri, Jun 12, 2020 at 06:28:15PM +0800, Xiaoming Ni wrote:
> Cred release and usage check code flow:
> 	1. put_cred()
> 		if (atomic_dec_and_test(&(cred)->usage))
> 			__put_cred(cred);
> 
> 	2. __put_cred()
> 		BUG_ON(atomic_read(&cred->usage) != 0);
> 		call_rcu(&cred->rcu, put_cred_rcu);
> 
> 	3. put_cred_rcu()
> 		if (atomic_read(&cred->usage) != 0)
> 			panic("CRED: put_cred_rcu() sees %p with usage %d\n",
> 			       cred, atomic_read(&cred->usage));
> 		kmem_cache_free(cred_jar, cred);
> 
> If panic is triggered on put_cred_rcu(), there are two possibilities
> 	1. Call get_cred() after __put_cred(), usage > 0
> 	2. Call put_cred() after __put_cred(), usage < 0
> Since put_cred_rcu is an asynchronous behavior, it is no longer the first
> scene when panic, there is no information about the murderer in the panic
> call stack...
> 
> So, add WARN() in get_cred()/put_cred(), and pray to catch the murderer
> at the first scene.

Why not not use refcount_t? It has all that goodness and more.

Re: [PATCH RFC] cred: Add WARN to detect wrong use of get/put_cred

[Kees Cook]

On Fri, Jun 12, 2020 at 06:33:45PM +0200, Peter Zijlstra wrote:
> On Fri, Jun 12, 2020 at 06:28:15PM +0800, Xiaoming Ni wrote:
> > Cred release and usage check code flow:
> > 	1. put_cred()
> > 		if (atomic_dec_and_test(&(cred)->usage))
> > 			__put_cred(cred);
> > 
> > 	2. __put_cred()
> > 		BUG_ON(atomic_read(&cred->usage) != 0);
> > 		call_rcu(&cred->rcu, put_cred_rcu);
> > 
> > 	3. put_cred_rcu()
> > 		if (atomic_read(&cred->usage) != 0)
> > 			panic("CRED: put_cred_rcu() sees %p with usage %d\n",
> > 			       cred, atomic_read(&cred->usage));
> > 		kmem_cache_free(cred_jar, cred);
> > 
> > If panic is triggered on put_cred_rcu(), there are two possibilities
> > 	1. Call get_cred() after __put_cred(), usage > 0
> > 	2. Call put_cred() after __put_cred(), usage < 0
> > Since put_cred_rcu is an asynchronous behavior, it is no longer the first
> > scene when panic, there is no information about the murderer in the panic
> > call stack...
> > 
> > So, add WARN() in get_cred()/put_cred(), and pray to catch the murderer
> > at the first scene.
> 
> Why not not use refcount_t? It has all that goodness and more.

I thought these had been applied already, I guess not:
https://lore.kernel.org/lkml/20190306110549.7628-1-elena.reshetova@intel.com/

Can we try again?

-- 
Kees Cook