[PATCH] sparc64: convert mdesc_handle.refcnt from atomic_t to refcount_t

[PATCH] sparc64: convert mdesc_handle.refcnt from atomic_t to refcount_t

[Elena Reshetova]

atomic_t variables are currently used to implement reference
counters with the following properties:
 - counter is initialized to 1 using atomic_set()
 - a resource is freed upon counter reaching zero
 - once counter reaches zero, its further
   increments aren't allowed
 - counter schema uses basic atomic operations
   (set, inc, inc_not_zero, dec_and_test, etc.)

Such atomic variables should be converted to a newly provided
refcount_t type and API that prevents accidental counter overflows
and underflows. This is important since overflows and underflows
can lead to use-after-free situation and be exploitable.

The variable mdesc_handle.refcnt is used as pure reference counter.
Convert it to refcount_t and fix up the operations.

Suggested-by: Kees Cook <keescook@chromium.org>
Reviewed-by: David Windsor <dwindsor@gmail.com>
Reviewed-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
---
 arch/sparc/kernel/mdesc.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/arch/sparc/kernel/mdesc.c b/arch/sparc/kernel/mdesc.c
index fa466ce..821a724 100644
--- a/arch/sparc/kernel/mdesc.c
+++ b/arch/sparc/kernel/mdesc.c
@@ -12,6 +12,7 @@
 #include <linux/miscdevice.h>
 #include <linux/bootmem.h>
 #include <linux/export.h>
+#include <linux/refcount.h>
 
 #include <asm/cpudata.h>
 #include <asm/hypervisor.h>
@@ -70,7 +71,7 @@ struct mdesc_handle {
 	struct list_head	list;
 	struct mdesc_mem_ops	*mops;
 	void			*self_base;
-	atomic_t		refcnt;
+	refcount_t		refcnt;
 	unsigned int		handle_size;
 	struct mdesc_hdr	mdesc;
 };
@@ -152,7 +153,7 @@ static void mdesc_handle_init(struct mdesc_handle *hp,
 	memset(hp, 0, handle_size);
 	INIT_LIST_HEAD(&hp->list);
 	hp->self_base = base;
-	atomic_set(&hp->refcnt, 1);
+	refcount_set(&hp->refcnt, 1);
 	hp->handle_size = handle_size;
 }
 
@@ -182,7 +183,7 @@ static void __init mdesc_memblock_free(struct mdesc_handle *hp)
 	unsigned int alloc_size;
 	unsigned long start;
 
-	BUG_ON(atomic_read(&hp->refcnt) != 0);
+	BUG_ON(refcount_read(&hp->refcnt) != 0);
 	BUG_ON(!list_empty(&hp->list));
 
 	alloc_size = PAGE_ALIGN(hp->handle_size);
@@ -220,7 +221,7 @@ static struct mdesc_handle *mdesc_kmalloc(unsigned int mdesc_size)
 
 static void mdesc_kfree(struct mdesc_handle *hp)
 {
-	BUG_ON(atomic_read(&hp->refcnt) != 0);
+	BUG_ON(refcount_read(&hp->refcnt) != 0);
 	BUG_ON(!list_empty(&hp->list));
 
 	kfree(hp->self_base);
@@ -259,7 +260,7 @@ struct mdesc_handle *mdesc_grab(void)
 	spin_lock_irqsave(&mdesc_lock, flags);
 	hp = cur_mdesc;
 	if (hp)
-		atomic_inc(&hp->refcnt);
+		refcount_inc(&hp->refcnt);
 	spin_unlock_irqrestore(&mdesc_lock, flags);
 
 	return hp;
@@ -271,7 +272,7 @@ void mdesc_release(struct mdesc_handle *hp)
 	unsigned long flags;
 
 	spin_lock_irqsave(&mdesc_lock, flags);
-	if (atomic_dec_and_test(&hp->refcnt)) {
+	if (refcount_dec_and_test(&hp->refcnt)) {
 		list_del_init(&hp->list);
 		hp->mops->free(hp);
 	}
@@ -513,7 +514,7 @@ void mdesc_update(void)
 	if (status != HV_EOK || real_len > len) {
 		printk(KERN_ERR "MD: mdesc reread fails with %lu\n",
 		       status);
-		atomic_dec(&hp->refcnt);
+		refcount_dec(&hp->refcnt);
 		mdesc_free(hp);
 		goto out;
 	}
@@ -526,7 +527,7 @@ void mdesc_update(void)
 	mdesc_notify_clients(orig_hp, hp);
 
 	spin_lock_irqsave(&mdesc_lock, flags);
-	if (atomic_dec_and_test(&orig_hp->refcnt))
+	if (refcount_dec_and_test(&orig_hp->refcnt))
 		mdesc_free(orig_hp);
 	else
 		list_add(&orig_hp->list, &mdesc_zombie_list);
-- 
2.7.4

Re: [PATCH] sparc64: convert mdesc_handle.refcnt from atomic_t to refcount_t

[Shannon Nelson]

On 10/20/2017 12:57 AM, Elena Reshetova wrote:
> atomic_t variables are currently used to implement reference
> counters with the following properties:
>   - counter is initialized to 1 using atomic_set()
>   - a resource is freed upon counter reaching zero
>   - once counter reaches zero, its further
>     increments aren't allowed
>   - counter schema uses basic atomic operations
>     (set, inc, inc_not_zero, dec_and_test, etc.)
> 
> Such atomic variables should be converted to a newly provided
> refcount_t type and API that prevents accidental counter overflows
> and underflows. This is important since overflows and underflows
> can lead to use-after-free situation and be exploitable.
> 
> The variable mdesc_handle.refcnt is used as pure reference counter.
> Convert it to refcount_t and fix up the operations.
> 
> Suggested-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: David Windsor <dwindsor@gmail.com>
> Reviewed-by: Hans Liljestrand <ishkamiel@gmail.com>
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>

Acked-by: Shannon Nelson <shannon.nelson@oracle.com>

> ---
>   arch/sparc/kernel/mdesc.c | 17 +++++++++--------
>   1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/sparc/kernel/mdesc.c b/arch/sparc/kernel/mdesc.c
> index fa466ce..821a724 100644
> --- a/arch/sparc/kernel/mdesc.c
> +++ b/arch/sparc/kernel/mdesc.c
> @@ -12,6 +12,7 @@
>   #include <linux/miscdevice.h>
>   #include <linux/bootmem.h>
>   #include <linux/export.h>
> +#include <linux/refcount.h>
>   
>   #include <asm/cpudata.h>
>   #include <asm/hypervisor.h>
> @@ -70,7 +71,7 @@ struct mdesc_handle {
>   	struct list_head	list;
>   	struct mdesc_mem_ops	*mops;
>   	void			*self_base;
> -	atomic_t		refcnt;
> +	refcount_t		refcnt;
>   	unsigned int		handle_size;
>   	struct mdesc_hdr	mdesc;
>   };
> @@ -152,7 +153,7 @@ static void mdesc_handle_init(struct mdesc_handle *hp,
>   	memset(hp, 0, handle_size);
>   	INIT_LIST_HEAD(&hp->list);
>   	hp->self_base = base;
> -	atomic_set(&hp->refcnt, 1);
> +	refcount_set(&hp->refcnt, 1);
>   	hp->handle_size = handle_size;
>   }
>   
> @@ -182,7 +183,7 @@ static void __init mdesc_memblock_free(struct mdesc_handle *hp)
>   	unsigned int alloc_size;
>   	unsigned long start;
>   
> -	BUG_ON(atomic_read(&hp->refcnt) != 0);
> +	BUG_ON(refcount_read(&hp->refcnt) != 0);
>   	BUG_ON(!list_empty(&hp->list));
>   
>   	alloc_size = PAGE_ALIGN(hp->handle_size);
> @@ -220,7 +221,7 @@ static struct mdesc_handle *mdesc_kmalloc(unsigned int mdesc_size)
>   
>   static void mdesc_kfree(struct mdesc_handle *hp)
>   {
> -	BUG_ON(atomic_read(&hp->refcnt) != 0);
> +	BUG_ON(refcount_read(&hp->refcnt) != 0);
>   	BUG_ON(!list_empty(&hp->list));
>   
>   	kfree(hp->self_base);
> @@ -259,7 +260,7 @@ struct mdesc_handle *mdesc_grab(void)
>   	spin_lock_irqsave(&mdesc_lock, flags);
>   	hp = cur_mdesc;
>   	if (hp)
> -		atomic_inc(&hp->refcnt);
> +		refcount_inc(&hp->refcnt);
>   	spin_unlock_irqrestore(&mdesc_lock, flags);
>   
>   	return hp;
> @@ -271,7 +272,7 @@ void mdesc_release(struct mdesc_handle *hp)
>   	unsigned long flags;
>   
>   	spin_lock_irqsave(&mdesc_lock, flags);
> -	if (atomic_dec_and_test(&hp->refcnt)) {
> +	if (refcount_dec_and_test(&hp->refcnt)) {
>   		list_del_init(&hp->list);
>   		hp->mops->free(hp);
>   	}
> @@ -513,7 +514,7 @@ void mdesc_update(void)
>   	if (status != HV_EOK || real_len > len) {
>   		printk(KERN_ERR "MD: mdesc reread fails with %lu\n",
>   		       status);
> -		atomic_dec(&hp->refcnt);
> +		refcount_dec(&hp->refcnt);
>   		mdesc_free(hp);
>   		goto out;
>   	}
> @@ -526,7 +527,7 @@ void mdesc_update(void)
>   	mdesc_notify_clients(orig_hp, hp);
>   
>   	spin_lock_irqsave(&mdesc_lock, flags);
> -	if (atomic_dec_and_test(&orig_hp->refcnt))
> +	if (refcount_dec_and_test(&orig_hp->refcnt))
>   		mdesc_free(orig_hp);
>   	else
>   		list_add(&orig_hp->list, &mdesc_zombie_list);
> 

RE: [PATCH] sparc64: convert mdesc_handle.refcnt from atomic_t to refcount_t

[Reshetova, Elena]

> On 10/20/2017 12:57 AM, Elena Reshetova wrote:
> > atomic_t variables are currently used to implement reference
> > counters with the following properties:
> >   - counter is initialized to 1 using atomic_set()
> >   - a resource is freed upon counter reaching zero
> >   - once counter reaches zero, its further
> >     increments aren't allowed
> >   - counter schema uses basic atomic operations
> >     (set, inc, inc_not_zero, dec_and_test, etc.)
> >
> > Such atomic variables should be converted to a newly provided
> > refcount_t type and API that prevents accidental counter overflows
> > and underflows. This is important since overflows and underflows
> > can lead to use-after-free situation and be exploitable.
> >
> > The variable mdesc_handle.refcnt is used as pure reference counter.
> > Convert it to refcount_t and fix up the operations.
> >
> > Suggested-by: Kees Cook <keescook@chromium.org>
> > Reviewed-by: David Windsor <dwindsor@gmail.com>
> > Reviewed-by: Hans Liljestrand <ishkamiel@gmail.com>
> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> 
> Acked-by: Shannon Nelson <shannon.nelson@oracle.com>

Thank you Shannon! Would you be able to take this patch into the respective tree
to propagate normally from there?

Best Regards,
Elena.
> 
> > ---
> >   arch/sparc/kernel/mdesc.c | 17 +++++++++--------
> >   1 file changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/sparc/kernel/mdesc.c b/arch/sparc/kernel/mdesc.c
> > index fa466ce..821a724 100644
> > --- a/arch/sparc/kernel/mdesc.c
> > +++ b/arch/sparc/kernel/mdesc.c
> > @@ -12,6 +12,7 @@
> >   #include <linux/miscdevice.h>
> >   #include <linux/bootmem.h>
> >   #include <linux/export.h>
> > +#include <linux/refcount.h>
> >
> >   #include <asm/cpudata.h>
> >   #include <asm/hypervisor.h>
> > @@ -70,7 +71,7 @@ struct mdesc_handle {
> >   	struct list_head	list;
> >   	struct mdesc_mem_ops	*mops;
> >   	void			*self_base;
> > -	atomic_t		refcnt;
> > +	refcount_t		refcnt;
> >   	unsigned int		handle_size;
> >   	struct mdesc_hdr	mdesc;
> >   };
> > @@ -152,7 +153,7 @@ static void mdesc_handle_init(struct mdesc_handle *hp,
> >   	memset(hp, 0, handle_size);
> >   	INIT_LIST_HEAD(&hp->list);
> >   	hp->self_base = base;
> > -	atomic_set(&hp->refcnt, 1);
> > +	refcount_set(&hp->refcnt, 1);
> >   	hp->handle_size = handle_size;
> >   }
> >
> > @@ -182,7 +183,7 @@ static void __init mdesc_memblock_free(struct
> mdesc_handle *hp)
> >   	unsigned int alloc_size;
> >   	unsigned long start;
> >
> > -	BUG_ON(atomic_read(&hp->refcnt) != 0);
> > +	BUG_ON(refcount_read(&hp->refcnt) != 0);
> >   	BUG_ON(!list_empty(&hp->list));
> >
> >   	alloc_size = PAGE_ALIGN(hp->handle_size);
> > @@ -220,7 +221,7 @@ static struct mdesc_handle *mdesc_kmalloc(unsigned int
> mdesc_size)
> >
> >   static void mdesc_kfree(struct mdesc_handle *hp)
> >   {
> > -	BUG_ON(atomic_read(&hp->refcnt) != 0);
> > +	BUG_ON(refcount_read(&hp->refcnt) != 0);
> >   	BUG_ON(!list_empty(&hp->list));
> >
> >   	kfree(hp->self_base);
> > @@ -259,7 +260,7 @@ struct mdesc_handle *mdesc_grab(void)
> >   	spin_lock_irqsave(&mdesc_lock, flags);
> >   	hp = cur_mdesc;
> >   	if (hp)
> > -		atomic_inc(&hp->refcnt);
> > +		refcount_inc(&hp->refcnt);
> >   	spin_unlock_irqrestore(&mdesc_lock, flags);
> >
> >   	return hp;
> > @@ -271,7 +272,7 @@ void mdesc_release(struct mdesc_handle *hp)
> >   	unsigned long flags;
> >
> >   	spin_lock_irqsave(&mdesc_lock, flags);
> > -	if (atomic_dec_and_test(&hp->refcnt)) {
> > +	if (refcount_dec_and_test(&hp->refcnt)) {
> >   		list_del_init(&hp->list);
> >   		hp->mops->free(hp);
> >   	}
> > @@ -513,7 +514,7 @@ void mdesc_update(void)
> >   	if (status != HV_EOK || real_len > len) {
> >   		printk(KERN_ERR "MD: mdesc reread fails with %lu\n",
> >   		       status);
> > -		atomic_dec(&hp->refcnt);
> > +		refcount_dec(&hp->refcnt);
> >   		mdesc_free(hp);
> >   		goto out;
> >   	}
> > @@ -526,7 +527,7 @@ void mdesc_update(void)
> >   	mdesc_notify_clients(orig_hp, hp);
> >
> >   	spin_lock_irqsave(&mdesc_lock, flags);
> > -	if (atomic_dec_and_test(&orig_hp->refcnt))
> > +	if (refcount_dec_and_test(&orig_hp->refcnt))
> >   		mdesc_free(orig_hp);
> >   	else
> >   		list_add(&orig_hp->list, &mdesc_zombie_list);
> >

Re: [PATCH] sparc64: convert mdesc_handle.refcnt from atomic_t to refcount_t

[Shannon Nelson]

On 10/23/2017 12:10 AM, Reshetova, Elena wrote:
>> On 10/20/2017 12:57 AM, Elena Reshetova wrote:
>>> atomic_t variables are currently used to implement reference
>>> counters with the following properties:
>>>    - counter is initialized to 1 using atomic_set()
>>>    - a resource is freed upon counter reaching zero
>>>    - once counter reaches zero, its further
>>>      increments aren't allowed
>>>    - counter schema uses basic atomic operations
>>>      (set, inc, inc_not_zero, dec_and_test, etc.)
>>>
>>> Such atomic variables should be converted to a newly provided
>>> refcount_t type and API that prevents accidental counter overflows
>>> and underflows. This is important since overflows and underflows
>>> can lead to use-after-free situation and be exploitable.
>>>
>>> The variable mdesc_handle.refcnt is used as pure reference counter.
>>> Convert it to refcount_t and fix up the operations.
>>>
>>> Suggested-by: Kees Cook <keescook@chromium.org>
>>> Reviewed-by: David Windsor <dwindsor@gmail.com>
>>> Reviewed-by: Hans Liljestrand <ishkamiel@gmail.com>
>>> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
>>
>> Acked-by: Shannon Nelson <shannon.nelson@oracle.com>
> 
> Thank you Shannon! Would you be able to take this patch into the respective tree
> to propagate normally from there?
> 
> Best Regards,
> Elena.

Hi Elena,

Dave Miller takes good care of the sparclinux tree, I'm sure this is on 
his ToDo list already.

sln

Re: [PATCH] sparc64: convert mdesc_handle.refcnt from atomic_t to refcount_t

[David Miller]

From: Elena Reshetova <elena.reshetova@intel.com>
Date: Fri, 20 Oct 2017 10:57:57 +0300

> atomic_t variables are currently used to implement reference
> counters with the following properties:
>  - counter is initialized to 1 using atomic_set()
>  - a resource is freed upon counter reaching zero
>  - once counter reaches zero, its further
>    increments aren't allowed
>  - counter schema uses basic atomic operations
>    (set, inc, inc_not_zero, dec_and_test, etc.)
> 
> Such atomic variables should be converted to a newly provided
> refcount_t type and API that prevents accidental counter overflows
> and underflows. This is important since overflows and underflows
> can lead to use-after-free situation and be exploitable.
> 
> The variable mdesc_handle.refcnt is used as pure reference counter.
> Convert it to refcount_t and fix up the operations.
> 
> Suggested-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: David Windsor <dwindsor@gmail.com>
> Reviewed-by: Hans Liljestrand <ishkamiel@gmail.com>
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>

Applied.