| 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:
<BN8PR15MB251384CC8D55FE475B1F85C099E82@BN8PR15MB2513.namprd15.prod.outlook.com>
Date: Fri, 31 Jan 2025 12:24:32 +0000
From: Bernard Metzler <BMT@...ich.ibm.com>
To: Eric Biggers <ebiggers@...nel.org>,
"linux-rdma@...r.kernel.org"
<linux-rdma@...r.kernel.org>,
Mustafa Ismail <mustafa.ismail@...el.com>,
Tatyana Nikolova <tatyana.e.nikolova@...el.com>,
Jason Gunthorpe
<jgg@...pe.ca>, Leon Romanovsky <leon@...nel.org>,
Zhu Yanjun
<zyjzyj2000@...il.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 5/6] RDMA/siw: fix type of CRC field
> -----Original Message-----
> From: Eric Biggers <ebiggers@...nel.org>
> Sent: Monday, January 27, 2025 11:39 PM
> To: linux-rdma@...r.kernel.org; Mustafa Ismail <mustafa.ismail@...el.com>;
> Tatyana Nikolova <tatyana.e.nikolova@...el.com>; Jason Gunthorpe
> <jgg@...pe.ca>; Leon Romanovsky <leon@...nel.org>; Zhu Yanjun
> <zyjzyj2000@...il.com>; Bernard Metzler <BMT@...ich.ibm.com>
> Cc: linux-kernel@...r.kernel.org
> Subject: [EXTERNAL] [PATCH 5/6] RDMA/siw: fix type of CRC field
>
> From: Eric Biggers <ebiggers@...gle.com>
>
Many thanks for making the correct CRC32c handling explicit.
> The usual big endian convention of InfiniBand does not apply to the MPA
'InfiniBand' is misleading here. Transmitting protocol header fields
in big endian order is a network protocol convention in general, so
it is often called network byte order. iWarp is not an InfiniBand
protocol but an IETF protocol. So in IMO, the specifics of CRC32c
network transmission would be better highlighted when comparing to
NBO in general.
> CRC field, whose transmission is specified in terms of the CRC32
> polynomial coefficients. The coefficients are transmitted in descending
> order from the x^31 coefficient to the x^0 one. When the result is
> interpreted as a 32-bit integer using the required reverse mapping
> between bits and polynomial coefficients, it's a __le32.
>
> Fix the code to use the correct type.
>
> The endianness conversion to little endian was actually already done in
> crypto_shash_final(). Therefore this patch does not change any
> behavior, except for adding the missing le32_to_cpu() to the pr_warn()
> message in siw_get_trailer() which makes the correct values be printed
> on big endian systems.
>
> Signed-off-by: Eric Biggers <ebiggers@...gle.com>
> ---
> drivers/infiniband/sw/siw/iwarp.h | 2 +-
> drivers/infiniband/sw/siw/siw.h | 8 ++++----
> drivers/infiniband/sw/siw/siw_qp.c | 2 +-
> drivers/infiniband/sw/siw/siw_qp_rx.c | 16 +++++++++++-----
> drivers/infiniband/sw/siw/siw_qp_tx.c | 2 +-
> 5 files changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/infiniband/sw/siw/iwarp.h
> b/drivers/infiniband/sw/siw/iwarp.h
> index 8cf69309827d6..afebb5d8643e3 100644
> --- a/drivers/infiniband/sw/siw/iwarp.h
> +++ b/drivers/infiniband/sw/siw/iwarp.h
> @@ -79,11 +79,11 @@ struct mpa_marker {
> /*
> * maximum MPA trailer
> */
> struct mpa_trailer {
> __u8 pad[4];
> - __be32 crc;
> + __le32 crc;
> };
>
> #define MPA_HDR_SIZE 2
> #define MPA_CRC_SIZE 4
>
> diff --git a/drivers/infiniband/sw/siw/siw.h
> b/drivers/infiniband/sw/siw/siw.h
> index ea5eee50dc39d..50649971f6254 100644
> --- a/drivers/infiniband/sw/siw/siw.h
> +++ b/drivers/infiniband/sw/siw/siw.h
> @@ -336,26 +336,26 @@ struct siw_rx_fpdu {
> * Shorthands for short packets w/o payload
> * to be transmitted more efficient.
> */
> struct siw_send_pkt {
> struct iwarp_send send;
> - __be32 crc;
> + __le32 crc;
> };
>
> struct siw_write_pkt {
> struct iwarp_rdma_write write;
> - __be32 crc;
> + __le32 crc;
> };
>
> struct siw_rreq_pkt {
> struct iwarp_rdma_rreq rreq;
> - __be32 crc;
> + __le32 crc;
> };
>
> struct siw_rresp_pkt {
> struct iwarp_rdma_rresp rresp;
> - __be32 crc;
> + __le32 crc;
> };
>
> struct siw_iwarp_tx {
> union {
> union iwarp_hdr hdr;
> diff --git a/drivers/infiniband/sw/siw/siw_qp.c
> b/drivers/infiniband/sw/siw/siw_qp.c
> index da92cfa2073d7..ea7d2f5c8b8ee 100644
> --- a/drivers/infiniband/sw/siw/siw_qp.c
> +++ b/drivers/infiniband/sw/siw/siw_qp.c
> @@ -394,11 +394,11 @@ void siw_send_terminate(struct siw_qp *qp)
> struct iwarp_terminate *term = NULL;
> union iwarp_hdr *err_hdr = NULL;
> struct socket *s = qp->attrs.sk;
> struct siw_rx_stream *srx = &qp->rx_stream;
> union iwarp_hdr *rx_hdr = &srx->hdr;
> - u32 crc = 0;
> + __le32 crc = 0;
> int num_frags, len_terminate, rv;
>
> if (!qp->term_info.valid)
> return;
>
> diff --git a/drivers/infiniband/sw/siw/siw_qp_rx.c
> b/drivers/infiniband/sw/siw/siw_qp_rx.c
> index ed4fc39718b49..098e32fb36fb1 100644
> --- a/drivers/infiniband/sw/siw/siw_qp_rx.c
> +++ b/drivers/infiniband/sw/siw/siw_qp_rx.c
> @@ -951,11 +951,11 @@ int siw_proc_terminate(struct siw_qp *qp)
> static int siw_get_trailer(struct siw_qp *qp, struct siw_rx_stream *srx)
> {
> struct sk_buff *skb = srx->skb;
> int avail = min(srx->skb_new, srx->fpdu_part_rem);
> u8 *tbuf = (u8 *)&srx->trailer.crc - srx->pad;
> - __wsum crc_in, crc_own = 0;
> + __le32 crc_in, crc_own = 0;
>
> siw_dbg_qp(qp, "expected %d, available %d, pad %u\n",
> srx->fpdu_part_rem, srx->skb_new, srx->pad);
>
> skb_copy_bits(skb, srx->skb_offset, tbuf, avail);
> @@ -969,20 +969,26 @@ static int siw_get_trailer(struct siw_qp *qp, struct
> siw_rx_stream *srx)
> if (!srx->mpa_crc_hd)
> return 0;
>
> if (srx->pad)
> crypto_shash_update(srx->mpa_crc_hd, tbuf, srx->pad);
> +
> /*
> - * CRC32 is computed, transmitted and received directly in NBO,
> - * so there's never a reason to convert byte order.
> + * The usual big endian convention of InfiniBand does not apply to
> the
Better not mention InfiniBand here, this is an IETF protocol on top
of TCP/SCTP. Just compare to NBO in general.
> + * CRC field, whose transmission is specified in terms of the CRC32
> + * polynomial coefficients. The coefficients are transmitted in
> + * descending order from the x^31 coefficient to the x^0 one. When
> the
> + * result is interpreted as a 32-bit integer using the required
> reverse
> + * mapping between bits and polynomial coefficients, it's a __le32.
> */
> crypto_shash_final(srx->mpa_crc_hd, (u8 *)&crc_own);
> - crc_in = (__force __wsum)srx->trailer.crc;
> + crc_in = srx->trailer.crc;
>
> if (unlikely(crc_in != crc_own)) {
> pr_warn("siw: crc error. in: %08x, own %08x, op %u\n",
> - crc_in, crc_own, qp->rx_stream.rdmap_op);
> + le32_to_cpu(crc_in), le32_to_cpu(crc_own),
> + qp->rx_stream.rdmap_op);
>
> siw_init_terminate(qp, TERM_ERROR_LAYER_LLP,
> LLP_ETYPE_MPA,
> LLP_ECODE_RECEIVED_CRC, 0);
> return -EINVAL;
> diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c
> b/drivers/infiniband/sw/siw/siw_qp_tx.c
> index a034264c56698..f9db69a82cdd5 100644
> --- a/drivers/infiniband/sw/siw/siw_qp_tx.c
> +++ b/drivers/infiniband/sw/siw/siw_qp_tx.c
> @@ -242,11 +242,11 @@ static int siw_qp_prepare_tx(struct siw_iwarp_tx
> *c_tx)
> else
> c_tx->pkt.c_tagged.ddp_to =
> cpu_to_be64(wqe->sqe.raddr);
> }
>
> - *(u32 *)crc = 0;
> + *(__le32 *)crc = 0;
> /*
> * Do complete CRC if enabled and short packet
> */
> if (c_tx->mpa_crc_hd &&
> crypto_shash_digest(c_tx->mpa_crc_hd, (u8 *)&c_tx->pkt,
> --
> 2.48.1
Reviewed-by: Bernard Metzler <bmt@...ich.ibm.com>
Powered by blists - more mailing lists