| Message ID | 20180729130248.29612-1-tomasbortoli@gmail.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | 9p: fix Use-After-Free in p9_write_work() | expand |
Tomas Bortoli wrote on Sun, Jul 29, 2018: > 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 <tomasbortoli@gmail.com> > Reported-by: syzbot+467050c1ce275af2a5b8@syzkaller.appspotmail.com 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? > --- > > 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. > 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,
Dominique Martinet wrote on Mon, Jul 30, 2018: > 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)
On Mon, Jul 30, 2018 at 1:33 AM, Dominique Martinet <asmadeus@codewreck.org> wrote: > Tomas Bortoli wrote on Sun, Jul 29, 2018: >> 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 <tomasbortoli@gmail.com> >> Reported-by: syzbot+467050c1ce275af2a5b8@syzkaller.appspotmail.com > > 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. > 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
Dmitry Vyukov wrote on Mon, Jul 30, 2018: > On Mon, Jul 30, 2018 at 1:33 AM, Dominique Martinet > <asmadeus@codewreck.org> wrote: > > Tomas Bortoli wrote on Sun, Jul 29, 2018: > >> 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 <tomasbortoli@gmail.com> > >> Reported-by: syzbot+467050c1ce275af2a5b8@syzkaller.appspotmail.com > > > > 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. > > 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,
On 07/30/2018 01:33 AM, Dominique Martinet wrote: > Tomas Bortoli wrote on Sun, Jul 29, 2018: >> 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 <tomasbortoli@gmail.com> >> Reported-by: syzbot+467050c1ce275af2a5b8@syzkaller.appspotmail.com > > 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? > >> --- >> >> 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... :) > Yep, Oops > > 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. > > Which commit ? that's a comment. That sound like the proper solution. Let's do it that way then. > 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. > > >> 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. > - 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. > > Thanks, >
Tomas Bortoli wrote on Mon, Jul 30, 2018: > > 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. > > Which commit ? that's a comment. Sorry, the commit is this one: http://lkml.kernel.org/r/20180711210225.19730-6-willy@infradead.org 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. > That sound like the proper solution. Let's do it that way then. Cool :)
diff --git a/include/net/9p/trans_fd.h b/include/net/9p/trans_fd.h new file mode 100644 index 000000000000..cfd4457c40fb --- /dev/null +++ b/include/net/9p/trans_fd.h @@ -0,0 +1,139 @@ +/* + * include/fs/9p/trans_fd.h + * + * Fd transport layer definitions. + * + * Copyright (C) 2006 by Russ Cox <rsc@swtch.com> + * Copyright (C) 2004-2005 by Latchesar Ionkov <lucho@ionkov.net> + * Copyright (C) 2004-2008 by Eric Van Hensbergen <ericvh@gmail.com> + * Copyright (C) 1997-2002 by Ron Minnich <rminnich@sarnoff.com> + * + * 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 diff --git a/net/9p/client.c b/net/9p/client.c index 2ec0edc6104f..ddfb63672a63 100644 --- a/net/9p/client.c +++ b/net/9p/client.c @@ -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); diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c index e2ef3c782c53..63b3c7ef0d90 100644 --- a/net/9p/trans_fd.c +++ b/net/9p/trans_fd.c @@ -45,122 +45,15 @@ #include <net/9p/9p.h> #include <net/9p/client.h> #include <net/9p/transport.h> +#include <net/9p/trans_fd.h> #include <linux/syscalls.h> /* killme */ -#define P9_PORT 564 -#define MAX_SOCK_BUF (64*1024) -#define MAXPOLLWADDR 2 +static void p9_poll_workfn(struct work_struct *work); static struct p9_trans_module p9_tcp_trans; static struct p9_trans_module p9_fd_trans; -/** - * 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) - * - */ - -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 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; -}; - -static void p9_poll_workfn(struct work_struct *work); - static DEFINE_SPINLOCK(p9_poll_lock); static LIST_HEAD(p9_poll_pending_list); static DECLARE_WORK(p9_poll_work, p9_poll_workfn); @@ -765,7 +658,7 @@ static int parse_opts(char *params, struct p9_fd_opts *opts) int r; if (!*p) continue; - token = match_token(p, tokens, args); + token = match_token(p, trans_tokens, args); if ((token != Opt_err) && (token != Opt_privport)) { r = match_int(&args[0], &option); if (r < 0) {
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 <tomasbortoli@gmail.com> Reported-by: syzbot+467050c1ce275af2a5b8@syzkaller.appspotmail.com --- 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