Re: [PATCH 1/2] 9p: p9_client_create: use p9_client_destroy on failure

Christian Schoenebeck
2022-09-04T16:31:22Z

On Sonntag, 4. September 2022 08:39:36 CEST Dominique Martinet wrote:
> If trans was connected it's somehow possible to fail with requests in
> flight that could still be accessed after free if we just free the clnt
> on failure.
> Just use p9_client_destroy instead that has proper safeguards.
>
> Reported-by: syzbot+de52531662ebb8823b26@syzkaller.appspotmail.com
> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
> ---

Reviewed-by: Christian Schoenebeck <linux_oss@crudebyte.com>

> net/9p/client.c | 24 ++++++++----------------
> 1 file changed, 8 insertions(+), 16 deletions(-)
>
> diff --git a/net/9p/client.c b/net/9p/client.c
> index 0a6110e15d0f..d340dbbd2ace 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -931,14 +931,10 @@ struct p9_client *p9_client_create(const char
> *dev_name, char *options) char *client_id;
>
> err = 0;
> - clnt = kmalloc(sizeof(*clnt), GFP_KERNEL);
> + clnt = kzalloc(sizeof(*clnt), GFP_KERNEL);
> if (!clnt)
> return ERR_PTR(-ENOMEM);
>
> - clnt->trans_mod = NULL;
> - clnt->trans = NULL;
> - clnt->fcall_cache = NULL;
> -
> client_id = utsname()->nodename;
> memcpy(clnt->name, client_id, strlen(client_id) + 1);
>
> @@ -948,7 +944,7 @@ struct p9_client *p9_client_create(const char *dev_name,
> char *options)
>
> err = parse_opts(options, clnt);
> if (err < 0)
> - goto free_client;
> + goto out;
>
> if (!clnt->trans_mod)
> clnt->trans_mod = v9fs_get_default_trans();
> @@ -957,7 +953,7 @@ struct p9_client *p9_client_create(const char *dev_name,
> char *options) err = -EPROTONOSUPPORT;
> p9_debug(P9_DEBUG_ERROR,
> "No transport defined or default transport\n");
> - goto free_client;
> + goto out;
> }
>
> p9_debug(P9_DEBUG_MUX, "clnt %p trans %p msize %d protocol %d\n",
> @@ -965,7 +961,7 @@ struct p9_client *p9_client_create(const char *dev_name,
> char *options)
>
> err = clnt->trans_mod->create(clnt, dev_name, options);
> if (err)
> - goto put_trans;
> + goto out;
>
> if (clnt->msize > clnt->trans_mod->maxsize) {
> clnt->msize = clnt->trans_mod->maxsize;
> @@ -979,12 +975,12 @@ struct p9_client *p9_client_create(const char
> *dev_name, char *options) p9_debug(P9_DEBUG_ERROR,
> "Please specify a msize of at least 4k\n");
> err = -EINVAL;
> - goto close_trans;
> + goto out;
> }
>
> err = p9_client_version(clnt);
> if (err)
> - goto close_trans;
> + goto out;
>
> /* P9_HDRSZ + 4 is the smallest packet header we can have that is
> * followed by data accessed from userspace by read
> @@ -997,12 +993,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);