Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Christian Schoenebeck <linux_oss@crudebyte.com>
To:     v9fs-developer@lists.sourceforge.net,
        Dominique Martinet <asmadeus@codewreck.org>
Cc:     syzkaller-bugs@googlegroups.com, linux-kernel@vger.kernel.org,
        Dominique Martinet <asmadeus@codewreck.org>,
        syzbot+de52531662ebb8823b26@syzkaller.appspotmail.com
Subject: Re: [PATCH 1/2] 9p: p9_client_create: use p9_client_destroy on failure
Date:   Sun, 04 Sep 2022 18:31:12 +0200
Message-ID: <7389539.FyhnmifhmU@silver>
In-Reply-To: <20220904063936.1305139-1-asmadeus@codewreck.org>
References: <0000000000001c3efc05e6693f06@google.com>
 <20220904063936.1305139-1-asmadeus@codewreck.org>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

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);