linux-kernel - RE: [PATCH RFC] cred: Add WARN to detect wrong use of get/put

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9a680489a44b44d397f8a3e77a6503e7@AcuMS.aculab.com>
Date:   Fri, 12 Jun 2020 16:16:56 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Xiaoming Ni' <nixiaoming@...wei.com>,
        "paul@...l-moore.com" <paul@...l-moore.com>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "paulmck@...nel.org" <paulmck@...nel.org>,
        "dhowells@...hat.com" <dhowells@...hat.com>,
        "keescook@...omium.org" <keescook@...omium.org>,
        "shakeelb@...gle.com" <shakeelb@...gle.com>,
        "jamorris@...ux.microsoft.com" <jamorris@...ux.microsoft.com>
CC:     "alex.huangjianhui@...wei.com" <alex.huangjianhui@...wei.com>,
        "dylix.dailei@...wei.com" <dylix.dailei@...wei.com>,
        "chenzefeng2@...wei.com" <chenzefeng2@...wei.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH RFC] cred: Add WARN to detect wrong use of get/put_cred

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@...wei.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)