| 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
| ||
|
Date: Mon, 22 May 2023 11:21:46 +0200
From: Oliver Neukum <oneukum@...e.com>
To: Ye Xiang <xiang.ye@...el.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Arnd Bergmann <arnd@...db.de>,
Matthias Kaehlcke <mka@...omium.org>,
Lee Jones <lee@...nel.org>, Wolfram Sang <wsa@...nel.org>,
Tyrone Ting <kfting@...oton.com>,
Mark Brown <broonie@...nel.org>,
Linus Walleij <linus.walleij@...aro.org>,
Marc Zyngier <maz@...nel.org>,
Bartosz Golaszewski <brgl@...ev.pl>, linux-usb@...r.kernel.org,
linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-spi@...r.kernel.org, linux-gpio@...r.kernel.org
Cc: srinivas.pandruvada@...el.com, heikki.krogerus@...ux.intel.com,
andriy.shevchenko@...ux.intel.com, sakari.ailus@...ux.intel.com,
zhifeng.wang@...el.com, wentong.wu@...el.com, lixu.zhang@...el.com
Subject: Re: [PATCH v8 1/6] usb: Add support for Intel LJCA device
On 11.05.23 19:58, Ye Xiang wrote:
> Implements the USB part of Intel USB-I2C/GPIO/SPI adapter device
> named "La Jolla Cove Adapter" (LJCA).
>
> The communication between the various LJCA module drivers and the
> hardware will be muxed/demuxed by this driver. Three modules (
> I2C, GPIO, and SPI) are supported currently.
>
> Each sub-module of LJCA device is identified by type field within
> the LJCA message header.
>
Hi,
I am terribly sorry to come up with issues after so many iterations,
but I am really afraid there are issues left.
> The minimum code in ASL that covers this board is
> Scope (\_SB.PCI0.DWC3.RHUB.HS01)
> {
> Device (GPIO)
> {
> Name (_ADR, Zero)
> Name (_STA, 0x0F)
> }
>
> Device (I2C)
> {
> Name (_ADR, One)
> Name (_STA, 0x0F)
> }
>
> Device (SPI)
> {
> Name (_ADR, 0x02)
> Name (_STA, 0x0F)
> }
> }
>
> Signed-off-by: Ye Xiang <xiang.ye@...el.com>
> ---
> +
> +/* MNG stub commands */
> +enum ljca_mng_cmd {
> + LJCA_MNG_GET_VERSION = 1,
> + LJCA_MNG_RESET_NOTIFY,
> + LJCA_MNG_RESET,
> + LJCA_MNG_ENUM_GPIO,
> + LJCA_MNG_ENUM_I2C,
> + LJCA_MNG_POWER_STATE_CHANGE,
> + LJCA_MNG_SET_DFU_MODE,
> + LJCA_MNG_ENUM_SPI,
> +};
> +
> +/* DIAG commands */
> +enum ljca_diag_cmd {
> + LJCA_DIAG_GET_STATE = 1,
> + LJCA_DIAG_GET_STATISTIC,
> + LJCA_DIAG_SET_TRACE_LEVEL,
> + LJCA_DIAG_SET_ECHO_MODE,
> + LJCA_DIAG_GET_FW_LOG,
> + LJCA_DIAG_GET_FW_COREDUMP,
> + LJCA_DIAG_TRIGGER_WDT,
> + LJCA_DIAG_TRIGGER_FAULT,
> + LJCA_DIAG_FEED_WDT,
> + LJCA_DIAG_GET_SECURE_STATE,
> +};
Should those really be enum? They just happen
to be nummerically dense.
> +static int ljca_parse(struct ljca_dev *dev, struct ljca_msg *header)
> +{
> + struct ljca_stub *stub;
> + unsigned int *ibuf_len;
> + u8 *ibuf;
> +
> + stub = ljca_stub_find(dev, header->type);
> + if (IS_ERR(stub))
> + return PTR_ERR(stub);
> +
> + if (!(header->flags & LJCA_ACK_FLAG)) {
> + ljca_stub_notify(stub, header->cmd, header->data, header->len);
> + return 0;
> + }
First you ack ...
> +
> + if (stub->cur_cmd != header->cmd) {
> + dev_err(&dev->intf->dev, "header and stub current command mismatch (%x vs %x)\n",
> + header->cmd, stub->cur_cmd);
> + return -EINVAL;
> + }
... then you check whether this is for the correct command?
> +
> + ibuf_len = READ_ONCE(stub->ipacket.ibuf_len);
> + ibuf = READ_ONCE(stub->ipacket.ibuf);
This races against stub_write(). Yes, every value now is consistent
within this function. But that does not solve the issue. The pair needs
to be consistent. You need to rule out that you are reading a different
length and buffer location. I am afraid this needs a spinlock.
> +
> + if (ibuf && ibuf_len) {
> + unsigned int newlen;
> +
> + newlen = min_t(unsigned int, header->len, *ibuf_len);
> +
> + *ibuf_len = newlen;
> + memcpy(ibuf, header->data, newlen);
> + }
> +
> + stub->acked = true;
> + wake_up(&dev->ack_wq);
> +
> + return 0;
> +}
> +
> +static int ljca_stub_write(struct ljca_stub *stub, u8 cmd, const void *obuf, unsigned int obuf_len,
> + void *ibuf, unsigned int *ibuf_len, bool wait_ack, unsigned long timeout)
> +{
> + struct ljca_dev *dev = usb_get_intfdata(stub->intf);
> + u8 flags = LJCA_CMPL_FLAG;
> + struct ljca_msg *header;
> + unsigned int msg_len = sizeof(*header) + obuf_len;
> + int actual;
> + int ret;
> +
> + if (msg_len > LJCA_MAX_PACKET_SIZE)
> + return -EINVAL;
> +
> + if (wait_ack)
> + flags |= LJCA_ACK_FLAG;
> +
> + header = kmalloc(msg_len, GFP_KERNEL);
> + if (!header)
> + return -ENOMEM;
Do you really want to first set a flag, then error out?
> + header->type = stub->type;
> + header->cmd = cmd;
> + header->flags = flags;
> + header->len = obuf_len;
> +
> + if (obuf)
> + memcpy(header->data, obuf, obuf_len);
> +
> + dev_dbg(&dev->intf->dev, "send: type:%d cmd:%d flags:%d len:%d\n", header->type,
> + header->cmd, header->flags, header->len);
> +
> + usb_autopm_get_interface(dev->intf);
> + if (!dev->started) {
> + kfree(header);
> + ret = -ENODEV;
Again, the flag remains set.
> + goto error_put;
> + }
> +
> + mutex_lock(&dev->mutex);
> + stub->cur_cmd = cmd;
> + stub->ipacket.ibuf = ibuf;
> + stub->ipacket.ibuf_len = ibuf_len;
> + stub->acked = false;
> + ret = usb_bulk_msg(interface_to_usbdev(dev->intf),
> + usb_sndbulkpipe(interface_to_usbdev(dev->intf), dev->out_ep), header,
> + msg_len, &actual, LJCA_USB_WRITE_TIMEOUT_MS);
> + kfree(header);
> + if (ret) {
> + dev_err(&dev->intf->dev, "bridge write failed ret:%d\n", ret);
> + goto error_unlock;
> + }
> +
> + if (actual != msg_len) {
> + dev_err(&dev->intf->dev, "bridge write length mismatch (%d vs %d)\n", msg_len,
> + actual);
> + ret = -EINVAL;
> + goto error_unlock;
> + }
> +
> + if (wait_ack) {
> + ret = wait_event_timeout(dev->ack_wq, stub->acked, msecs_to_jiffies(timeout));
> + if (!ret) {
> + dev_err(&dev->intf->dev, "acked wait timeout\n");
> + ret = -ETIMEDOUT;
> + goto error_unlock;
> + }
> + }
> +
> + ret = 0;
> +error_unlock:
> + stub->ipacket.ibuf = NULL;
> + stub->ipacket.ibuf_len = NULL;
> + mutex_unlock(&dev->mutex);
> +error_put:
> + usb_autopm_put_interface(dev->intf);
> +
> + return ret;
> +}
> +
[..]
> +static int ljca_start(struct ljca_dev *dev)
> +{
> + int ret;
> +
> + usb_fill_bulk_urb(dev->in_urb, interface_to_usbdev(dev->intf),
> + usb_rcvbulkpipe(interface_to_usbdev(dev->intf), dev->in_ep), dev->ibuf,
> + dev->ibuf_len, ljca_read_complete, dev);
> +
> + ret = usb_submit_urb(dev->in_urb, GFP_KERNEL);
> + if (ret) {
> + dev_err(&dev->intf->dev, "failed submitting read urb, error %d\n", ret);
> + return ret;
> + }
> +
> + mutex_lock(&dev->mutex);
> + dev->started = true;
> + mutex_unlock(&dev->mutex);
Why do you take a mutex here? Either this function cannot race with anything else,
or you set the flag too late.
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_ACPI
> +static void ljca_aux_dev_acpi_bind(struct ljca_dev *dev, struct auxiliary_device *auxdev,
> + unsigned int adr)
> +{
> + struct acpi_device *parent;
> + struct acpi_device *adev = NULL;
> +
> + /* new auxiliary device bind to acpi device */
> + parent = ACPI_COMPANION(&dev->intf->dev);
> + if (!parent)
> + return;
> +
> + adev = acpi_find_child_device(parent, adr, false);
> + ACPI_COMPANION_SET(&auxdev->dev, adev ?: parent);
> +}
> +#else
> +static void ljca_aux_dev_acpi_bind(struct ljca_dev *dev, struct auxiliary_device *auxdev,
> + unsigned int adr)
> +{
> +}
> +#endif
> +
> +static int ljca_add_aux_dev(struct ljca_dev *dev, char *name, u8 type, u8 id, u8 adr, void *data,
> + unsigned int len)
> +{
> + struct auxiliary_device *auxdev;
> + struct ljca *ljca;
> + int ret;
> +
> + if (dev->ljca_count >= ARRAY_SIZE(dev->ljcas))
> + return -EINVAL;
> +
> + ljca = kzalloc(sizeof(*ljca), GFP_KERNEL);
> + if (!ljca)
> + return -ENOMEM;
> +
> + ljca->type = type;
> + ljca->id = id;
> + ljca->dev = dev;
> +
> + auxdev = &ljca->auxdev;
> + auxdev->name = name;
> + auxdev->id = id;
> + auxdev->dev.platform_data = kmemdup(data, len, GFP_KERNEL);
> + if (!auxdev->dev.platform_data)
> + return -ENOMEM;
memory leak of struct ljca *ljca
> +
> + auxdev->dev.parent = &dev->intf->dev;
> + auxdev->dev.release = ljca_aux_release;
Regards
Oliver
Powered by blists - more mailing lists