linux-kernel - Re: [PATCH 1/3] usb: gadget: function: 9pfs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <de0bd3d3-bb7b-4f94-aae2-0d9323b273fb@wanadoo.fr>
Date: Tue, 16 Jan 2024 22:14:10 +0100
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Michael Grzeschik <m.grzeschik@...gutronix.de>,
 Eric Van Hensbergen <ericvh@...nel.org>, Latchesar Ionkov
 <lucho@...kov.net>, Dominique Martinet <asmadeus@...ewreck.org>,
 Christian Schoenebeck <linux_oss@...debyte.com>,
 Jonathan Corbet <corbet@....net>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: v9fs@...ts.linux.dev, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
 kernel@...gutronix.de
Subject: Re: [PATCH 1/3] usb: gadget: function: 9pfs

Le 16/01/2024 à 02:49, Michael Grzeschik a écrit :
> Add the new gadget function for 9pfs transport. This function is
> defining an simple 9pfs transport interface that consists of one in and
> one out endpoint. The endpoints transmit and receive the 9pfs protocol
> payload when mounting a 9p filesystem over usb.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@...gutronix.de>
> ---
>   Documentation/filesystems/9p.rst     |  12 +
>   drivers/usb/gadget/Kconfig           |  11 +
>   drivers/usb/gadget/function/Makefile |   2 +
>   drivers/usb/gadget/function/f_9pfs.c | 849 +++++++++++++++++++++++++++++++++++
>   4 files changed, 874 insertions(+)

..

> +static int alloc_requests(struct usb_composite_dev *cdev,
> +			  struct f_usb9pfs *usb9pfs)
> +{
> +	int result = 0;
> +
> +	usb9pfs->in_req = usb_ep_alloc_request(usb9pfs->in_ep, GFP_ATOMIC);
> +	if (!usb9pfs->in_req)
> +		goto fail;
> +
> +	usb9pfs->out_req = usb9pfs_alloc_ep_req(usb9pfs->out_ep, usb9pfs->buflen);
> +	if (!usb9pfs->out_req)
> +		goto fail_in;
> +
> +	usb9pfs->in_req->complete = usb9pfs_tx_complete;
> +	usb9pfs->out_req->complete = usb9pfs_rx_complete;
> +
> +	/* length will be set in complete routine */
> +	usb9pfs->in_req->context = usb9pfs->out_req->context = usb9pfs;
> +
> +	return 0;
> +
> +fail_in:
> +	usb_ep_free_request(usb9pfs->in_ep, usb9pfs->in_req);
> +fail:
> +	return result;

'result' is never overwritten, so we always return 0.
Is it on purpose?

> +}

..

> +static struct usb_function *usb9pfs_alloc(struct usb_function_instance *fi)
> +{
> +	struct f_usb9pfs_opts *usb9pfs_opts;
> +	struct f_usb9pfs *usb9pfs;
> +
> +	usb9pfs = kzalloc(sizeof(*usb9pfs), GFP_KERNEL);
> +	if (!usb9pfs)
> +		return ERR_PTR(-ENOMEM);
> +
> +	usb9pfs_opts = container_of(fi, struct f_usb9pfs_opts, func_inst);
> +
> +	mutex_lock(&usb9pfs_opts->lock);
> +	usb9pfs_opts->refcnt++;
> +	mutex_unlock(&usb9pfs_opts->lock);
> +
> +	usb9pfs->buflen = usb9pfs_opts->buflen;
> +
> +	usb9pfs->function.name = "usb9pfs";
> +	usb9pfs->function.bind = usb9pfs_func_bind;
> +	usb9pfs->function.set_alt = usb9pfs_set_alt;
> +	usb9pfs->function.disable = usb9pfs_disable;
> +	usb9pfs->function.strings = usb9pfs_strings;
> +
> +	usb9pfs->function.free_func = usb9pfs_free_func;
> +
> +	mutex_lock(&usb9pfs_ida_lock);
> +
> +	usb9pfs->index = ida_simple_get(&usb9pfs_ida, 0, 100, GFP_KERNEL);

This API will soon be removed.
Please use ida_alloc_max() instead.

Also, there is no corresponding ida_free()? (or isa_simple_remove())

> +	if (usb9pfs->index < 0) {
> +		struct usb_function *ret = ERR_PTR(usb9pfs->index);
> +
> +		kfree(usb9pfs);
> +		mutex_unlock(&usb9pfs_ida_lock);
> +		return ret;
> +	}
> +
> +	mutex_unlock(&usb9pfs_ida_lock);
> +
> +	usb9pfs->tag = kasprintf(GFP_KERNEL, "%s%d", usb9pfs->function.name,
> +				 usb9pfs->index);

Same here, it seems to be freed nowhere?
I think that both should be added in usb9pfs_free_func().

CJ

> +
> +	INIT_LIST_HEAD(&usb9pfs->function_list);
> +
> +	mutex_lock(&usb9pfs_lock);
> +	list_add_tail(&usb9pfs->function_list, &usbg_function_list);
> +	mutex_unlock(&usb9pfs_lock);
> +
> +	return &usb9pfs->function;
> +}

..