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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 5 May 2022 14:13:08 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Wan Jiabing <wanjiabing@...o.com>
Cc:     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

On Thu, May 05, 2022 at 11:02:17AM +0800, Wan Jiabing wrote:
> Fix following coccicheck warning:
> ./drivers/net/phy/micrel.c:2679:6-20: WARNING: Unsigned expression compared with zero: tsu_irq_status > 0

There are at least two different possibilities here:

As you say, the comparison is pointless, in which case, it can be
removed.

The code author really did have something in mind here, the comparison
is correct, but there is another bug.

I would generally assume the second, and try to first find the other
bug. If that bug really exists, removing the comparisons just adds one
bug on top of another.

So, check the return type of lanphy_read_page_reg(). It is int. If you
dig down, you get to __phy_read(), which calls __mdiobus_read(), all
of which return int. All these functions return a negative error code,
or a positive register value.

So the real problem here is, tsu_irq_status is defined as u16, when in
fact it should be an int.

As a result, a negative error code is going to get cast positive, and
then used as the value of the interrupt register. The code author
wanted to avoid this, so added a comparison. In an interrupt handler
you cannot actually return an error code, so the safe thing to do is
ignore it.

Please consider coccicheck just a hint, there is something wrong
somewhere around here. You then need to really investigate and figure
out what the real issue is, which might be exactly what coccicheck
says, but more likely it is something else.

NACK

   Andrew