netdev - Re: [PATCH net v2 1/2] mlx5: fix possible ptp queue fifo overflow

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 23 Jan 2023 20:19:12 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Vadim Fedorenko <vfedorenko@...ek.ru>
Cc:     Vadim Fedorenko <vadfed@...com>, Aya Levin <ayal@...dia.com>,
        Saeed Mahameed <saeedm@...dia.com>,
        Gal Pressman <gal@...dia.com>,
        Vadim Fedorenko <vadfed@...a.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net v2 1/2] mlx5: fix possible ptp queue fifo overflow

On Tue, 24 Jan 2023 03:08:35 +0300 Vadim Fedorenko wrote:
> From: Vadim Fedorenko <vadfed@...a.com>
> 
> Fifo pointers are not checked for overflow and this could potentially
> lead to overflow and double free under heavy PTP traffic.
> 
> Also there were accidental OOO cqe which lead to absolutely broken fifo.
> Add checks to workaround OOO cqe and add counters to show the amount of
> such events.

May be worth adding a mention of the brokenness of the empty() check.
Comparing free running counters works well unless C promotes the types
to something wider than the counters themselves. So unsigned or u32
works, but comparing two u16s or u8s needs a explicit cast.

> -static void mlx5e_ptp_skb_fifo_ts_cqe_resync(struct mlx5e_ptpsq *ptpsq, u16 skb_cc, u16 skb_id)
> +static bool mlx5e_ptp_skb_fifo_ts_cqe_resync(struct mlx5e_ptpsq *ptpsq, u16 skb_cc, u16 skb_id)
>  {
>  	struct skb_shared_hwtstamps hwts = {};
>  	struct sk_buff *skb;
>  
>  	ptpsq->cq_stats->resync_event++;
>  
> -	while (skb_cc != skb_id) {
> -		skb = mlx5e_skb_fifo_pop(&ptpsq->skb_fifo);
> +	if (skb_cc > skb_id || PTP_WQE_CTR2IDX(ptpsq->skb_fifo_pc) < skb_id) {

Are you sure this works for all cases?
Directly comparing indexes of a ring buffer seems dangerous.
We'd need to compare like this:

	(s16)(skb_cc - skb_id) < 0

> +		ptpsq->cq_stats->ooo_cqe++;
> +		return false;
> +	}

> @@ -128,7 +141,8 @@ static void mlx5e_ptp_handle_ts_cqe(struct mlx5e_ptpsq *ptpsq,
>  	ptpsq->cq_stats->cqe++;
>  
>  out:
> -	napi_consume_skb(skb, budget);
> +	if (skb)
> +		napi_consume_skb(skb, budget);

I think napi_consume_skb() takes NULLs.