[PATCH] RDMA/rxe: Implement packet length validation on responder

[PATCH] RDMA/rxe: Implement packet length validation on responder

[Daisuke Matsuda]

The function check_length() is supposed to check the length of inbound
packets on responder, but it actually has been a stub since the driver was
born. Let it check the payload length and the DMA length.

Signed-off-by: Daisuke Matsuda <matsuda-daisuke@fujitsu.com>
---
FOR REVIEWERS
  I referred to IB Specification Vol 1-Revision-1.5 to create this patch.
  Please see 9.7.4.1.6 (page.330).

 drivers/infiniband/sw/rxe/rxe_resp.c | 36 ++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 7 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
index ed5a09e86417..62e3a8195072 100644
--- a/drivers/infiniband/sw/rxe/rxe_resp.c
+++ b/drivers/infiniband/sw/rxe/rxe_resp.c
@@ -390,16 +390,38 @@ static enum resp_states check_resource(struct rxe_qp *qp,
 static enum resp_states check_length(struct rxe_qp *qp,
 				     struct rxe_pkt_info *pkt)
 {
-	switch (qp_type(qp)) {
-	case IB_QPT_RC:
-		return RESPST_CHK_RKEY;
+	int mtu = qp->mtu;
+	u32 payload = payload_size(pkt);
+	u32 dmalen = reth_len(pkt);
 
-	case IB_QPT_UC:
-		return RESPST_CHK_RKEY;
+	/* RoCEv2 packets do not have LRH.
+	 * Let's skip checking it.
+	 */
 
-	default:
-		return RESPST_CHK_RKEY;
+	if ((pkt->opcode & RXE_START_MASK) &&
+	    (pkt->opcode & RXE_END_MASK)) {
+		/* "only" packets */
+		if (payload > mtu)
+			return RESPST_ERR_LENGTH;
+
+	} else if ((pkt->opcode & RXE_START_MASK) ||
+		   (pkt->opcode & RXE_MIDDLE_MASK)) {
+		/* "first" or "middle" packets */
+		if (payload != mtu)
+			return RESPST_ERR_LENGTH;
+
+	} else if (pkt->opcode & RXE_END_MASK) {
+		/* "last" packets */
+		if ((pkt->paylen == 0) || (pkt->paylen > mtu))
+			return RESPST_ERR_LENGTH;
+	}
+
+	if (pkt->opcode & (RXE_WRITE_MASK | RXE_READ_MASK)) {
+		if (dmalen > (1 << 31))
+			return RESPST_ERR_LENGTH;
 	}
+
+	return RESPST_CHK_RKEY;
 }
 
 static enum resp_states check_rkey(struct rxe_qp *qp,
-- 
2.31.1

Re: [PATCH] RDMA/rxe: Implement packet length validation on responder

[lizhijian]

On 28/10/2022 17:04, Daisuke Matsuda wrote:
> The function check_length() is supposed to check the length of inbound
> packets on responder, but it actually has been a stub since the driver was
> born. Let it check the payload length and the DMA length.
>
> Signed-off-by: Daisuke Matsuda <matsuda-daisuke@fujitsu.com>
> ---
> FOR REVIEWERS
>    I referred to IB Specification Vol 1-Revision-1.5 to create this patch.
>    Please see 9.7.4.1.6 (page.330).
>
>   drivers/infiniband/sw/rxe/rxe_resp.c | 36 ++++++++++++++++++++++------
>   1 file changed, 29 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
> index ed5a09e86417..62e3a8195072 100644
> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
> @@ -390,16 +390,38 @@ static enum resp_states check_resource(struct rxe_qp *qp,
>   static enum resp_states check_length(struct rxe_qp *qp,
>   				     struct rxe_pkt_info *pkt)
>   {
> -	switch (qp_type(qp)) {
> -	case IB_QPT_RC:
> -		return RESPST_CHK_RKEY;
> +	int mtu = qp->mtu;
> +	u32 payload = payload_size(pkt);
> +	u32 dmalen = reth_len(pkt);
>   
> -	case IB_QPT_UC:
> -		return RESPST_CHK_RKEY;
> +	/* RoCEv2 packets do not have LRH.
> +	 * Let's skip checking it.
> +	 */
>   
> -	default:
> -		return RESPST_CHK_RKEY;
> +	if ((pkt->opcode & RXE_START_MASK) &&
> +	    (pkt->opcode & RXE_END_MASK)) {
> +		/* "only" packets */
> +		if (payload > mtu)
> +			return RESPST_ERR_LENGTH;
> +
> +	} else if ((pkt->opcode & RXE_START_MASK) ||
> +		   (pkt->opcode & RXE_MIDDLE_MASK)) {
> +		/* "first" or "middle" packets */
> +		if (payload != mtu)
> +			return RESPST_ERR_LENGTH;
> +
> +	} else if (pkt->opcode & RXE_END_MASK) {
> +		/* "last" packets */
> +		if ((pkt->paylen == 0) || (pkt->paylen > mtu))

As per IBA spec, i didn't see any difference about the 'packet payload 
length' from others,

so May I know why here you are using 'pkt->paylen' instead of payload ?


Thanks

Zhijian



> +			return RESPST_ERR_LENGTH;
> +	}
> +
> +	if (pkt->opcode & (RXE_WRITE_MASK | RXE_READ_MASK)) {
> +		if (dmalen > (1 << 31))
> +			return RESPST_ERR_LENGTH;
>   	}
> +
> +	return RESPST_CHK_RKEY;
>   }
>   
>   static enum resp_states check_rkey(struct rxe_qp *qp,

RE: [PATCH] RDMA/rxe: Implement packet length validation on responder

[Daisuke Matsuda (Fujitsu)]

On Sat, Nov 5, 2022 6:37 PM Li, Zhijian wrote:
> On 28/10/2022 17:04, Daisuke Matsuda wrote:
> > The function check_length() is supposed to check the length of inbound
> > packets on responder, but it actually has been a stub since the driver was
> > born. Let it check the payload length and the DMA length.
> >
> > Signed-off-by: Daisuke Matsuda <matsuda-daisuke@fujitsu.com>
> > ---
> > FOR REVIEWERS
> >    I referred to IB Specification Vol 1-Revision-1.5 to create this patch.
> >    Please see 9.7.4.1.6 (page.330).
> >
> >   drivers/infiniband/sw/rxe/rxe_resp.c | 36 ++++++++++++++++++++++------
> >   1 file changed, 29 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
> > index ed5a09e86417..62e3a8195072 100644
> > --- a/drivers/infiniband/sw/rxe/rxe_resp.c
> > +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
> > @@ -390,16 +390,38 @@ static enum resp_states check_resource(struct rxe_qp *qp,
> >   static enum resp_states check_length(struct rxe_qp *qp,
> >   				     struct rxe_pkt_info *pkt)
> >   {
> > -	switch (qp_type(qp)) {
> > -	case IB_QPT_RC:
> > -		return RESPST_CHK_RKEY;
> > +	int mtu = qp->mtu;
> > +	u32 payload = payload_size(pkt);
> > +	u32 dmalen = reth_len(pkt);
> >
> > -	case IB_QPT_UC:
> > -		return RESPST_CHK_RKEY;
> > +	/* RoCEv2 packets do not have LRH.
> > +	 * Let's skip checking it.
> > +	 */
> >
> > -	default:
> > -		return RESPST_CHK_RKEY;
> > +	if ((pkt->opcode & RXE_START_MASK) &&
> > +	    (pkt->opcode & RXE_END_MASK)) {
> > +		/* "only" packets */
> > +		if (payload > mtu)
> > +			return RESPST_ERR_LENGTH;
> > +
> > +	} else if ((pkt->opcode & RXE_START_MASK) ||
> > +		   (pkt->opcode & RXE_MIDDLE_MASK)) {
> > +		/* "first" or "middle" packets */
> > +		if (payload != mtu)
> > +			return RESPST_ERR_LENGTH;
> > +
> > +	} else if (pkt->opcode & RXE_END_MASK) {
> > +		/* "last" packets */
> > +		if ((pkt->paylen == 0) || (pkt->paylen > mtu))
> 
> As per IBA spec, i didn't see any difference about the 'packet payload
> length' from others,
> 
> so May I know why here you are using 'pkt->paylen' instead of payload ?

Thank you for taking a look.
It seems this is my mistake. I forgot to replace them with 'payload'.
I'll post v2 later.

Actually 'pkt->paylen' is larger than 'payload':
pkt->paylen = (IB BTH) + (IB payload) + (ICRC)

Daisuke

> 
> 
> Thanks
> 
> Zhijian
> 
> 
> 
> > +			return RESPST_ERR_LENGTH;
> > +	}
> > +
> > +	if (pkt->opcode & (RXE_WRITE_MASK | RXE_READ_MASK)) {
> > +		if (dmalen > (1 << 31))
> > +			return RESPST_ERR_LENGTH;
> >   	}
> > +
> > +	return RESPST_CHK_RKEY;
> >   }
> >
> >   static enum resp_states check_rkey(struct rxe_qp *qp,