net: sched: taprio: Fix potential integer overflow in taprio_set_picos_per_byte

Add suffix LL to constant 1000 in order to avoid a potential integer overflow and give the compiler complete information about the proper arithmetic to use. Notice that this constant is being used in a context that expects an expression of type s64, but it's currently evaluated using 32-bit arithmetic.

Addresses-Coverity-ID: 1453459 ("Unintentional integer overflow")

Fixes f04b514c0ce2 ("taprio: Set default link speed to 10 Mbps in taprio_set_picos_per_byte")

Signed-off-by Gustavo A. R. Silva <[email protected]>

---

 net/sched/sch_taprio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

net/sched/sch_taprio.c +1 -2

index 8d8bc2ec5cd6..956f837436ea 100644
@@ -966,7 +966,7 @@ static void taprio_set_picos_per_byte(struct net_device *dev,
 
 skip:
        picos_per_byte = div64_s64(NSEC_PER_SEC * 1000LL * 8,
-                                  speed * 1000 * 1000);
+                                  speed * 1000LL * 1000);
 
        atomic64_set(&q->picos_per_byte, picos_per_byte);
        netdev_dbg(dev, "taprio: set %s's picos_per_byte to: %lld, linkspeed: %d\n",
--

2.23.0

Add suffix LL to constant 1000 in order to avoid a potential integer overflow and give the compiler complete information about the proper arithmetic to use. Notice that this constant is being used in a context that expects an expression of type s64, but it's currently evaluated using 32-bit arithmetic.

Addresses-Coverity-ID: 1453459 ("Unintentional integer overflow")

Fixes f04b514c0ce2 ("taprio: Set default link speed to 10 Mbps in taprio_set_picos_per_byte")

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
 net/sched/sch_taprio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

net/sched/sch_taprio.c +0 -0

index 8d8bc2ec5cd6..956f837436ea 100644

Quoted 19 lines · Gustavo A. R. Silva

Add suffix LL to constant 1000 in order to avoid a potential integer overflow and give the compiler complete information about the proper arithmetic to use. Notice that this constant is being used in a context that expects an expression of type s64, but it's currently evaluated using 32-bit arithmetic.

Addresses-Coverity-ID: 1453459 ("Unintentional integer overflow")

Fixes f04b514c0ce2 ("taprio: Set default link speed to 10 Mbps in taprio_set_picos_per_byte")

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
 net/sched/sch_taprio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

net/sched/sch_taprio.c +0 -0

index 8d8bc2ec5cd6..956f837436ea 100644

net/sched/sch_taprio.c static void taprio_set_picos_per_byte(struct net_device *dev, · line 972

Quoted 8 lines

net/sched/sch_taprio.c +1 -1

@@ -966,7 +966,7 @@ static void taprio_set_picos_per_byte(struct net_device *dev,
 
 skip:
      picos_per_byte = div64_s64(NSEC_PER_SEC * 1000LL * 8,
-                                speed * 1000 * 1000);
+                                speed * 1000LL * 1000);
 
      atomic64_set(&q->picos_per_byte, picos_per_byte);
      netdev_dbg(dev, "taprio: set %s's picos_per_byte to: %lld, linkspeed: %d\n",

But, why even multiplying by 1,000,000 in the first place, this seems silly, a standard 32 bit divide could be used instead.

->

net/sched/sch_taprio.c +0 -0

index 8d8bc2ec5cd6281d811fd5d8a5c5211ebb0edd73..944b1af3215668e927d486b6c6c65c4599fb9539 100644

Right. And while we're at it, there's still the potential division-by-zero problem which I still don't know how to solve without implementing a full-blown __ethtool_get_link_ksettings parser that checks against all the possible outputs it can have under the "no carrier" condition - see "[RFC PATCH 1/1] phylink: Set speed to SPEED_UNKNOWN when there is no PHY connected" for details. And there's also a third fix to be made: the netdev_dbg should be made to print "speed" instead of "ecmd.base.speed".

Hi,

Quoted 8 lines · Vladimir Oltean

Right. And while we're at it, there's still the potential division-by-zero problem which I still don't know how to solve without implementing a full-blown __ethtool_get_link_ksettings parser that checks against all the possible outputs it can have under the "no carrier" condition - see "[RFC PATCH 1/1] phylink: Set speed to SPEED_UNKNOWN when there is no PHY connected" for details. And there's also a third fix to be made: the netdev_dbg should be made to print "speed" instead of "ecmd.base.speed".

For the ksettings part I am thinking on adding something like this to ethtool.c. Do you think anything is missing (apart from the documentation)?

->

include/linux/ethtool.h +3 -0

index 95991e43..d37c80b 100644
@@ -177,6 +177,9 @@ void ethtool_convert_legacy_u32_to_link_mode(unsigned long *dst,
 bool ethtool_convert_link_mode_to_legacy_u32(u32 *legacy_u32,
                                     const unsigned long *src);
 
+u32 ethtool_link_ksettings_to_speed(const struct ethtool_link_ksettings *settings,
+                                   u32 default_speed);
+
 /**
  * struct ethtool_ops - optional netdev operations
  * @get_drvinfo: Report driver/device information.  Should only set the

net/core/ethtool.c +5 -0

index 6288e69..80e3db3 100644
@@ -539,6 +539,18 @@ struct ethtool_link_usettings {
        } link_modes;
 };
 
+u32 ethtool_link_ksettings_to_speed(const struct ethtool_link_ksettings *settings,
+                                  u32 default_speed)
+{
+       if (settings->base.speed == SPEED_UNKNOWN)
+               return default_speed;

Looks ok to me, but I have no saying over ethtool API. Actually I don't even know whom to ask - the output of ./scripts/get_maintainer.pl net/core/ethtool.c is a bit overwhelming.
To avoid conflicts, there needs to be somebody out of us who takes Eric's simplification, with Gustavo's Reported-by tag, and the 2 ethtool & taprio patches to avoid division by zero, and the printing fix, and maybe do the same in cbs. Will you be the one? Should I?

Thanks,
-Vladimir