| 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
| ||
|
Message-ID: <4f4bc2958dc1512087f19db64e8e43f1247cf2dd.camel@mellanox.com>
Date: Tue, 23 Jul 2019 17:22:43 +0000
From: Saeed Mahameed <saeedm@...lanox.com>
To: "davem@...emloft.net" <davem@...emloft.net>,
"leon@...nel.org" <leon@...nel.org>
CC: Jason Gunthorpe <jgg@...lanox.com>,
Yamin Friedman <yaminf@...lanox.com>,
"linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
Tal Gilboa <talgi@...lanox.com>,
Leon Romanovsky <leonro@...lanox.com>,
"dledford@...hat.com" <dledford@...hat.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net 1/2] linux/dim: Fix overflow in dim calculation
On Tue, 2019-07-23 at 10:22 +0300, Leon Romanovsky wrote:
> From: Yamin Friedman <yaminf@...lanox.com>
>
> While using net_dim, a dim_sample was used without ever initializing
> the
> comps value. Added use of DIV_ROUND_DOWN_ULL() to prevent potential
> overflow, it should not be a problem to save the final result in an
> int
> because after the division by epms the value should not be larger
> than a
> few thousand.
>
> [ 1040.127124] UBSAN: Undefined behaviour in lib/dim/dim.c:78:23
> [ 1040.130118] signed integer overflow:
> [ 1040.131643] 134718714 * 100 cannot be represented in type 'int'
>
> Fixes: 398c2b05bbee ("linux/dim: Add completions count to
> dim_sample")
> Signed-off-by: Yamin Friedman <yaminf@...lanox.com>
> Signed-off-by: Leon Romanovsky <leonro@...lanox.com>
> ---
> drivers/net/ethernet/broadcom/bcmsysport.c | 2 +-
> drivers/net/ethernet/broadcom/bnxt/bnxt.c | 2 +-
> drivers/net/ethernet/broadcom/genet/bcmgenet.c | 2 +-
> drivers/net/ethernet/mellanox/mlx5/core/en_txrx.c | 4 ++--
> lib/dim/dim.c | 4 ++--
> 5 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c
> b/drivers/net/ethernet/broadcom/bcmsysport.c
> index b9c5cea8db16..9483553ce444 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -992,7 +992,7 @@ static int bcm_sysport_poll(struct napi_struct
> *napi, int budget)
> {
> struct bcm_sysport_priv *priv =
> container_of(napi, struct bcm_sysport_priv, napi);
> - struct dim_sample dim_sample;
> + struct dim_sample dim_sample = {};
net_dim implementation doesn't care about sample->comp_ctr, so this is
unnecessary for the sake of fixing the rdma overflow issue, but it
doens't hurt anyone to have this change in this patch, although Tariq
already sent me a fix that i applied to my internal queues.
> unsigned int work_done = 0;
>
> work_done = bcm_sysport_desc_rx(priv, budget);
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 7134d2c3eb1c..7070349915bc 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -2136,7 +2136,7 @@ static int bnxt_poll(struct napi_struct *napi,
> int budget)
> }
> }
> if (bp->flags & BNXT_FLAG_DIM) {
> - struct dim_sample dim_sample;
> + struct dim_sample dim_sample = {};
>
> dim_update_sample(cpr->event_ctr,
> cpr->rx_packets,
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index a2b57807453b..d3a0b614dbfa 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -1895,7 +1895,7 @@ static int bcmgenet_rx_poll(struct napi_struct
> *napi, int budget)
> {
> struct bcmgenet_rx_ring *ring = container_of(napi,
> struct bcmgenet_rx_ring, napi);
> - struct dim_sample dim_sample;
> + struct dim_sample dim_sample = {};
> unsigned int work_done;
>
> work_done = bcmgenet_desc_rx(ring, budget);
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_txrx.c
> b/drivers/net/ethernet/mellanox/mlx5/core/en_txrx.c
> index c50b6f0769c8..49b06b256c92 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_txrx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_txrx.c
> @@ -49,7 +49,7 @@ static inline bool
> mlx5e_channel_no_affinity_change(struct mlx5e_channel *c)
> static void mlx5e_handle_tx_dim(struct mlx5e_txqsq *sq)
> {
> struct mlx5e_sq_stats *stats = sq->stats;
> - struct dim_sample dim_sample;
> + struct dim_sample dim_sample = {};
>
> if (unlikely(!test_bit(MLX5E_SQ_STATE_AM, &sq->state)))
> return;
> @@ -61,7 +61,7 @@ static void mlx5e_handle_tx_dim(struct mlx5e_txqsq
> *sq)
> static void mlx5e_handle_rx_dim(struct mlx5e_rq *rq)
> {
> struct mlx5e_rq_stats *stats = rq->stats;
> - struct dim_sample dim_sample;
> + struct dim_sample dim_sample = {};
>
> if (unlikely(!test_bit(MLX5E_RQ_STATE_AM, &rq->state)))
> return;
> diff --git a/lib/dim/dim.c b/lib/dim/dim.c
> index 439d641ec796..38045d6d0538 100644
> --- a/lib/dim/dim.c
> +++ b/lib/dim/dim.c
> @@ -74,8 +74,8 @@ void dim_calc_stats(struct dim_sample *start,
> struct dim_sample *end,
> delta_us);
> curr_stats->cpms = DIV_ROUND_UP(ncomps * USEC_PER_MSEC,
> delta_us);
> if (curr_stats->epms != 0)
BTW unrelated to this changed but curr_stats->epms can never be 0 due
to DIV_ROUND_UP. we can save a condition here.
> - curr_stats->cpe_ratio =
> - (curr_stats->cpms * 100) / curr_stats-
> >epms;
> + curr_stats->cpe_ratio = DIV_ROUND_DOWN_ULL(
> + curr_stats->cpms * 100, curr_stats->epms);
> else
> curr_stats->cpe_ratio = 0;
>
LGTM,
Acked-by: Saeed Mahameed <saeedm@...lanox.com>
> --
> 2.20.1
>
Powered by blists - more mailing lists