2021-02-26 07:58:31

by Dinghao Liu

[permalink] [raw]
Subject: [PATCH] RDMA/siw: Fix missing check in siw_get_hdr

We should also check the range of opcode after calling
__rdmap_get_opcode() in the else branch to prevent potential
overflow.

Fixes: 8b6a361b8c482 ("rdma/siw: receive path")
Signed-off-by: Dinghao Liu <[email protected]>
---
drivers/infiniband/sw/siw/siw_qp_rx.c | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/drivers/infiniband/sw/siw/siw_qp_rx.c b/drivers/infiniband/sw/siw/siw_qp_rx.c
index 60116f20653c..301e7fe2c61a 100644
--- a/drivers/infiniband/sw/siw/siw_qp_rx.c
+++ b/drivers/infiniband/sw/siw/siw_qp_rx.c
@@ -1072,6 +1072,16 @@ static int siw_get_hdr(struct siw_rx_stream *srx)
siw_dbg_qp(rx_qp(srx), "new header, opcode %u\n", opcode);
} else {
opcode = __rdmap_get_opcode(c_hdr);
+
+ if (opcode > RDMAP_TERMINATE) {
+ pr_warn("siw: received unknown packet type %u\n",
+ opcode);
+
+ siw_init_terminate(rx_qp(srx), TERM_ERROR_LAYER_RDMAP,
+ RDMAP_ETYPE_REMOTE_OPERATION,
+ RDMAP_ECODE_OPCODE, 0);
+ return -EINVAL;
+ }
}
set_rx_fpdu_context(qp, opcode);
frx = qp->rx_fpdu;
--
2.17.1


2021-02-26 09:23:12

by Bernard Metzler

[permalink] [raw]
Subject: Re: [PATCH] RDMA/siw: Fix missing check in siw_get_hdr

-----"Dinghao Liu" <[email protected]> wrote: -----

>To: [email protected], [email protected]
>From: "Dinghao Liu" <[email protected]>
>Date: 02/26/2021 08:56AM
>Cc: "Bernard Metzler" <[email protected]>, "Doug Ledford"
><[email protected]>, "Jason Gunthorpe" <[email protected]>,
>[email protected], [email protected]
>Subject: [EXTERNAL] [PATCH] RDMA/siw: Fix missing check in
>siw_get_hdr
>
>We should also check the range of opcode after calling
>__rdmap_get_opcode() in the else branch to prevent potential
>overflow.

Hi Dinghao,
No this is not needed. We always first read the minimum
header information (MPA len, DDP flags, RDMAP opcode,
STag, target offset). Only if we have received that
into local buffer, we check for the opcode this one time.
Now the opcode determines the remaining length of the
variably sized part of the header to be received.

We do not have to check the opcode again, since we
already received and checked it.

Best,
Bernard.

>
>Fixes: 8b6a361b8c482 ("rdma/siw: receive path")
>Signed-off-by: Dinghao Liu <[email protected]>
>---
> drivers/infiniband/sw/siw/siw_qp_rx.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
>diff --git a/drivers/infiniband/sw/siw/siw_qp_rx.c
>b/drivers/infiniband/sw/siw/siw_qp_rx.c
>index 60116f20653c..301e7fe2c61a 100644
>--- a/drivers/infiniband/sw/siw/siw_qp_rx.c
>+++ b/drivers/infiniband/sw/siw/siw_qp_rx.c
>@@ -1072,6 +1072,16 @@ static int siw_get_hdr(struct siw_rx_stream
>*srx)
> siw_dbg_qp(rx_qp(srx), "new header, opcode %u\n", opcode);
> } else {
> opcode = __rdmap_get_opcode(c_hdr);
>+
>+ if (opcode > RDMAP_TERMINATE) {
>+ pr_warn("siw: received unknown packet type %u\n",
>+ opcode);
>+
>+ siw_init_terminate(rx_qp(srx), TERM_ERROR_LAYER_RDMAP,
>+ RDMAP_ETYPE_REMOTE_OPERATION,
>+ RDMAP_ECODE_OPCODE, 0);
>+ return -EINVAL;
>+ }
> }
> set_rx_fpdu_context(qp, opcode);
> frx = qp->rx_fpdu;
>--
>2.17.1
>
>

2021-02-26 12:36:35

by Dinghao Liu

[permalink] [raw]
Subject: Re: Re: [PATCH] RDMA/siw: Fix missing check in siw_get_hdr



&quot;Bernard Metzler&quot; &lt;[email protected]&gt;写道:
> -----"Dinghao Liu" <[email protected]> wrote: -----
>
> >To: [email protected], [email protected]
> >From: "Dinghao Liu" <[email protected]>
> >Date: 02/26/2021 08:56AM
> >Cc: "Bernard Metzler" <[email protected]>, "Doug Ledford"
> ><[email protected]>, "Jason Gunthorpe" <[email protected]>,
> >[email protected], [email protected]
> >Subject: [EXTERNAL] [PATCH] RDMA/siw: Fix missing check in
> >siw_get_hdr
> >
> >We should also check the range of opcode after calling
> >__rdmap_get_opcode() in the else branch to prevent potential
> >overflow.
>
> Hi Dinghao,
> No this is not needed. We always first read the minimum
> header information (MPA len, DDP flags, RDMAP opcode,
> STag, target offset). Only if we have received that
> into local buffer, we check for the opcode this one time.
> Now the opcode determines the remaining length of the
> variably sized part of the header to be received.
>
> We do not have to check the opcode again, since we
> already received and checked it.
>

It's clear to me, thanks!

Regards,
Dinghao