netdev - Re: [PATCH net-next 2/2] net: dsa: realtek: rtl8365mb: serialize indirect PHY register access

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] [day] [month] [year] [list]

Date:   Mon, 21 Feb 2022 21:18:17 -0300
From:   Luiz Angelo Daros de Luca <luizluca@...il.com>
To:     Alvin Šipraga <ALSI@...g-olufsen.dk>
Cc:     Vladimir Oltean <olteanv@...il.com>,
        Alvin Šipraga <alvin@...s.dk>,
        Linus Walleij <linus.walleij@...aro.org>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Arınç ÜNAL <arinc.unal@...nc9.com>,
        Michael Rasmussen <MIR@...g-olufsen.dk>,
        "open list:NETWORKING DRIVERS" <netdev@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next 2/2] net: dsa: realtek: rtl8365mb: serialize
 indirect PHY register access

> > The realtek "API/driver" does exactly how the driver was doing. They
> > do have a lock/unlock placeholder, but only in the equivalent
> > regmap_{read,write} functions. Indirect access does not use locks at
> > all (in fact, there is no other mention of "lock" elsewhere), even
> > being obvious that it is not thread-safe. It was just with a DSA
> > driver that we started to exercise register access for real, specially
> > without interruptions. And even in that case, we could only notice
> > this issue in multicore devices. I believe that, if they know about
> > this issue, they might not be worried because it has never affected a
> > real device. It would be very interesting to hear from Realtek but I
> > do not have the contacts.
>
> This is not true, at least with the sources I am reading. As I said in
> my reply to Vladimir, the Realtek code takes a lock around each
> top-level API call. Example:
>
> rtk_api_ret_t rtk_port_phyStatus_get(...)
> {
>     rtk_api_ret_t retVal;
>
>     if (NULL == RT_MAPPER->port_phyStatus_get)
>         return RT_ERR_DRIVER_NOT_FOUND;
>
>     RTK_API_LOCK();
>     retVal = RT_MAPPER->port_phyStatus_get(port, pLinkStatus, pSpeed, pDuplex);
>     RTK_API_UNLOCK();
>
>     return retVal;
> }
>
> Deep down in this port_phyStatus_get() callback, the indirect PHY
> register access takes place.

For the record, in the rtl8367c driver I'm using as reference, there
is no mention of RTK_API_LOCK(). Check here same function you copied:

https://github.com/openwrt/openwrt/blob/aae7af4219e56c2787f675109d9dd1a44a5dcba4/target/linux/mediatek/files-5.10/drivers/net/phy/rtk/rtl8367c/port.c#L1003-L1040

So, this indirect reg access protection is something they added along
the way, probably when they started to use it with SMP systems.

> Kind regards,
> Alvin