| 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: <eaf05cf1486c418790a1b54cbcda3a98@realtek.com>
Date: Fri, 20 Oct 2023 11:31:16 +0000
From: Hayes Wang <hayeswang@...ltek.com>
To: Douglas Anderson <dianders@...omium.org>,
Jakub Kicinski <kuba@...nel.org>,
"David S . Miller" <davem@...emloft.net>
CC: Grant Grundler <grundler@...omium.org>,
Edward Hill <ecgh@...omium.org>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
Simon Horman <horms@...nel.org>,
Laura Nao <laura.nao@...labora.com>,
Alan Stern <stern@...land.harvard.edu>,
Bjørn Mork <bjorn@...k.no>,
Eric Dumazet <edumazet@...gle.com>,
Paolo Abeni <pabeni@...hat.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH v4 5/5] r8152: Block future register access if register access fails
Douglas Anderson <dianders@...omium.org>
> Sent: Friday, October 20, 2023 5:20 AM
[...]
> static int generic_ocp_read(struct r8152 *tp, u16 index, u16 size,
> @@ -8265,6 +8353,17 @@ static int rtl8152_pre_reset(struct usb_interface *intf)
> if (!tp)
> return 0;
>
> + /* We can only use the optimized reset if we made it to the end of
> + * probe without any register access fails, which sets
> + * `PROBED_WITH_NO_ERRORS` to true. If we didn't have that then return
> + * an error here which tells the USB framework to fully unbind/rebind
> + * our driver.
> + */
> + if (!test_bit(PROBED_WITH_NO_ERRORS, &tp->flags)) {
> + mutex_unlock(&tp->control);
I think you forget to remove mutex_unlock here.
> + return -EIO;
> + }
> +
> netdev = tp->netdev;
> if (!netif_running(netdev))
> return 0;
> @@ -8277,7 +8376,9 @@ static int rtl8152_pre_reset(struct usb_interface *intf)
> napi_disable(&tp->napi);
> if (netif_carrier_ok(netdev)) {
> mutex_lock(&tp->control);
> + set_bit(IN_PRE_RESET, &tp->flags);
> tp->rtl_ops.disable(tp);
> + clear_bit(IN_PRE_RESET, &tp->flags);
> mutex_unlock(&tp->control);
> }
>
> @@ -8293,6 +8394,8 @@ static int rtl8152_post_reset(struct usb_interface *intf)
> if (!tp)
> return 0;
>
> + rtl_set_accessible(tp);
> +
Excuse me. I have a new idea. You could check if it is possible.
If you remove test_bit(PROBED_WITH_NO_ERRORS, &tp->flags) in pre_reset(),
the driver wouldn't be unbound and rebound. Instead, you test PROBED_WITH_NO_ERRORS
here to re-initialize the device. Then, you could limit the times of USB reset, and
the infinite loop wouldn't occur. The code would be like the following,
if (!test_bit(PROBED_WITH_NO_ERRORS, &tp->flags)) {
/* re-init */
mutex_lock(&tp->control);
tp->rtl_ops.init(tp);
mutex_unlock(&tp->control);
rtl_hw_phy_work_func_t(&tp->hw_phy_work.work);
/* re-open(). Maybe move after checking netif_running(netdev) */
mutex_lock(&tp->control);
tp->rtl_ops.up(tp);
mutex_unlock(&tp->control);
/* check if there is any control error */
if (test_bit(RTL8152_INACCESSIBLE, &tp->flags) {
if (tp->reg_access_reset_count < REGISTER_ACCESS_MAX_RESETS) {
/* queue reset again ? */
} else {
...
}
/* return 0 ? */
} else {
set_bit(PROBED_WITH_NO_ERRORS, &tp->flags)
}
}
Best Regards,
Hayes
> /* reset the MAC address in case of policy change */
> if (determine_ethernet_addr(tp, &sa) >= 0) {
> rtnl_lock();
> @@ -9494,17 +9597,35 @@ static u8 __rtl_get_hw_ver(struct usb_device *udev)
> __le32 *tmp;
> u8 version;
> int ret;
> + int i;
>
> tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
> if (!tmp)
> return 0;
>
> - ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
> - RTL8152_REQ_GET_REGS, RTL8152_REQT_READ,
> - PLA_TCR0, MCU_TYPE_PLA, tmp, sizeof(*tmp),
> - USB_CTRL_GET_TIMEOUT);
> - if (ret > 0)
> - ocp_data = (__le32_to_cpu(*tmp) >> 16) & VERSION_MASK;
> + /* Retry up to 3 times in case there is a transitory error. We do this
> + * since retrying a read of the version is always safe and this
> + * function doesn't take advantage of r8152_control_msg() which would
> + * queue up a reset upon error.
> + *
> + * NOTE: The fact that this read never queues up a reset prevents us
> + * from getting into a unbind/bind loop if usb_control_msg() fails
> + * 100% of the time. This is the first control message we do at
> + * probe time and 3 failures in a row here will cause probe to fail.
> + */
> + for (i = 0; i < 3; i++) {
> + ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
> + RTL8152_REQ_GET_REGS, RTL8152_REQT_READ,
> + PLA_TCR0, MCU_TYPE_PLA, tmp, sizeof(*tmp),
> + USB_CTRL_GET_TIMEOUT);
> + if (ret > 0) {
> + ocp_data = (__le32_to_cpu(*tmp) >> 16) & VERSION_MASK;
> + break;
> + }
> + }
> +
> + if (i != 0 && ret > 0)
> + dev_warn(&udev->dev, "Needed %d retries to read version\n", i);
>
> kfree(tmp);
>
> @@ -9784,7 +9905,29 @@ static int rtl8152_probe(struct usb_interface *intf,
> else
> device_set_wakeup_enable(&udev->dev, false);
>
> - netif_info(tp, probe, netdev, "%s\n", DRIVER_VERSION);
> + mutex_lock(&tp->control);
> + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) {
> + /* If the device is marked inaccessible before probe even
> + * finished then one of two things happened. Either we got a
> + * USB error during probe or the user already unplugged the
> + * device.
> + *
> + * If we got a USB error during probe then we skipped doing a
> + * reset in r8152_control_msg() and deferred it to here. This
> + * is because the queued reset will give up after 1 second
> + * (see usb_lock_device_for_reset()) and we want to make sure
> + * that we queue things up right before probe finishes.
> + *
> + * If the user already unplugged the device then the USB
> + * framework will call unbind right away for us. The extra
> + * reset we queue up here will be harmless.
> + */
> + usb_queue_reset_device(tp->intf);
> + } else {
> + set_bit(PROBED_WITH_NO_ERRORS, &tp->flags);
> + netif_info(tp, probe, netdev, "%s\n", DRIVER_VERSION);
> + }
> + mutex_unlock(&tp->control);
>
> return 0;
>
> --
> 2.42.0.758.gaed0368e0e-goog
Powered by blists - more mailing lists