diff mbox series

[for-next,03/20] RDMA/rtrs-clt: No need to check queue_depth when receiving

Message ID 20210503114818.288896-4-gi-oh.kim@ionos.com (mailing list archive)
State Superseded
Headers show
Series Misc update for rtrs | expand

Commit Message

Gioh Kim May 3, 2021, 11:48 a.m. UTC 
From: Gioh Kim <gi-oh.kim@cloud.ionos.com>

The queue_depth size is sent from server and
server already checks validity of the value.

Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs-clt.c | 6 ------
 1 file changed, 6 deletions(-)

Comments

Leon Romanovsky May 9, 2021, 11:24 a.m. UTC | #1 
On Mon, May 03, 2021 at 01:48:01PM +0200, Gioh Kim wrote:
> From: Gioh Kim <gi-oh.kim@cloud.ionos.com>
> 
> The queue_depth size is sent from server and
> server already checks validity of the value.

Do you trust server? What will be if server is not reliable and sends
garbage?

Thanks

> 
> Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
> Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
> ---
>  drivers/infiniband/ulp/rtrs/rtrs-clt.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> index 930a1b496f84..0c828ea0f500 100644
> --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> @@ -1772,12 +1772,6 @@ static int rtrs_rdma_conn_established(struct rtrs_clt_con *con,
>  	}
>  	if (con->c.cid == 0) {
>  		queue_depth = le16_to_cpu(msg->queue_depth);
> -
> -		if (queue_depth > MAX_SESS_QUEUE_DEPTH) {
> -			rtrs_err(clt, "Invalid RTRS message: queue=%d\n",
> -				  queue_depth);
> -			return -ECONNRESET;
> -		}
>  		if (!sess->rbufs || sess->queue_depth < queue_depth) {
>  			kfree(sess->rbufs);
>  			sess->rbufs = kcalloc(queue_depth, sizeof(*sess->rbufs),
> -- 
> 2.25.1
>
Md Haris Iqbal May 10, 2021, 11 a.m. UTC | #2 
On Sun, May 9, 2021 at 1:24 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Mon, May 03, 2021 at 01:48:01PM +0200, Gioh Kim wrote:
> > From: Gioh Kim <gi-oh.kim@cloud.ionos.com>
> >
> > The queue_depth size is sent from server and
> > server already checks validity of the value.
>
> Do you trust server? What will be if server is not reliable and sends
> garbage?

Hi Leon,

The server code checks for the queue_depth before sending. If the
server is really running malicious code, then the queue_depth is the
last thing that the client needs to worry about.

>
> Thanks
>
> >
> > Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
> > Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
> > ---
> >  drivers/infiniband/ulp/rtrs/rtrs-clt.c | 6 ------
> >  1 file changed, 6 deletions(-)
> >
> > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> > index 930a1b496f84..0c828ea0f500 100644
> > --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> > +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> > @@ -1772,12 +1772,6 @@ static int rtrs_rdma_conn_established(struct rtrs_clt_con *con,
> >       }
> >       if (con->c.cid == 0) {
> >               queue_depth = le16_to_cpu(msg->queue_depth);
> > -
> > -             if (queue_depth > MAX_SESS_QUEUE_DEPTH) {
> > -                     rtrs_err(clt, "Invalid RTRS message: queue=%d\n",
> > -                               queue_depth);
> > -                     return -ECONNRESET;
> > -             }
> >               if (!sess->rbufs || sess->queue_depth < queue_depth) {
> >                       kfree(sess->rbufs);
> >                       sess->rbufs = kcalloc(queue_depth, sizeof(*sess->rbufs),
> > --
> > 2.25.1
> >
Leon Romanovsky May 10, 2021, 11:53 a.m. UTC | #3 
On Mon, May 10, 2021 at 01:00:33PM +0200, Haris Iqbal wrote:
> On Sun, May 9, 2021 at 1:24 PM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Mon, May 03, 2021 at 01:48:01PM +0200, Gioh Kim wrote:
> > > From: Gioh Kim <gi-oh.kim@cloud.ionos.com>
> > >
> > > The queue_depth size is sent from server and
> > > server already checks validity of the value.
> >
> > Do you trust server? What will be if server is not reliable and sends
> > garbage?
> 
> Hi Leon,
> 
> The server code checks for the queue_depth before sending. If the
> server is really running malicious code, then the queue_depth is the
> last thing that the client needs to worry about.

Like what? for an example?

Thanks
Md Haris Iqbal May 10, 2021, 12:06 p.m. UTC | #4 
On Mon, May 10, 2021 at 1:53 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Mon, May 10, 2021 at 01:00:33PM +0200, Haris Iqbal wrote:
> > On Sun, May 9, 2021 at 1:24 PM Leon Romanovsky <leon@kernel.org> wrote:
> > >
> > > On Mon, May 03, 2021 at 01:48:01PM +0200, Gioh Kim wrote:
> > > > From: Gioh Kim <gi-oh.kim@cloud.ionos.com>
> > > >
> > > > The queue_depth size is sent from server and
> > > > server already checks validity of the value.
> > >
> > > Do you trust server? What will be if server is not reliable and sends
> > > garbage?
> >
> > Hi Leon,
> >
> > The server code checks for the queue_depth before sending. If the
> > server is really running malicious code, then the queue_depth is the
> > last thing that the client needs to worry about.
>
> Like what? for an example?

Like accessing compromised block devices. If the queue_depth is
garbage, the client would fail at allocation with ENOMEM; thats it.

>
> Thanks
Leon Romanovsky May 10, 2021, 12:17 p.m. UTC | #5 
On Mon, May 10, 2021 at 02:06:55PM +0200, Haris Iqbal wrote:
> On Mon, May 10, 2021 at 1:53 PM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Mon, May 10, 2021 at 01:00:33PM +0200, Haris Iqbal wrote:
> > > On Sun, May 9, 2021 at 1:24 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > >
> > > > On Mon, May 03, 2021 at 01:48:01PM +0200, Gioh Kim wrote:
> > > > > From: Gioh Kim <gi-oh.kim@cloud.ionos.com>
> > > > >
> > > > > The queue_depth size is sent from server and
> > > > > server already checks validity of the value.
> > > >
> > > > Do you trust server? What will be if server is not reliable and sends
> > > > garbage?
> > >
> > > Hi Leon,
> > >
> > > The server code checks for the queue_depth before sending. If the
> > > server is really running malicious code, then the queue_depth is the
> > > last thing that the client needs to worry about.
> >
> > Like what? for an example?
> 
> Like accessing compromised block devices. If the queue_depth is
> garbage, the client would fail at allocation with ENOMEM; thats it.

The client will get wrong data, check it and discard. The case of ENOMEM triggered
by remote side is different. It can cause to DDOS on the client.

Thanks

> 
> >
> > Thanks
Md Haris Iqbal May 10, 2021, 12:26 p.m. UTC | #6 
On Mon, May 10, 2021 at 2:18 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Mon, May 10, 2021 at 02:06:55PM +0200, Haris Iqbal wrote:
> > On Mon, May 10, 2021 at 1:53 PM Leon Romanovsky <leon@kernel.org> wrote:
> > >
> > > On Mon, May 10, 2021 at 01:00:33PM +0200, Haris Iqbal wrote:
> > > > On Sun, May 9, 2021 at 1:24 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > > >
> > > > > On Mon, May 03, 2021 at 01:48:01PM +0200, Gioh Kim wrote:
> > > > > > From: Gioh Kim <gi-oh.kim@cloud.ionos.com>
> > > > > >
> > > > > > The queue_depth size is sent from server and
> > > > > > server already checks validity of the value.
> > > > >
> > > > > Do you trust server? What will be if server is not reliable and sends
> > > > > garbage?
> > > >
> > > > Hi Leon,
> > > >
> > > > The server code checks for the queue_depth before sending. If the
> > > > server is really running malicious code, then the queue_depth is the
> > > > last thing that the client needs to worry about.
> > >
> > > Like what? for an example?
> >
> > Like accessing compromised block devices. If the queue_depth is
> > garbage, the client would fail at allocation with ENOMEM; thats it.
>
> The client will get wrong data, check it and discard. The case of ENOMEM triggered
> by remote side is different. It can cause to DDOS on the client.

True. This makes sense. Will drop this patch.

Thanks.

>
> Thanks
>
> >
> > >
> > > Thanks
diff mbox series

Patch

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
index 930a1b496f84..0c828ea0f500 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
@@ -1772,12 +1772,6 @@  static int rtrs_rdma_conn_established(struct rtrs_clt_con *con,
 	}
 	if (con->c.cid == 0) {
 		queue_depth = le16_to_cpu(msg->queue_depth);
-
-		if (queue_depth > MAX_SESS_QUEUE_DEPTH) {
-			rtrs_err(clt, "Invalid RTRS message: queue=%d\n",
-				  queue_depth);
-			return -ECONNRESET;
-		}
 		if (!sess->rbufs || sess->queue_depth < queue_depth) {
 			kfree(sess->rbufs);
 			sess->rbufs = kcalloc(queue_depth, sizeof(*sess->rbufs),