* [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.