[PATCH] net: sched: taprio: Fix potential integer overflow in taprio_set_picos_per_byte

[PATCH] net: sched: taprio: Fix potential integer overflow in taprio_set_picos_per_byte

[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 <gustavo@embeddedor.com>
---
 net/sched/sch_taprio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 8d8bc2ec5cd6..956f837436ea 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -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

Re: [PATCH] net: sched: taprio: Fix potential integer overflow in taprio_set_picos_per_byte

[Vladimir Oltean]

On Tue, 3 Sep 2019 at 04:08, Gustavo A. R. Silva <gustavo@embeddedor.com> wrote:
>
> 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 <gustavo@embeddedor.com>
> ---

Acked-by: Vladimir Oltean <olteanv@gmail.com>

>  net/sched/sch_taprio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index 8d8bc2ec5cd6..956f837436ea 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -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
>

Re: [PATCH] net: sched: taprio: Fix potential integer overflow in taprio_set_picos_per_byte

[Eric Dumazet]

On 9/3/19 3:08 AM, Gustavo A. R. Silva wrote:
> 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 <gustavo@embeddedor.com>
> ---
>  net/sched/sch_taprio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index 8d8bc2ec5cd6..956f837436ea 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -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.

->

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 8d8bc2ec5cd6281d811fd5d8a5c5211ebb0edd73..944b1af3215668e927d486b6c6c65c4599fb9539 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -965,8 +965,7 @@ static void taprio_set_picos_per_byte(struct net_device *dev,
                speed = ecmd.base.speed;
 
 skip:
-       picos_per_byte = div64_s64(NSEC_PER_SEC * 1000LL * 8,
-                                  speed * 1000 * 1000);
+       picos_per_byte = (USEC_PER_SEC * 8) / speed;
 
        atomic64_set(&q->picos_per_byte, picos_per_byte);
        netdev_dbg(dev, "taprio: set %s's picos_per_byte to: %lld, linkspeed: %d\n",

Re: [PATCH] net: sched: taprio: Fix potential integer overflow in taprio_set_picos_per_byte

[Vladimir Oltean]

On Tue, 3 Sep 2019 at 10:19, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>
> On 9/3/19 3:08 AM, Gustavo A. R. Silva wrote:
> > 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 <gustavo@embeddedor.com>
> > ---
> >  net/sched/sch_taprio.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> > index 8d8bc2ec5cd6..956f837436ea 100644
> > --- a/net/sched/sch_taprio.c
> > +++ b/net/sched/sch_taprio.c
> > @@ -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.
>
> ->
>
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index 8d8bc2ec5cd6281d811fd5d8a5c5211ebb0edd73..944b1af3215668e927d486b6c6c65c4599fb9539 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -965,8 +965,7 @@ static void taprio_set_picos_per_byte(struct net_device *dev,
>                 speed = ecmd.base.speed;
>
>  skip:
> -       picos_per_byte = div64_s64(NSEC_PER_SEC * 1000LL * 8,
> -                                  speed * 1000 * 1000);
> +       picos_per_byte = (USEC_PER_SEC * 8) / speed;
>
>         atomic64_set(&q->picos_per_byte, picos_per_byte);
>         netdev_dbg(dev, "taprio: set %s's picos_per_byte to: %lld, linkspeed: %d\n",
>
>
>

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".

Thanks,
-Vladimir

Re: [PATCH] net: sched: taprio: Fix potential integer overflow in taprio_set_picos_per_byte

[Vinicius Costa Gomes]

Hi,

Vladimir Oltean <olteanv@gmail.com> writes:

> 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)?

->

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 95991e43..d37c80b 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -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
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 6288e69..80e3db3 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -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;
+
+	if (settings->base.speed == 0)
+		return default_speed;
+
+	return settings->base.speed;
+}
+
 /* Internal kernel helper to query a device ethtool_link_settings. */
 int __ethtool_get_link_ksettings(struct net_device *dev,
 				 struct ethtool_link_ksettings *link_ksettings)

Re: [PATCH] net: sched: taprio: Fix potential integer overflow in taprio_set_picos_per_byte

[Vladimir Oltean]

Hi Vinicius,

On Wed, 4 Sep 2019 at 00:26, Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
>
> Hi,
>
> Vladimir Oltean <olteanv@gmail.com> writes:
>
> > 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)?
>
> ->
>
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 95991e43..d37c80b 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -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
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index 6288e69..80e3db3 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -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;
> +
> +       if (settings->base.speed == 0)
> +               return default_speed;
> +
> +       return settings->base.speed;
> +}
> +
>  /* Internal kernel helper to query a device ethtool_link_settings. */
>  int __ethtool_get_link_ksettings(struct net_device *dev,
>                                  struct ethtool_link_ksettings *link_ksettings)

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

RE: [PATCH] net: sched: taprio: Fix potential integer overflow in taprio_set_picos_per_byte

[Gomes, Vinicius]

Hi Vladimir,

> 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?

If you have the cycles to do it, go for it. I would only be able to work on this next week.

> 
> Thanks,
> -Vladimir

Thanks a lot,
--
Vinicius