linux-kernel - Re: [PATCH] net: phy: micrel: Remove unnecessary comparison in lan8814_handle

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:   Thu, 5 May 2022 16:09:06 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Jiabing Wan <jiabing.wan@...mail.com>
Cc:     Jiabing Wan <wanjiabing@...o.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net: phy: micrel: Remove unnecessary comparison in
 lan8814_handle_interrupt

> I write the coccicheck and find these reports:

Nice!

> For directly call __phy_read():
> 
> ./drivers/net/phy/micrel.c:1969:59-60: WARNING: __phy_read() assigned to an
> unsigned int 'data'

This one we know about.

> ./drivers/net/phy/mscc/mscc_macsec.c:49:50-51: WARNING: __phy_read()
> assigned to an unsigned int 'val'
> ./drivers/net/phy/mscc/mscc_macsec.c:52:51-52: WARNING: __phy_read()
> assigned to an unsigned int 'val_l'
> ./drivers/net/phy/mscc/mscc_macsec.c:53:51-52: WARNING: __phy_read()
> assigned to an unsigned int 'val_h'
> ./drivers/net/phy/mscc/mscc_macsec.c:89:50-51: WARNING: __phy_read()
> assigned to an unsigned int 'val'

These are all in the same function. Looking at the first check, it has
been decided that if something goes wrong, return 0. Not the best
solution, but better than returning something random. So the do/while
loop can goto failed: val_l and val_h are a bit more messy, but a
check would be nice, return 0 on error.

Actually returning an error code is not going to be easy. The rest of
the code assumes vsc8584_macsec_phy_read() never fails :-(

> ./drivers/net/phy/mscc/mscc_main.c:1511:50-51: WARNING: __phy_read()
> assigned to an unsigned int 'addr'
> ./drivers/net/phy/mscc/mscc_main.c:1514:47-48: WARNING: __phy_read()
> assigned to an unsigned int 'val'

More code which assumes a read can never fail. vsc8514_probe() calls
this, so it should be reasonable easy to catch the error, return it,
and make the probe fail.

> ./drivers/net/phy/mscc/mscc_main.c:366:54-55: WARNING: __phy_read() assigned
> to an unsigned int 'reg_val'
> ./drivers/net/phy/mscc/mscc_main.c:370:55-56: WARNING: __phy_read() assigned
> to an unsigned int 'pwd [ 0 ]'
> ./drivers/net/phy/mscc/mscc_main.c:371:53-54: WARNING: __phy_read() assigned
> to an unsigned int 'pwd [ 1 ]'
> ./drivers/net/phy/mscc/mscc_main.c:372:55-56: WARNING: __phy_read() assigned
> to an unsigned int 'pwd [ 2 ]'
> ./drivers/net/phy/mscc/mscc_main.c:317:54-55: WARNING: __phy_read() assigned
> to an unsigned int 'reg_val'

Another void function which assumes it cannot fail. Error checks would
be nice, don't return random data in the wol structure.

Thanks
	Andrew