[PATCH net] net: pse-pd: tps23881: Fix boolean evaluation for bitmask checks

[PATCH net] net: pse-pd: tps23881: Fix boolean evaluation for bitmask checks

[Kory Maincent]

Fixed potential incorrect boolean evaluation when checking bitmask values.
The existing code assigned the result of bitwise operations directly to
boolean variables, which could lead to unexpected values.
This has been corrected by explicitly converting the results to booleans
using the !! operator.

Fixes: 20e6d190ffe1 ("net: pse-pd: Add TI TPS23881 PSE controller driver")
Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 drivers/net/pse-pd/tps23881.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/pse-pd/tps23881.c b/drivers/net/pse-pd/tps23881.c
index 5c4e88be46ee..1a57c55f8577 100644
--- a/drivers/net/pse-pd/tps23881.c
+++ b/drivers/net/pse-pd/tps23881.c
@@ -139,9 +139,9 @@ static int tps23881_pi_is_enabled(struct pse_controller_dev *pcdev, int id)
 
 	chan = priv->port[id].chan[0];
 	if (chan < 4)
-		enabled = ret & BIT(chan);
+		enabled = !!(ret & BIT(chan));
 	else
-		enabled = ret & BIT(chan + 4);
+		enabled = !!(ret & BIT(chan + 4));
 
 	if (priv->port[id].is_4p) {
 		chan = priv->port[id].chan[1];
@@ -172,11 +172,11 @@ static int tps23881_ethtool_get_status(struct pse_controller_dev *pcdev,
 
 	chan = priv->port[id].chan[0];
 	if (chan < 4) {
-		enabled = ret & BIT(chan);
-		delivering = ret & BIT(chan + 4);
+		enabled = !!(ret & BIT(chan));
+		delivering = !!(ret & BIT(chan + 4));
 	} else {
-		enabled = ret & BIT(chan + 4);
-		delivering = ret & BIT(chan + 8);
+		enabled = !!(ret & BIT(chan + 4));
+		delivering = !!(ret & BIT(chan + 8));
 	}
 
 	if (priv->port[id].is_4p) {
-- 
2.34.1

Re: [PATCH net] net: pse-pd: tps23881: Fix boolean evaluation for bitmask checks

[Oleksij Rempel]

On Mon, Sep 23, 2024 at 05:34:26PM +0200, Kory Maincent wrote:
> Fixed potential incorrect boolean evaluation when checking bitmask values.
> The existing code assigned the result of bitwise operations directly to
> boolean variables, which could lead to unexpected values.
> This has been corrected by explicitly converting the results to booleans
> using the !! operator.
> 
> Fixes: 20e6d190ffe1 ("net: pse-pd: Add TI TPS23881 PSE controller driver")
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
 
Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>

Thank you!
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net] net: pse-pd: tps23881: Fix boolean evaluation for bitmask checks

[Simon Horman]

On Mon, Sep 23, 2024 at 05:34:26PM +0200, Kory Maincent wrote:
> Fixed potential incorrect boolean evaluation when checking bitmask values.
> The existing code assigned the result of bitwise operations directly to
> boolean variables, which could lead to unexpected values.
> This has been corrected by explicitly converting the results to booleans
> using the !! operator.
> 
> Fixes: 20e6d190ffe1 ("net: pse-pd: Add TI TPS23881 PSE controller driver")
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>

Thanks Kory,

I agree that these changes are correct.
But are they fixes; can this manifest in a bug?
(If so, I suspect the Kernel is riddled with such bugs.)

...

Re: [PATCH net] net: pse-pd: tps23881: Fix boolean evaluation for bitmask checks

[Kory Maincent]

On Tue, 24 Sep 2024 08:18:39 +0100
Simon Horman <horms@kernel.org> wrote:

> On Mon, Sep 23, 2024 at 05:34:26PM +0200, Kory Maincent wrote:
>  [...]  
> 
> Thanks Kory,
> 
> I agree that these changes are correct.
> But are they fixes; can this manifest in a bug?

I didn't face it but I think yes.
In case of a 4 pairs PoE ports without the fix:

        chan = priv->port[id].chan[0];                                          
        if (chan < 4) {                                                         
                enabled = ret & BIT(chan);                                  
                delivering = ret & BIT(chan + 4);                           
	...                          
        }                                                                       
                                                                                
        if (priv->port[id].is_4p) {                                             
                chan = priv->port[id].chan[1];                                  
                if (chan < 4) {
                        enabled &= !!(ret & BIT(chan));                         
                        delivering &= !!(ret & BIT(chan + 4));

If enabled = 0x2 here, enabled would be assigned to 0 instead of 1.
...

		} 
	}  

I have an issue using 4pairs PoE port with my board so I can't test it.


> (If so, I suspect the Kernel is riddled with such bugs.)

Don't know about it but if I can remove it from my driver it would be nice. :)

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net] net: pse-pd: tps23881: Fix boolean evaluation for bitmask checks

[Simon Horman]

On Tue, Sep 24, 2024 at 10:15:29AM +0200, Kory Maincent wrote:
> On Tue, 24 Sep 2024 08:18:39 +0100
> Simon Horman <horms@kernel.org> wrote:
> 
> > On Mon, Sep 23, 2024 at 05:34:26PM +0200, Kory Maincent wrote:
> >  [...]  
> > 
> > Thanks Kory,
> > 
> > I agree that these changes are correct.
> > But are they fixes; can this manifest in a bug?
> 
> I didn't face it but I think yes.
> In case of a 4 pairs PoE ports without the fix:
> 
>         chan = priv->port[id].chan[0];                                          
>         if (chan < 4) {                                                         
>                 enabled = ret & BIT(chan);                                  
>                 delivering = ret & BIT(chan + 4);                           
> 	...                          
>         }                                                                       
>                                                                                 
>         if (priv->port[id].is_4p) {                                             
>                 chan = priv->port[id].chan[1];                                  
>                 if (chan < 4) {
>                         enabled &= !!(ret & BIT(chan));                         
>                         delivering &= !!(ret & BIT(chan + 4));
> 
> If enabled = 0x2 here, enabled would be assigned to 0 instead of 1.
> ...
> 
> 		} 
> 	}  
> 
> I have an issue using 4pairs PoE port with my board so I can't test it.
> 
> 
> > (If so, I suspect the Kernel is riddled with such bugs.)
> 
> Don't know about it but if I can remove it from my driver it would be nice. :)

Right, no question from my side that this change is a good one.
I'm just wondering if it is best for net or net-next.

Re: [PATCH net] net: pse-pd: tps23881: Fix boolean evaluation for bitmask checks

[Kory Maincent]

On Tue, 24 Sep 2024 09:26:12 +0100
Simon Horman <horms@kernel.org> wrote:

> > Don't know about it but if I can remove it from my driver it would be nice.
> > :)  
> 
> Right, no question from my side that this change is a good one.
> I'm just wondering if it is best for net or net-next.

Indeed, I don't know the policy on this. Do you think it shouldn't go to net?
I will let net maintainers decide. ;)

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net] net: pse-pd: tps23881: Fix boolean evaluation for bitmask checks

[Oleksij Rempel]

On Tue, Sep 24, 2024 at 10:33:57AM +0200, Kory Maincent wrote:
> On Tue, 24 Sep 2024 09:26:12 +0100
> Simon Horman <horms@kernel.org> wrote:
> 
> > > Don't know about it but if I can remove it from my driver it would be nice.
> > > :)  
> > 
> > Right, no question from my side that this change is a good one.
> > I'm just wondering if it is best for net or net-next.
> 
> Indeed, I don't know the policy on this. Do you think it shouldn't go to net?
> I will let net maintainers decide. ;)

The net is always the right place for this kind of fixes. In any case, it
would be better to have actual symptoms of the issue addressed by
this patch in the commit message.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net] net: pse-pd: tps23881: Fix boolean evaluation for bitmask checks

[Simon Horman]

On Tue, Sep 24, 2024 at 10:44:06AM +0200, Oleksij Rempel wrote:
> On Tue, Sep 24, 2024 at 10:33:57AM +0200, Kory Maincent wrote:
> > On Tue, 24 Sep 2024 09:26:12 +0100
> > Simon Horman <horms@kernel.org> wrote:
> > 
> > > > Don't know about it but if I can remove it from my driver it would be nice.
> > > > :)  
> > > 
> > > Right, no question from my side that this change is a good one.
> > > I'm just wondering if it is best for net or net-next.
> > 
> > Indeed, I don't know the policy on this. Do you think it shouldn't go to net?
> > I will let net maintainers decide. ;)
> 
> The net is always the right place for this kind of fixes. In any case, it
> would be better to have actual symptoms of the issue addressed by
> this patch in the commit message.

Thanks, I think that would address my questions about this patch.