| 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
| ||
|
Message-ID: <82f03be5-b8b1-4df2-8b4b-0cae5d6d67ba@suse.com>
Date: Mon, 22 Jul 2024 10:49:49 +0200
From: Oliver Neukum <oneukum@...e.com>
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: Andrzej Pietrasiewicz <andrzej.p@...labora.com>, 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 v7 2/3] net/9p/usbg: Add new usb gadget function transport
On 22.07.24 00:08, Michael Grzeschik wrote:
> +
> +static int usb9pfs_queue_tx(struct f_usb9pfs *usb9pfs, struct usb_request *req,
> + gfp_t gfp_flags)
> +{
> + struct usb_composite_dev *cdev = usb9pfs->function.config->cdev;
> + int ret = -ENOMEM;
No need. This will be overwritten.
> +
> + if (!(usb9pfs->p9_tx_req->tc.size % usb9pfs->in_ep->maxpacket))
> + req->zero = 1;
> +
> + req->buf = usb9pfs->p9_tx_req->tc.sdata;
> + req->length = usb9pfs->p9_tx_req->tc.size;
> +
> + dev_dbg(&cdev->gadget->dev, "%s usb9pfs send --> %d/%d, zero: %d\n",
> + usb9pfs->in_ep->name, req->actual, req->length, req->zero);
> +
> + ret = usb_ep_queue(usb9pfs->in_ep, req, gfp_flags);
> +
> + dev_dbg(&cdev->gadget->dev, "tx submit --> %d\n", ret);
> +
> + return ret;
> +}
> +
> +static int usb9pfs_queue_rx(struct f_usb9pfs *usb9pfs, struct usb_request *req,
> + gfp_t gfp_flags)
> +{
> + struct usb_composite_dev *cdev = usb9pfs->function.config->cdev;
> + int ret = -ENOMEM;
Overwritten in literally the next statement.
> + ret = usb_ep_queue(usb9pfs->out_ep, req, gfp_flags);
> +
> + dev_dbg(&cdev->gadget->dev, "rx submit --> %d\n", ret);
> +
> + return ret;
> +}
> +
> +static int usb9pfs_transmit(struct f_usb9pfs *usb9pfs)
> +{
> + struct p9_req_t *p9_req = NULL;
> + unsigned long flags;
> + int ret = 0;
> +
> + spin_lock_irqsave(&usb9pfs->lock, flags);
> + if (usb9pfs->p9_tx_req) {
> + spin_unlock_irqrestore(&usb9pfs->lock, flags);
> + return -EBUSY;
> + }
> +
> + p9_req = list_first_entry_or_null(&usb9pfs->tx_req_list,
> + struct p9_req_t, req_list);
> + if (!p9_req) {
> + spin_unlock_irqrestore(&usb9pfs->lock, flags);
> + return -ENOENT;
> + }
> +
> + list_del(&p9_req->req_list);
You have deleted it from the list
> + usb9pfs->p9_tx_req = p9_req;
> +
> + p9_req_get(usb9pfs->p9_tx_req);
> +
> + ret = usb9pfs_queue_tx(usb9pfs, usb9pfs->in_req, GFP_ATOMIC);
This means that if this function returns an error, the deletion
from the list may or may not have happened.
> + spin_unlock_irqrestore(&usb9pfs->lock, flags);
> +
> + return ret;
> +}
> +
> +static void usb9pfs_tx_complete(struct usb_ep *ep, struct usb_request *req)
> +{
> + struct f_usb9pfs *usb9pfs = ep->driver_data;
> + struct usb_composite_dev *cdev = usb9pfs->function.config->cdev;
> + int ret = 0;
> +
> + if (req->status) {
> + dev_err(&cdev->gadget->dev, "%s usb9pfs complete --> %d, %d/%d\n",
> + ep->name, req->status, req->actual, req->length);
> + return;
> + }
> +
> + /* reset zero packages */
> + req->zero = 0;
> +
> + dev_dbg(&cdev->gadget->dev, "%s usb9pfs complete --> %d, %d/%d\n",
> + ep->name, req->status, req->actual, req->length);
> +
> + WRITE_ONCE(usb9pfs->p9_tx_req->status, REQ_STATUS_SENT);
> +
> + p9_req_put(usb9pfs->client, usb9pfs->p9_tx_req);
> +
> + ret = usb9pfs_queue_rx(usb9pfs, usb9pfs->out_req, GFP_ATOMIC);
> + if (ret)
> + return;
Ehhh ? Could you explain the error handling here?
> +
> + return;
> +}
> +
> +static struct p9_req_t *usb9pfs_rx_header(struct f_usb9pfs *usb9pfs, void *buf)
> +{
> + struct p9_req_t *p9_rx_req;
> + struct p9_fcall rc;
> + int ret;
> +
> + /* start by reading header */
> + rc.sdata = buf;
> + rc.offset = 0;
> + rc.capacity = P9_HDRSZ;
> + rc.size = P9_HDRSZ;
> +
> + p9_debug(P9_DEBUG_TRANS, "mux %p got %zu bytes\n", usb9pfs,
> + rc.capacity - rc.offset);
> +
> + ret = p9_parse_header(&rc, &rc.size, NULL, NULL, 0);
> + if (ret) {
> + p9_debug(P9_DEBUG_ERROR,
> + "error parsing header: %d\n", ret);
> + return NULL;
> + }
> +
> + p9_debug(P9_DEBUG_TRANS,
> + "mux %p pkt: size: %d bytes tag: %d\n",
> + usb9pfs, rc.size, rc.tag);
> +
> + p9_rx_req = p9_tag_lookup(usb9pfs->client, rc.tag);
> + if (!p9_rx_req || p9_rx_req->status != REQ_STATUS_SENT) {
> + p9_debug(P9_DEBUG_ERROR, "Unexpected packet tag %d\n", rc.tag);
> + return NULL;
> + }
> +
> + if (rc.size > p9_rx_req->rc.capacity) {
> + p9_debug(P9_DEBUG_ERROR,
> + "requested packet size too big: %d for tag %d with capacity %zd\n",
> + rc.size, rc.tag, p9_rx_req->rc.capacity);
> + p9_req_put(usb9pfs->client, p9_rx_req);
> + return NULL;
> + }
> +
> + if (!p9_rx_req->rc.sdata) {
> + p9_debug(P9_DEBUG_ERROR,
> + "No recv fcall for tag %d (req %p), disconnecting!\n",
> + rc.tag, p9_rx_req);
> + p9_req_put(usb9pfs->client, p9_rx_req);
> + return NULL;
> + }
> +
> + return p9_rx_req;
> +}
> +
> +static void usb9pfs_rx_complete(struct usb_ep *ep, struct usb_request *req)
> +{
> + struct f_usb9pfs *usb9pfs = ep->driver_data;
> + struct usb_composite_dev *cdev = usb9pfs->function.config->cdev;
> + struct p9_req_t *p9_rx_req;
> + unsigned long flags;
> +
> + if (req->status) {
> + dev_err(&cdev->gadget->dev, "%s usb9pfs complete --> %d, %d/%d\n",
> + ep->name, req->status, req->actual, req->length);
> + return;
> + }
> +
> + p9_rx_req = usb9pfs_rx_header(usb9pfs, req->buf);
> + if (!p9_rx_req)
> + return;
> +
> + memcpy(p9_rx_req->rc.sdata, req->buf, req->actual);
> +
> + p9_rx_req->rc.size = req->actual;
> +
> + p9_client_cb(usb9pfs->client, p9_rx_req, REQ_STATUS_RCVD);
> + p9_req_put(usb9pfs->client, p9_rx_req);
> +
> + spin_lock_irqsave(&usb9pfs->lock, flags);
> + usb9pfs->p9_tx_req = NULL;
> +
> + spin_unlock_irqrestore(&usb9pfs->lock, flags);
Why can usb9pfs_tx_complete() touch this without taking the spinlock?
> +
> + usb9pfs_transmit(usb9pfs);
This can fail. What happens then?
> +
> + return;
> +}
> +
[..]
> +static int p9_usbg_cancel(struct p9_client *client, struct p9_req_t *req)
This ought to be boolean
> +{
> + struct f_usb9pfs *usb9pfs = client->trans;
> + unsigned long flags;
> + int ret = 1;
> +
> + p9_debug(P9_DEBUG_TRANS, "client %p req %p\n", client, req);
> +
> + spin_lock_irqsave(&usb9pfs->lock, flags);
> +
> + if (req->status == REQ_STATUS_UNSENT) {
> + list_del(&req->req_list);
> + WRITE_ONCE(req->status, REQ_STATUS_FLSHD);
> + p9_req_put(client, req);
> + ret = 0;
> + }
> + spin_unlock_irqrestore(&usb9pfs->lock, flags);
> +
> + return ret;
> +}
Regards
Oliver
Powered by blists - more mailing lists