netdev - Re: [PATCH] net: sched: taprio: Fix potential integer overflow in taprio_set_picos_per

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+h21hqtuGuJm0rMx_SZAy_HCjSVD_UK1j8wa7fv+p_zUGNV7A@mail.gmail.com>
Date:   Thu, 5 Sep 2019 23:47:13 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Vinicius Costa Gomes <vinicius.gomes@...el.com>
Cc:     Eric Dumazet <eric.dumazet@...il.com>,
        "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>,
        "David S. Miller" <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        lkml <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] net: sched: taprio: Fix potential integer overflow in taprio_set_picos_per_byte

Hi Vinicius,

On Wed, 4 Sep 2019 at 00:26, Vinicius Costa Gomes
<vinicius.gomes@...el.com> wrote:
>
> Hi,
>
> Vladimir Oltean <olteanv@...il.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