netdev - Re: [PATCH net] net: phy: fix a signedness bug in genphy

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: Tue, 30 May 2023 22:28:07 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Andrew Lunn <andrew@...n.ch>
Cc: Jakub Kicinski <kuba@...nel.org>,
	Dan Carpenter <dan.carpenter@...aro.org>,
	Oleksij Rempel <linux@...pel-privat.de>,
	Heiner Kallweit <hkallweit1@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH net] net: phy: fix a signedness bug in genphy_loopback()

On Tue, May 30, 2023 at 10:09:24PM +0100, Russell King (Oracle) wrote:
> Having thought about this, the best I can come up with is this, which
> I think gives us everything we want without needing BUILD_BUG_ONs:
> 
> #define phy_read_poll_timeout(phydev, regnum, val, cond, sleep_us, \
>                                 timeout_us, sleep_before_read) \
> ({ \
>         int __ret, __val;
> 	__ret = read_poll_timeout(__val = phy_read, val, __val < 0 || (cond), \
>                 sleep_us, timeout_us, sleep_before_read, phydev, regnum); \
>         if (__val < 0) \
>                 __ret = __val; \
>         if (__ret) \
>                 phydev_err(phydev, "%s failed: %d\n", __func__, __ret); \
>         __ret; \
> })
> 
> This looks rather horrid, but what it essentially does is:
> 
>                 (val) = op(args); \
>                 if (cond) \
>                         break; \
> 
> expands to:
> 
> 		(val) = __val = phy_read(args);
> 		if (__val < 0 || (cond))
> 			break;
> 
> As phy_read() returns an int, there is no cast or loss assigning it
> to __val, since that is also an int. The conversion from int to
> something else happens at the same point it always has.

... and actually produces nicer code on 32-bit ARM:

Old (with the u16 val changed to an int val):

 2f8:   ebfffffe        bl      0 <mdiobus_read>
 2fc:   e7e03150        ubfx    r3, r0, #2, #1		extract bit 2 into r3
 300:   e1a04000        mov     r4, r0			save return value
 304:   e2002004        and     r2, r0, #4		extract bit 2 again
 308:   e1933fa0        orrs    r3, r3, r0, lsr #31	grab sign bit
 30c:   1a00000d        bne     348 <genphy_loopback+0xd8>
		breaks out of loop if r3 is nonzero
	... rest of loop ...
...
 348:   e3520000        cmp     r2, #0
 34c:   0a00000b        beq     380 <genphy_loopback+0x110>
		basically tests whether bit 2 was zero, and jumps if it
		was. Basically (cond) is false.

 350:   e3540000        cmp     r4, #0
 354:   a3a04000        movge   r4, #0
 358:   ba00000a        blt     388 <genphy_loopback+0x118>
		tests whether a phy_read returned an error and jumps
		if it did. r4 is basically __ret.
...

 380:   e3540000        cmp     r4, #0
 384:   a3e0406d        mvnge   r4, #109        ; 0x6d
		if r4 (__ret) was >= 0, sets an error code (-ETIMEDOUT).
 388:   e1a03004        mov     r3, r4
 ... dev_err() bit.

The new generated code is:

 2f8:   ebfffffe        bl      0 <mdiobus_read>
                        2f8: R_ARM_CALL mdiobus_read
 2fc:   e2504000        subs    r4, r0, #0		__val assignment
 300:   ba000014        blt     358 <genphy_loopback+0xe8>
		if <0, go direct to dev_err code
 304:   e3140004        tst     r4, #4			cond test within loop
 308:   1a00000d        bne     344 <genphy_loopback+0xd4>
	... rest of loop ...

 344:   e6ff4074        uxth    r4, r4			cast to 16-bit uint
 348:   e3140004        tst     r4, #4			test
 34c:   13a04000        movne   r4, #0			__ret is zero if bit set
 350:   1a000007        bne     374 <genphy_loopback+0x104> basically returns
 354:   e3e0406d        mvn     r4, #109        ; 0x6d
	... otherwise sets __ret to -ETIMEDOUT
	... dev_err() code

Is there a reason why it was written (cond) || val < 0 rather than
val < 0 || (cond) ? Note that the order of these tests makes no
difference in this situation, but I'm wondering whether it was
intentional?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!