Messages in this thread | | | Date | Fri, 19 Aug 2022 05:23:36 +0900 | From | asmadeus@codewrec ... | Subject | Re: [syzbot] KASAN: use-after-free Read in p9_req_put |
| |
Christian Schoenebeck wrote on Thu, Aug 18, 2022 at 05:12:17PM +0200: > > @@ -997,12 +997,8 @@ struct p9_client *p9_client_create(const char > > *dev_name, char *options) > > > > return clnt; > > > > -close_trans: > > - clnt->trans_mod->close(clnt); > > -put_trans: > > - v9fs_put_trans(clnt->trans_mod); > > -free_client: > > - kfree(clnt); > > +out: > > + p9_client_destroy(clnt); > > return ERR_PTR(err); > > } > > EXPORT_SYMBOL(p9_client_create); > > Looks like a nice reduction to me! > > As p9_client_destroy() is doing a bit more than current code, I would probably > additionally do s/kmalloc/kzmalloc/ at the start of the function, which would > add more safety & reduction.
Good point, I checked the variables p9_client_destroy cares about get initialized but kzalloc is safer, let's switch that as well. > > diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c > > index e758978b44be..60fcc6b30b46 100644 > > --- a/net/9p/trans_fd.c > > +++ b/net/9p/trans_fd.c > > @@ -205,6 +205,8 @@ static void p9_conn_cancel(struct p9_conn *m, int err) > > list_move(&req->req_list, &cancel_list); > > } > > > > + spin_unlock(&m->client->lock); > > + > > list_for_each_entry_safe(req, rtmp, &cancel_list, req_list) { > > p9_debug(P9_DEBUG_ERROR, "call back req %p\n", req); > > list_del(&req->req_list); > > @@ -212,7 +214,6 @@ static void p9_conn_cancel(struct p9_conn *m, int err) > > req->t_err = err; > > p9_client_cb(m->client, req, REQ_STATUS_ERROR); > > } > > - spin_unlock(&m->client->lock); > > } > > Are you sure that would resolve that (other) syzbot report? I just had a > glimpse at it yet, but I don't see this list iteration portion being involved > in the backtrace provided by the report, is it?
It won't fix the inconsistent locking ones, but should take care of http://lkml.kernel.org/r/000000000000cad57405e5b5dbb7@google.com
ffff888026be2c18 (&clnt->lock){+.+.}-{2:2}, at: p9_conn_cancel+0xaa/0x970 net/9p/trans_fd.c:192 holding the lock in that function, calling p9_client_cb itself calling p9_req_put and locking again when refcount hits 0.
And that one has a reproducer, so syzbot will confirm if it helps when I get around to pushing it (probably this weekend) :)
-- Dominique
| |