netdev - RE: [PATCH v3 5/5] r8152: Block future register access if register access fails

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: <29f9a2ff1979406489213909b940184f@realtek.com>
Date: Mon, 16 Oct 2023 09:15:51 +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: Alan Stern <stern@...land.harvard.edu>, Simon Horman <horms@...nel.org>,
        Edward Hill <ecgh@...omium.org>, Laura Nao <laura.nao@...labora.com>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        Grant Grundler
	<grundler@...omium.org>,
        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 v3 5/5] r8152: Block future register access if register access fails

Douglas Anderson <dianders@...omium.org>
> Sent: Friday, October 13, 2023 3:25 AM
[...]
>  static int generic_ocp_read(struct r8152 *tp, u16 index, u16 size,
> @@ -8265,6 +8353,19 @@ 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.

Would you stay in a loop of unbind and rebind,
if the control transfers in the probe() are not always successful?
I just think about the worst case that at least one control always fails in probe().

> +        */
> +       mutex_lock(&tp->control);

I don't think you need the mutex for testing the bit.

> +       if (!test_bit(PROBED_WITH_NO_ERRORS, &tp->flags)) {
> +               mutex_unlock(&tp->control);
> +               return -EIO;
> +       }
> +       mutex_unlock(&tp->control);
> +
>         netdev = tp->netdev;
>         if (!netif_running(netdev))
>                 return 0;
> @@ -8277,7 +8378,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 +8396,10 @@ static int rtl8152_post_reset(struct usb_interface
> *intf)
>         if (!tp)
>                 return 0;
> 
> +       mutex_lock(&tp->control);

I don't think clear_bit() needs the protection of mutex.
I think you could call rtl_set_accessible() directly.

> +       rtl_set_accessible(tp);
> +       mutex_unlock(&tp->control);
> +
>         /* reset the MAC address in case of policy change */
>         if (determine_ethernet_addr(tp, &sa) >= 0) {
>                 rtnl_lock();

Best Regards,
Hayes