netdev - Re: [PATCH net-next v3 1/3] rust: core abstractions for network PHY drivers

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: <72a268b1-aabc-4d98-aba4-4d92c3f3dd21@lunn.ch>
Date: Sat, 14 Oct 2023 23:55:48 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Benno Lossin <benno.lossin@...ton.me>
Cc: FUJITA Tomonori <fujita.tomonori@...il.com>, boqun.feng@...il.com,
	tmgross@...ch.edu, netdev@...r.kernel.org,
	rust-for-linux@...r.kernel.org, miguel.ojeda.sandonis@...il.com,
	greg@...ah.com
Subject: Re: [PATCH net-next v3 1/3] rust: core abstractions for network PHY
 drivers

> > The PHY driver asks the state from the abstractions then the
> > abstractions ask the state from PHYLIB. So when the abstractions get a
> > bad value from PHYLIB, the abstractions must return something to the
> > PHY driver. As I wrote, the abstractions return a random value or an
> > error. In either way, probably the system cannot continue.
> 
> Sure then let the system BUG if it cannot continue. I think that
> allowing UB is worse than BUGing.

There is nothing a PHY driver can do which justifies calling BUG().

BUG() indicates the system is totally messed up, and running further
is going to result in more file system corruption, causing more data
loss, so we need to stop the machine immediately.

Anyway, we are talking about this bit of code in the C driver:

        /* Reset PHY, otherwise MII_LPA will provide outdated information.
         * This issue is reproducible only with some link partner PHYs
         */
        if (phydev->state == PHY_NOLINK) {
                phy_init_hw(phydev);
                _phy_start_aneg(phydev);
        }

and what we should do if phydev->state is not one of the values
defined in enum phy_state, but is actually 42. The system will
continue, but it could be that the hardware reports the wrong value
for LPA, the Link Partner Advertisement. That is not critical
information, the link is likely to work, but the debug tool ethtool(1)
will report the wrong value.

Can we turn this UB into DB? I guess you can make the abstraction
accept any value, validate it against the values in enum phy_state, do
a WARN_ON_ONCE() if its not valid so we get a stack trace, and pass
the value on. Life will likely continue, hopefully somebody will
report the stack trace, and we can try to figure out what went wrong.

  Andrew