diff mbox series

fs, cifs: convert tcon_link.tl_count from atomic_t to refcount_t

Message ID 1508496110-2292-1-git-send-email-elena.reshetova@intel.com
State New
Headers show
Series fs, cifs: convert tcon_link.tl_count from atomic_t to refcount_t | expand

Commit Message

Reshetova, Elena Oct. 20, 2017, 10:41 a.m. UTC 
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 tcon_link.tl_count 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>
---
 fs/cifs/cifsglob.h | 5 +++--
 fs/cifs/connect.c  | 8 ++++----
 2 files changed, 7 insertions(+), 6 deletions(-)

Comments

Aurélien Aptel Oct. 30, 2017, 3:03 p.m. UTC | #1 
Elena Reshetova <elena.reshetova@intel.com> writes:
> 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 tcon_link.tl_count is used as pure reference counter.
> Convert it to refcount_t and fix up the operations.

<snip>

> -		cifs_get_tlink(tlink);
> +		refcount_set(&tlink->tl_count, 1);

Since cifs_get_tlink() basically only increments the reference count and
given that this is a new tlink I also think this replacement is ok.

Looks good to me and passes 'make C=1'.

Reviewed-by: Aurelien Aptel <aaptel@suse.com>
Reshetova, Elena Oct. 31, 2017, 9:41 a.m. UTC | #2 
> Elena Reshetova <elena.reshetova@intel.com> writes:

> > 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 tcon_link.tl_count is used as pure reference counter.

> > Convert it to refcount_t and fix up the operations.

> 

> <snip>

> 

> > -		cifs_get_tlink(tlink);

> > +		refcount_set(&tlink->tl_count, 1);

> 

> Since cifs_get_tlink() basically only increments the reference count and

> given that this is a new tlink I also think this replacement is ok.

> 

> Looks good to me and passes 'make C=1'.

> 

> Reviewed-by: Aurelien Aptel <aaptel@suse.com>


Thank you very much for your review Aurelien! 
Do you know who would be the person to take this patch further?

Best Regards,
Elena.

> 

> --

> Aurélien Aptel / SUSE Labs Samba Team

> GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3

> SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany

> GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
Steve French Nov. 1, 2017, 5:56 p.m. UTC | #3 
merged into cifs-2.6.git for-next

On Mon, Oct 30, 2017 at 10:03 AM, Aurélien Aptel <aaptel@suse.com> wrote:
> Elena Reshetova <elena.reshetova@intel.com> writes:
>> 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 tcon_link.tl_count is used as pure reference counter.
>> Convert it to refcount_t and fix up the operations.
>
> <snip>
>
>> -             cifs_get_tlink(tlink);
>> +             refcount_set(&tlink->tl_count, 1);
>
> Since cifs_get_tlink() basically only increments the reference count and
> given that this is a new tlink I also think this replacement is ok.
>
> Looks good to me and passes 'make C=1'.
>
> Reviewed-by: Aurelien Aptel <aaptel@suse.com>
>
> --
> Aurélien Aptel / SUSE Labs Samba Team
> GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
> GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox series

Patch

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index de5b2e1..18963bf 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -28,6 +28,7 @@ 
 #include "cifsacl.h"
 #include <crypto/internal/hash.h>
 #include <linux/scatterlist.h>
+#include <linux/refcount.h>
 #include <uapi/linux/cifs/cifs_mount.h>
 #include "smb2pdu.h"
 
@@ -974,7 +975,7 @@  struct tcon_link {
 #define TCON_LINK_PENDING	1
 #define TCON_LINK_IN_TREE	2
 	unsigned long		tl_time;
-	atomic_t		tl_count;
+	refcount_t		tl_count;
 	struct cifs_tcon	*tl_tcon;
 };
 
@@ -992,7 +993,7 @@  static inline struct tcon_link *
 cifs_get_tlink(struct tcon_link *tlink)
 {
 	if (tlink && !IS_ERR(tlink))
-		atomic_inc(&tlink->tl_count);
+		refcount_inc(&tlink->tl_count);
 	return tlink;
 }
 
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 0bfc228..f861152 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -2868,7 +2868,7 @@  cifs_put_tlink(struct tcon_link *tlink)
 	if (!tlink || IS_ERR(tlink))
 		return;
 
-	if (!atomic_dec_and_test(&tlink->tl_count) ||
+	if (!refcount_dec_and_test(&tlink->tl_count) ||
 	    test_bit(TCON_LINK_IN_TREE, &tlink->tl_flags)) {
 		tlink->tl_time = jiffies;
 		return;
@@ -4328,7 +4328,7 @@  cifs_sb_tlink(struct cifs_sb_info *cifs_sb)
 		newtlink->tl_tcon = ERR_PTR(-EACCES);
 		set_bit(TCON_LINK_PENDING, &newtlink->tl_flags);
 		set_bit(TCON_LINK_IN_TREE, &newtlink->tl_flags);
-		cifs_get_tlink(newtlink);
+		refcount_set(&newtlink->tl_count, 1);
 
 		spin_lock(&cifs_sb->tlink_tree_lock);
 		/* was one inserted after previous search? */
@@ -4406,11 +4406,11 @@  cifs_prune_tlinks(struct work_struct *work)
 		tlink = rb_entry(tmp, struct tcon_link, tl_rbnode);
 
 		if (test_bit(TCON_LINK_MASTER, &tlink->tl_flags) ||
-		    atomic_read(&tlink->tl_count) != 0 ||
+		    refcount_read(&tlink->tl_count) != 0 ||
 		    time_after(tlink->tl_time + TLINK_IDLE_EXPIRE, jiffies))
 			continue;
 
-		cifs_get_tlink(tlink);
+		refcount_set(&tlink->tl_count, 1);
 		clear_bit(TCON_LINK_IN_TREE, &tlink->tl_flags);
 		rb_erase(tmp, root);