cifs: fix use-after-free bug in find_writable_file

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 <[email protected]>

---
 fs/cifs/file.c | 1 +
 1 file changed, 1 insertion(+)

fs/cifs/file.c +0 -0

index a94b3e6..ca30c39 100644

Quoted 61 lines · On Fri, 13 Mar 2015 14:20:29 +0100 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 <[email protected]>

---
 fs/cifs/file.c | 1 +
 1 file changed, 1 insertion(+)

fs/cifs/file.c +0 -0

index a94b3e6..ca30c39 100644

fs/cifs/file.c refind_writable: · line 1829

Quoted 7 lines

fs/cifs/file.c +1 -0

@@ -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 <[email protected]>

--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html