9p: fix Use-After-Free in p9_write_work()

PATCH In review Tomas Bortoli 3 participants 7 messages Jul 29, 2018 – Jul 30, 2018

Subsystems (3)

Networking Network drivers 9P file system

lore

Share & tools

lore

Tomas Bortoli

author

2018-07-29 13:02 UTC #0 lore raw

PATCH 1/1 9p: fix Use-After-Free in p9_write_work()

There is a race condition between p9_free_req() and p9_write_work(). A request might still need to be processed while p9_free_req() is called.

To fix it, flush the read/write work before freeing any request.

Signed-off-by Tomas Bortoli <[email protected]>

Reported-by [email protected]

---

To be able to flush the r/w work from client.c we need the p9_conn and p9_trans_fd definitions. Therefore this commit moves most of the declarations in trans_fd.c to trans_fd.h and import such file in client.c

Moreover, a couple of identifiers were altered to avoid name conflicts with the new import.

 include/net/9p/trans_fd.h | 139 ++++++++++++++++++++++++++++++++++++++++++++++
 net/9p/client.c           |   8 ++-
 net/9p/trans_fd.c         | 113 +------------------------------------
 3 files changed, 148 insertions(+), 112 deletions(-)
 create mode 100644 include/net/9p/trans_fd.h

include/net/9p/trans_fd.h new file +139 -0

new file mode 100644
index 000000000000..cfd4457c40fb
@@ -0,0 +1,139 @@
+/*
+ * include/fs/9p/trans_fd.h
+ *
+ * Fd transport layer definitions.
+ *
+ *  Copyright (C) 2006 by Russ Cox <[email protected]>
+ *  Copyright (C) 2004-2005 by Latchesar Ionkov <[email protected]>
+ *  Copyright (C) 2004-2008 by Eric Van Hensbergen <[email protected]>
+ *  Copyright (C) 1997-2002 by Ron Minnich <[email protected]>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2
+ *  as published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to:
+ *  Free Software Foundation
+ *  51 Franklin Street, Fifth Floor
+ *  Boston, MA  02111-1301  USA
+ *
+ */
+
+#ifndef P9_TRANS_FD_H
+#define P9_TRANS_FD_H
+
+/**
+ * struct p9_fd_opts - per-transport options
+ * @rfd: file descriptor for reading (trans=fd)
+ * @wfd: file descriptor for writing (trans=fd)
+ * @port: port to connect to (trans=tcp)
+ *
+ */
+
+#define P9_PORT 564
+#define MAX_SOCK_BUF (64*1024)
+#define MAXPOLLWADDR	2
+
+struct p9_fd_opts {
+	int rfd;
+	int wfd;
+	u16 port;
+	bool privport;
+};
+
+/*
+  * Option Parsing (code inspired by NFS code)
+  *  - a little lazy - parse all fd-transport options
+  */
+
+enum {
+	/* Options that take integer arguments */
+	Opt_port, Opt_rfdno, Opt_wfdno, Opt_err,
+	/* Options that take no arguments */
+	Opt_privport,
+};
+
+static const match_table_t trans_tokens = {
+	{Opt_port, "port=%u"},
+	{Opt_rfdno, "rfdno=%u"},
+	{Opt_wfdno, "wfdno=%u"},
+	{Opt_privport, "privport"},
+	{Opt_err, NULL},
+};
+
+enum {
+	Rworksched = 1,		/* read work scheduled or running */
+	Rpending = 2,		/* can read */
+	Wworksched = 4,		/* write work scheduled or running */
+	Wpending = 8,		/* can write */
+};
+
+struct p9_poll_wait {
+	struct p9_conn *conn;
+	wait_queue_entry_t wait;
+	wait_queue_head_t *wait_addr;
+};
+
+/**
+ * struct p9_conn - fd mux connection state information
+ * @mux_list: list link for mux to manage multiple connections (?)
+ * @client: reference to client instance for this connection
+ * @err: error state
+ * @req_list: accounting for requests which have been sent
+ * @unsent_req_list: accounting for requests that haven't been sent
+ * @req: current request being processed (if any)
+ * @tmp_buf: temporary buffer to read in header
+ * @rc: temporary fcall for reading current frame
+ * @wpos: write position for current frame
+ * @wsize: amount of data to write for current frame
+ * @wbuf: current write buffer
+ * @poll_pending_link: pending links to be polled per conn
+ * @poll_wait: array of wait_q's for various worker threads
+ * @pt: poll state
+ * @rq: current read work
+ * @wq: current write work
+ * @wsched: ????
+ *
+ */
+
+struct p9_conn {
+	struct list_head mux_list;
+	struct p9_client *client;
+	int err;
+	struct list_head req_list;
+	struct list_head unsent_req_list;
+	struct p9_req_t *req;
+	char tmp_buf[7];
+	struct p9_fcall rc;
+	int wpos;
+	int wsize;
+	char *wbuf;
+	struct list_head poll_pending_link;
+	struct p9_poll_wait poll_wait[MAXPOLLWADDR];
+	poll_table pt;
+	struct work_struct rq;
+	struct work_struct wq;
+	unsigned long wsched;
+};
+
+/**
+ * struct p9_trans_fd - transport state
+ * @rd: reference to file to read from
+ * @wr: reference of file to write to
+ * @conn: connection state reference
+ *
+ */
+
+struct p9_trans_fd {
+	struct file *rd;
+	struct file *wr;
+	struct p9_conn conn;
+};
+
+#endif

net/9p/client.c +6 -2

index 2ec0edc6104f..ddfb63672a63 100644
@@ -40,6 +40,7 @@
 #include <linux/seq_file.h>
 #include <net/9p/client.h>
 #include <net/9p/transport.h>
+#include <net/9p/trans_fd.h>
 #include "protocol.h"
 
 #define CREATE_TRACE_POINTS

@@ -55,7 +56,7 @@ enum {
 	Opt_trans,
 	Opt_legacy,
 	Opt_version,
-	Opt_err,
+	Opt_error,
 };
 
 static const match_table_t tokens = {

@@ -63,7 +64,7 @@ static const match_table_t tokens = {
 	{Opt_legacy, "noextend"},
 	{Opt_trans, "trans=%s"},
 	{Opt_version, "version=%s"},
-	{Opt_err, NULL},
+	{Opt_error, NULL},
 };
 
 inline int p9_is_proto_dotl(struct p9_client *clnt)

@@ -329,12 +330,15 @@ EXPORT_SYMBOL(p9_tag_lookup);
 static void p9_free_req(struct p9_client *c, struct p9_req_t *r)
 {
 	unsigned long flags;
+	struct p9_trans_fd *ts = c->trans;
 	u16 tag = r->tc->tag;
 
 	p9_debug(P9_DEBUG_MUX, "clnt %p req %p tag: %d\n", c, r, tag);
 	spin_lock_irqsave(&c->lock, flags);
 	idr_remove(&c->reqs, tag);
 	spin_unlock_irqrestore(&c->lock, flags);
+	flush_work(&ts->conn.wq);
+	flush_work(&ts->conn.rq);
 	kfree(r->tc);
 	kfree(r->rc);
 	kmem_cache_free(p9_req_cache, r);

net/9p/trans_fd.c +3 ~~-110~~

132 lines not on this page · raw message

Signature · 1 line

2.11.0

Dominique Martinet

maintainer

2018-07-29 23:33 UTC #1 lore raw

Tomas Bortoli wrote on Sun, Jul 29, 2018:

Quoted 7 lines

There is a race condition between p9_free_req() and p9_write_work(). A request might still need to be processed while p9_free_req() is called.
To fix it, flush the read/write work before freeing any request.
Signed-off-by Tomas Bortoli <[email protected]>
Reported-by [email protected]

It looks like I have not received this report, I found it through google in the lkml archives but Dmitry do you have a convenient-ish way of finding the report on the syzkaller website with that reported-by tag?

Quoted 5 lines

---
To be able to flush the r/w work from client.c we need the p9_conn and p9_trans_fd definitions. Therefore this commit moves most of the declarations in trans_fd.c to trans_fd.h and import such file in client.c

This cannot work as it is, because you're not just intorudcing the trans_fd types but you're really depending on the transport used being fd.
'conn.wq' won't even be valid memory in other transports so I don't want to know what trying to flush_work on this will do... :)

Other transports also have the same issue see discussion in
https://lkml.org/lkml/2018/7/19/727
(that is another syzbot report, slightly different but I believe it points to the same issue)

Basically, a more global view of the problem is a race between p9_tag_lookup returning a p9_req_t and another thread freeing it.

Matthew wrote the problem himself in a comment in p9_tag_lookup in his new version that used to be in linux-next at the time (I took the commit out temporarily until I've had time to benchmark it, but it will come back in, just you're working on thin air right now because the bug was only found thanks to this commit):
+ /* There's no refcount on the req; a malicious server could
cause
+ * us to dereference a NULL pointer
+ */

So a more proper solution would be to had a refcount to req, have p9_tag_lookup increment the refcount within rcu_read_lock, and have a deref function free the req when the count hits 0.

Now we're here though I'm not sure what to suggest, I had promised to get some performance benchmark out by this past weekend and I obviously failed, so this patch might be delayed to 4.20 and the refcount approach would not work with the current req cache/reuse system we have right now.
If you want to finish this anyway you can work on my 9p-test branch (I've kept the commit there), and I'll take that patch in at the same time as the other.

Quoted 2 lines

Moreover, a couple of identifiers were altered to avoid name conflicts with the new import.

If we were to stick to this approach, two suggestions:
- headers aren't all-or-nothing, I think it's better to only expose
what you need (and e.g. keep the Opt_* enum in the .c)
- instead of exposing trans_fd specific stuff, it's cleaner to add a
new op vector '.flush' to the p9_trans_module struct and call that if it exists.

Thanks,

Signature · 1 line

Dominique

Dominique Martinet

maintainer

2018-07-30 00:18 UTC #2 lore raw

Dominique Martinet wrote on Mon, Jul 30, 2018:

Quoted 2 lines

Basically, a more global view of the problem is a race between p9_tag_lookup returning a p9_req_t and another thread freeing it.

(just correcting myself here, p9_tag_lookup won't be enough for the write side, but similarily you'd just need to increment the refcount when you schedule work with it and decrement when the worker is done)

Signature · 1 line

Dominique

Dmitry Vyukov

2018-07-30 05:55 UTC #3 lore raw

In reply to Dominique Martinet

Quoted 11 lines · Dominique Martinet

Tomas Bortoli wrote on Sun, Jul 29, 2018:
Quoted 7 lines
There is a race condition between p9_free_req() and p9_write_work(). A request might still need to be processed while p9_free_req() is called.
To fix it, flush the read/write work before freeing any request.
Signed-off-by Tomas Bortoli <[email protected]>
Reported-by [email protected]
It looks like I have not received this report, I found it through google in the lkml archives

But you should have been received it? Or not?
We had some complaints that syzbot emails were not delivered, but in these cases they were not delivered to lkml, and only to explicitly CCed people.

Quoted 2 lines

but Dmitry do you have a convenient-ish way of
finding the report on the syzkaller website with that reported-by tag?

Well, you can do:
http://syzkaller.appspot.com/bug?extid=467050c1ce275af2a5b8

Dominique Martinet

maintainer

2018-07-30 06:00 UTC #4 lore raw

Dmitry Vyukov wrote on Mon, Jul 30, 2018:

Quoted 11 lines

Quoted 9 lines · Dominique Martinet
Tomas Bortoli wrote on Sun, Jul 29, 2018:
Quoted 7 lines
There is a race condition between p9_free_req() and p9_write_work(). A request might still need to be processed while p9_free_req() is called.
To fix it, flush the read/write work before freeing any request.
Signed-off-by Tomas Bortoli <[email protected]>
Reported-by [email protected]

Quoted 7 lines

Quoted 2 lines
It looks like I have not received this report, I found it through google in the lkml archives
But you should have been received it? Or not?
We had some complaints that syzbot emails were not delivered, but in these cases they were not delivered to lkml, and only to explicitly CCed people.

I'm not on lkml and the archives I found do not list who were Cc'd that I can see - it might have tried to send a copy to v9fs-developer that is held up in the moderation queue for all I know :/

I'm not complaining I didn't get a copy (if I ever find time to work on these, I can work through the list on the website) - I just need to know how to find the report corresponding to patchs being sent.

Quoted 5 lines

Quoted 2 lines
but Dmitry do you have a convenient-ish way of
finding the report on the syzkaller website with that reported-by tag?
Well, you can do:
http://syzkaller.appspot.com/bug?extid=467050c1ce275af2a5b8

Thanks, for some reason I only thought of bug?id= which didn't work, this is perfect.

Cheers,

Signature · 1 line

Dominique

Tomas Bortoli

author

2018-07-30 09:46 UTC #5 lore raw

In reply to Dominique Martinet

Quoted 19 lines · Dominique Martinet

Tomas Bortoli wrote on Sun, Jul 29, 2018:
Quoted 7 lines
There is a race condition between p9_free_req() and p9_write_work(). A request might still need to be processed while p9_free_req() is called.
To fix it, flush the read/write work before freeing any request.
Signed-off-by Tomas Bortoli <[email protected]>
Reported-by [email protected]
It looks like I have not received this report, I found it through google in the lkml archives but Dmitry do you have a convenient-ish way of finding the report on the syzkaller website with that reported-by tag?
Quoted 5 lines
---
To be able to flush the r/w work from client.c we need the p9_conn and p9_trans_fd definitions. Therefore this commit moves most of the declarations in trans_fd.c to trans_fd.h and import such file in client.c

Quoted 6 lines

This cannot work as it is, because you're not just intorudcing the trans_fd types but you're really depending on the transport used being fd.
'conn.wq' won't even be valid memory in other transports so I don't want to know what trying to flush_work on this will do... :)

Yep, Oops

Quoted 19 lines

Other transports also have the same issue see discussion in
https://lkml.org/lkml/2018/7/19/727
(that is another syzbot report, slightly different but I believe it points to the same issue)
Basically, a more global view of the problem is a race between p9_tag_lookup returning a p9_req_t and another thread freeing it.
Matthew wrote the problem himself in a comment in p9_tag_lookup in his new version that used to be in linux-next at the time (I took the commit out temporarily until I've had time to benchmark it, but it will come back in, just you're working on thin air right now because the bug was only found thanks to this commit):
+ /* There's no refcount on the req; a malicious server could
cause
(quoted diff) +2 -0
+        * us to dereference a NULL pointer
+        */

Quoted 5 lines

So a more proper solution would be to had a refcount to req, have p9_tag_lookup increment the refcount within rcu_read_lock, and have a deref function free the req when the count hits 0.

Which commit ? that's a comment.

That sound like the proper solution. Let's do it that way then.

Quoted 10 lines

Now we're here though I'm not sure what to suggest, I had promised to get some performance benchmark out by this past weekend and I obviously failed, so this patch might be delayed to 4.20 and the refcount approach would not work with the current req cache/reuse system we have right now.
If you want to finish this anyway you can work on my 9p-test branch (I've kept the commit there), and I'll take that patch in at the same time as the other.

Quoted 6 lines

Quoted 2 lines
Moreover, a couple of identifiers were altered to avoid name conflicts with the new import.
If we were to stick to this approach, two suggestions:
- headers aren't all-or-nothing, I think it's better to only expose
what you need (and e.g. keep the Opt_* enum in the .c)

No we don't have to stick with this patch.

Quoted 4 lines

- instead of exposing trans_fd specific stuff, it's cleaner to add a
new op vector '.flush' to the p9_trans_module struct and call that if it exists.

I didn't thought at this way, it's smarter.

Quoted 3 lines

Thanks,

Dominique Martinet

maintainer

2018-07-30 10:23 UTC #6 lore raw

Tomas Bortoli wrote on Mon, Jul 30, 2018:

Quoted 18 lines

Quoted 18 lines
Other transports also have the same issue see discussion in
https://lkml.org/lkml/2018/7/19/727
(that is another syzbot report, slightly different but I believe it points to the same issue)
Basically, a more global view of the problem is a race between p9_tag_lookup returning a p9_req_t and another thread freeing it.
Matthew wrote the problem himself in a comment in p9_tag_lookup in his new version that used to be in linux-next at the time (I took the commit out temporarily until I've had time to benchmark it, but it will come back in, just you're working on thin air right now because the bug was only found thanks to this commit):
+ /* There's no refcount on the req; a malicious server could
cause
(quoted diff) +2 -0
+        * us to dereference a NULL pointer
+        */

Quoted 5 lines

Quoted 3 lines
So a more proper solution would be to had a refcount to req, have p9_tag_lookup increment the refcount within rcu_read_lock, and have a deref function free the req when the count hits 0.
Which commit ? that's a comment.

Sorry, the commit is this one:
http://lkml.kernel.org/r/[email protected]

It's now out of my 9p-next branch due to performance reasons but I'll definitely take it back in once my performance mitigation patches have had a few reviews.

Quoted 1 line

That sound like the proper solution. Let's do it that way then.

Cool :)

Signature · 1 line

Dominique