netdev - Re: [PATCH 1/1] IB/mlx5: Add a signature check to received EQEs and CQEs

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, 11 Oct 2022 10:17:46 +0300
From:   Leon Romanovsky <leon@...nel.org>
To:     Rohit Nair <rohit.sajan.kumar@...cle.com>
Cc:     jgg@...pe.ca, saeedm@...dia.com, davem@...emloft.net,
        edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
        linux-rdma@...r.kernel.org, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org, manjunath.b.patil@...cle.com,
        rama.nichanamatlu@...cle.com,
        Michael Guralnik <michaelgur@...dia.com>
Subject: Re: [PATCH 1/1] IB/mlx5: Add a signature check to received EQEs and
 CQEs

There is no need to ping anyone, the patch is registered in patchworks
https://patchwork.kernel.org/project/linux-rdma/patch/20221005174521.63619-1-rohit.sajan.kumar@oracle.com/
and we will get to it.

You sent the patch during merge window, no wonder that none looked on it.

On Wed, Oct 05, 2022 at 10:45:20AM -0700, Rohit Nair wrote:
> As PRM defines, the bytewise XOR of the EQE and the EQE index should be
> 0xff. Otherwise, we can assume we have a corrupt EQE. The same is
> applicable to CQE as well.

I didn't find anything like this in my version of PRM.

> 
> Adding a check to verify the EQE and CQE is valid in that aspect and if
> not, dump the CQE and EQE to dmesg to be inspected.

While it is nice to see prints in dmesg, you need to explain why other
mechanisms (reporters, mlx5 events, e.t.c) are not enough.

> 
> This patch does not introduce any significant performance degradations
> and has been tested using qperf.

What does it mean? You made changes in kernel verbs flow, they are not
executed through qperf.

> 
> Suggested-by: Michael Guralnik <michaelgur@...dia.com>
> Signed-off-by: Rohit Nair <rohit.sajan.kumar@...cle.com>
> ---
>  drivers/infiniband/hw/mlx5/cq.c              | 40 ++++++++++++++++++++++++++++
>  drivers/net/ethernet/mellanox/mlx5/core/eq.c | 39 +++++++++++++++++++++++++++
>  2 files changed, 79 insertions(+)
> 
> diff --git a/drivers/infiniband/hw/mlx5/cq.c b/drivers/infiniband/hw/mlx5/cq.c
> index be189e0..2a6d722 100644
> --- a/drivers/infiniband/hw/mlx5/cq.c
> +++ b/drivers/infiniband/hw/mlx5/cq.c
> @@ -441,6 +441,44 @@ static void mlx5_ib_poll_sw_comp(struct mlx5_ib_cq *cq, int num_entries,
>  	}
>  }
>  
> +static void verify_cqe(struct mlx5_cqe64 *cqe64, struct mlx5_ib_cq *cq)
> +{
> +	int i = 0;
> +	u64 temp_xor = 0;
> +	struct mlx5_ib_dev *dev = to_mdev(cq->ibcq.device);
> +
> +	u32 cons_index = cq->mcq.cons_index;
> +	u64 *eight_byte_raw_cqe = (u64 *)cqe64;
> +	u8 *temp_bytewise_xor = (u8 *)(&temp_xor);
> +	u8 cqe_bytewise_xor = (cons_index & 0xff) ^
> +				((cons_index & 0xff00) >> 8) ^
> +				((cons_index & 0xff0000) >> 16);
> +
> +	for (i = 0; i < sizeof(struct mlx5_cqe64); i += 8) {
> +		temp_xor ^= *eight_byte_raw_cqe;
> +		eight_byte_raw_cqe++;
> +	}
> +
> +	for (i = 0; i < (sizeof(u64)); i++) {
> +		cqe_bytewise_xor ^= *temp_bytewise_xor;
> +		temp_bytewise_xor++;
> +	}
> +
> +	if (cqe_bytewise_xor == 0xff)
> +		return;
> +
> +	dev_err(&dev->mdev->pdev->dev,
> +		"Faulty CQE - checksum failure: cqe=0x%x cqn=0x%x cqe_bytewise_xor=0x%x\n",
> +		cq->ibcq.cqe, cq->mcq.cqn, cqe_bytewise_xor);
> +	dev_err(&dev->mdev->pdev->dev,
> +		"cons_index=%u arm_sn=%u irqn=%u cqe_size=0x%x\n",
> +		cq->mcq.cons_index, cq->mcq.arm_sn, cq->mcq.irqn, cq->mcq.cqe_sz);

mlx5_err ... and not dev_err ...

> +
> +	print_hex_dump(KERN_WARNING, "", DUMP_PREFIX_OFFSET,
> +		       16, 1, cqe64, sizeof(*cqe64), false);
> +	BUG();

No BUG() in new code.

Thanks