netdev - Re: [PATCH net 1/2] linux/dim: Fix overflow in dim calculation

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