All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] cifs: fix use-after-free bug in find_writable_file
@ 2015-03-13 13:20 David Disseldorp
       [not found] ` <1426252829-31444-1-git-send-email-ddiss-l3A5Bk7waGM@public.gmane.org>
  0 siblings, 1 reply; 3+ messages in thread
From: David Disseldorp @ 2015-03-13 13:20 UTC (permalink / raw)
  To: linux-cifs-u79uwXL29TY76Z2rM5mHXA; +Cc: David Disseldorp

Under intermittent network outages, find_writable_file() is susceptible
to the following race condition, which results in a user-after-free in
the cifs_writepages code-path:

Thread 1                                        Thread 2
========                                        ========

inv_file = NULL
refind = 0
spin_lock(&cifs_file_list_lock)

// invalidHandle found on openFileList

inv_file = open_file
// inv_file->count currently 1

cifsFileInfo_get(inv_file)
// inv_file->count = 2

spin_unlock(&cifs_file_list_lock);

cifs_reopen_file()                            cifs_close()
// fails (rc != 0)                            ->cifsFileInfo_put()
                                       spin_lock(&cifs_file_list_lock)
                                       // inv_file->count = 1
                                       spin_unlock(&cifs_file_list_lock)

spin_lock(&cifs_file_list_lock);
list_move_tail(&inv_file->flist,
      &cifs_inode->openFileList);
spin_unlock(&cifs_file_list_lock);

cifsFileInfo_put(inv_file);
->spin_lock(&cifs_file_list_lock)

  // inv_file->count = 0
  list_del(&cifs_file->flist);
  // cleanup!!
  kfree(cifs_file);

  spin_unlock(&cifs_file_list_lock);

spin_lock(&cifs_file_list_lock);
++refind;
// refind = 1
goto refind_writable;

At this point we loop back through with an invalid inv_file pointer
and a refind value of 1. On second pass, inv_file is not overwritten on
openFileList traversal, and is subsequently dereferenced.

Signed-off-by: David Disseldorp <ddiss-l3A5Bk7waGM@public.gmane.org>
---
 fs/cifs/file.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index a94b3e6..ca30c39 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -1823,6 +1823,7 @@ refind_writable:
 			cifsFileInfo_put(inv_file);
 			spin_lock(&cifs_file_list_lock);
 			++refind;
+			inv_file = NULL;
 			goto refind_writable;
 		}
 	}
-- 
2.1.4

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

* Re: [PATCH] cifs: fix use-after-free bug in find_writable_file
       [not found] ` <1426252829-31444-1-git-send-email-ddiss-l3A5Bk7waGM@public.gmane.org>
@ 2015-03-14 12:45   ` Jeff Layton
       [not found]     ` <20150314084523.7c615d42-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
  0 siblings, 1 reply; 3+ messages in thread
From: Jeff Layton @ 2015-03-14 12:45 UTC (permalink / raw)
  To: David Disseldorp; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA

On Fri, 13 Mar 2015 14:20:29 +0100
David Disseldorp <ddiss-l3A5Bk7waGM@public.gmane.org> wrote:

> Under intermittent network outages, find_writable_file() is susceptible
> to the following race condition, which results in a user-after-free in
> the cifs_writepages code-path:
> 
> Thread 1                                        Thread 2
> ========                                        ========
> 
> inv_file = NULL
> refind = 0
> spin_lock(&cifs_file_list_lock)
> 
> // invalidHandle found on openFileList
> 
> inv_file = open_file
> // inv_file->count currently 1
> 
> cifsFileInfo_get(inv_file)
> // inv_file->count = 2
> 
> spin_unlock(&cifs_file_list_lock);
> 
> cifs_reopen_file()                            cifs_close()
> // fails (rc != 0)                            ->cifsFileInfo_put()
>                                        spin_lock(&cifs_file_list_lock)
>                                        // inv_file->count = 1
>                                        spin_unlock(&cifs_file_list_lock)
> 
> spin_lock(&cifs_file_list_lock);
> list_move_tail(&inv_file->flist,
>       &cifs_inode->openFileList);
> spin_unlock(&cifs_file_list_lock);
> 
> cifsFileInfo_put(inv_file);
> ->spin_lock(&cifs_file_list_lock)
> 
>   // inv_file->count = 0
>   list_del(&cifs_file->flist);
>   // cleanup!!
>   kfree(cifs_file);
> 
>   spin_unlock(&cifs_file_list_lock);
> 
> spin_lock(&cifs_file_list_lock);
> ++refind;
> // refind = 1
> goto refind_writable;
> 
> At this point we loop back through with an invalid inv_file pointer
> and a refind value of 1. On second pass, inv_file is not overwritten on
> openFileList traversal, and is subsequently dereferenced.
> 
> Signed-off-by: David Disseldorp <ddiss-l3A5Bk7waGM@public.gmane.org>
> ---
>  fs/cifs/file.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index a94b3e6..ca30c39 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -1823,6 +1823,7 @@ refind_writable:
>  			cifsFileInfo_put(inv_file);
>  			spin_lock(&cifs_file_list_lock);
>  			++refind;
> +			inv_file = NULL;
>  			goto refind_writable;
>  		}
>  	}

Looks right. Probably also a stable candidate?

Reviewed-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

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

* Re: [PATCH] cifs: fix use-after-free bug in find_writable_file
       [not found]     ` <20150314084523.7c615d42-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
@ 2015-03-14 17:23       ` Steve French
  0 siblings, 0 replies; 3+ messages in thread
From: Steve French @ 2015-03-14 17:23 UTC (permalink / raw)
  To: Jeff Layton; +Cc: David Disseldorp, linux-cifs-u79uwXL29TY76Z2rM5mHXA

merged into cifs-2.6.git

added cc:stable

On Sat, Mar 14, 2015 at 7:45 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> On Fri, 13 Mar 2015 14:20:29 +0100
> David Disseldorp <ddiss-l3A5Bk7waGM@public.gmane.org> wrote:
>
>> Under intermittent network outages, find_writable_file() is susceptible
>> to the following race condition, which results in a user-after-free in
>> the cifs_writepages code-path:
>>
>> Thread 1                                        Thread 2
>> ========                                        ========
>>
>> inv_file = NULL
>> refind = 0
>> spin_lock(&cifs_file_list_lock)
>>
>> // invalidHandle found on openFileList
>>
>> inv_file = open_file
>> // inv_file->count currently 1
>>
>> cifsFileInfo_get(inv_file)
>> // inv_file->count = 2
>>
>> spin_unlock(&cifs_file_list_lock);
>>
>> cifs_reopen_file()                            cifs_close()
>> // fails (rc != 0)                            ->cifsFileInfo_put()
>>                                        spin_lock(&cifs_file_list_lock)
>>                                        // inv_file->count = 1
>>                                        spin_unlock(&cifs_file_list_lock)
>>
>> spin_lock(&cifs_file_list_lock);
>> list_move_tail(&inv_file->flist,
>>       &cifs_inode->openFileList);
>> spin_unlock(&cifs_file_list_lock);
>>
>> cifsFileInfo_put(inv_file);
>> ->spin_lock(&cifs_file_list_lock)
>>
>>   // inv_file->count = 0
>>   list_del(&cifs_file->flist);
>>   // cleanup!!
>>   kfree(cifs_file);
>>
>>   spin_unlock(&cifs_file_list_lock);
>>
>> spin_lock(&cifs_file_list_lock);
>> ++refind;
>> // refind = 1
>> goto refind_writable;
>>
>> At this point we loop back through with an invalid inv_file pointer
>> and a refind value of 1. On second pass, inv_file is not overwritten on
>> openFileList traversal, and is subsequently dereferenced.
>>
>> Signed-off-by: David Disseldorp <ddiss-l3A5Bk7waGM@public.gmane.org>
>> ---
>>  fs/cifs/file.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
>> index a94b3e6..ca30c39 100644
>> --- a/fs/cifs/file.c
>> +++ b/fs/cifs/file.c
>> @@ -1823,6 +1823,7 @@ refind_writable:
>>                       cifsFileInfo_put(inv_file);
>>                       spin_lock(&cifs_file_list_lock);
>>                       ++refind;
>> +                     inv_file = NULL;
>>                       goto refind_writable;
>>               }
>>       }
>
> Looks right. Probably also a stable candidate?
>
> Reviewed-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Thanks,

Steve

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

end of thread, other threads:[~2015-03-14 17:23 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-13 13:20 [PATCH] cifs: fix use-after-free bug in find_writable_file David Disseldorp
     [not found] ` <1426252829-31444-1-git-send-email-ddiss-l3A5Bk7waGM@public.gmane.org>
2015-03-14 12:45   ` Jeff Layton
     [not found]     ` <20150314084523.7c615d42-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
2015-03-14 17:23       ` Steve French

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.