[PATCH] IB/core: type promotion bug in rdma_rw_init_one_mr()

[PATCH] IB/core: type promotion bug in rdma_rw_init_one_mr()

[Dan Carpenter]

"nents" is an unsigned int, so if ib_map_mr_sg() returns a negative
error code then it's type promoted to a high unsigned int which is
treated as success.

Fixes: a060b5629ab0 ("IB/core: generic RDMA READ/WRITE API")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/infiniband/core/rw.c b/drivers/infiniband/core/rw.c
index c8963e91f92a..3ee0adfb45e9 100644
--- a/drivers/infiniband/core/rw.c
+++ b/drivers/infiniband/core/rw.c
@@ -87,7 +87,7 @@ static int rdma_rw_init_one_mr(struct ib_qp *qp, u8 port_num,
 	}
 
 	ret = ib_map_mr_sg(reg->mr, sg, nents, &offset, PAGE_SIZE);
-	if (ret < nents) {
+	if (ret < 0 || ret < nents) {
 		ib_mr_pool_put(qp, &qp->rdma_mrs, reg->mr);
 		return -EINVAL;
 	}

Re: [PATCH] IB/core: type promotion bug in rdma_rw_init_one_mr()

[Walter Harms]

Am 04.07.2018 11:32, schrieb Dan Carpenter:
> "nents" is an unsigned int, so if ib_map_mr_sg() returns a negative
> error code then it's type promoted to a high unsigned int which is
> treated as success.
> 
> Fixes: a060b5629ab0 ("IB/core: generic RDMA READ/WRITE API")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/infiniband/core/rw.c b/drivers/infiniband/core/rw.c
> index c8963e91f92a..3ee0adfb45e9 100644
> --- a/drivers/infiniband/core/rw.c
> +++ b/drivers/infiniband/core/rw.c
> @@ -87,7 +87,7 @@ static int rdma_rw_init_one_mr(struct ib_qp *qp, u8
> port_num,
>  	}
>  
>  	ret = ib_map_mr_sg(reg->mr, sg, nents, &offset, PAGE_SIZE);
> -	if (ret < nents) {
> +	if (ret < 0 || ret < nents) {
>  		ib_mr_pool_put(qp, &qp->rdma_mrs, reg->mr);
>  		return -EINVAL;
>  	}

I do not understand the error here.
 
ib_map_mr_sg() obviously knows about nents and returns an invalid value ?
Or is that a special case here ?

re,
 wh

> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] IB/core: type promotion bug in rdma_rw_init_one_mr()

[Dan Carpenter]

On Wed, Jul 04, 2018 at 12:41:53PM +0200, Walter Harms wrote:
> 
> Am 04.07.2018 11:32, schrieb Dan Carpenter:
> > "nents" is an unsigned int, so if ib_map_mr_sg() returns a negative
> > error code then it's type promoted to a high unsigned int which is
> > treated as success.
> > 
> > Fixes: a060b5629ab0 ("IB/core: generic RDMA READ/WRITE API")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > diff --git a/drivers/infiniband/core/rw.c b/drivers/infiniband/core/rw.c
> > index c8963e91f92a..3ee0adfb45e9 100644
> > --- a/drivers/infiniband/core/rw.c
> > +++ b/drivers/infiniband/core/rw.c
> > @@ -87,7 +87,7 @@ static int rdma_rw_init_one_mr(struct ib_qp *qp, u8
> > port_num,
> >  	}
> >  
> >  	ret = ib_map_mr_sg(reg->mr, sg, nents, &offset, PAGE_SIZE);
> > -	if (ret < nents) {
> > +	if (ret < 0 || ret < nents) {
> >  		ib_mr_pool_put(qp, &qp->rdma_mrs, reg->mr);
> >  		return -EINVAL;
> >  	}
> 
> I do not understand the error here.
>  
> ib_map_mr_sg() obviously knows about nents and returns an invalid value ?
> Or is that a special case here ?
> 

ib_map_mr_sg() can return -ENOMEM, -EINVAL, other error codes or a
positive number <= to nents.  If it returns -ENOMEM then that's type
promoted to a high positive value (more than nents for sure) and treated
as success.

regards,
dan carpenter

Re: [PATCH] IB/core: type promotion bug in rdma_rw_init_one_mr()

[Jason Gunthorpe]

On Wed, Jul 04, 2018 at 12:55:41PM +0200, HÃ¥kon Bugge wrote:
>    Is:
> 
>    if (ret < (int)nents) {
> 
>    a more intuitive fix?

That could lead to truncation/force negativeness of nents :(

I wonder how many bugs like this we have.

Jason
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/core: type promotion bug in rdma_rw_init_one_mr()

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 728 bytes --]

On Wed, Jul 04, 2018 at 08:49:47AM -0600, Jason Gunthorpe wrote:
> On Wed, Jul 04, 2018 at 12:55:41PM +0200, Håkon Bugge wrote:
> >    Is:
> >
> >    if (ret < (int)nents) {
> >
> >    a more intuitive fix?
>
> That could lead to truncation/force negativeness of nents :(

IS_ERROR_VALUE() from include/linux/err.h?

>
> I wonder how many bugs like this we have.

Judging by the number of similar patches in netdev, I don't think
that we will have many "bugs", but maybe I'm wrong about it.

>
> Jason
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] IB/core: type promotion bug in rdma_rw_init_one_mr()

[Dan Carpenter]

On Wed, Jul 04, 2018 at 08:49:47AM -0600, Jason Gunthorpe wrote:
> On Wed, Jul 04, 2018 at 12:55:41PM +0200, Håkon Bugge wrote:
> >    Is:
> > 
> >    if (ret < (int)nents) {
> > 
> >    a more intuitive fix?
> 
> That could lead to truncation/force negativeness of nents :(
> 

In this case, if nents is over INT_MAX we're already toasted.

> I wonder how many bugs like this we have.

This is a static checker fix, so Julia fixed 3 and I fixed 6...

regards,
dan carpenter


--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/core: type promotion bug in rdma_rw_init_one_mr()

[Dan Carpenter]

On Wed, Jul 04, 2018 at 07:27:43PM +0300, Leon Romanovsky wrote:
> On Wed, Jul 04, 2018 at 08:49:47AM -0600, Jason Gunthorpe wrote:
> > On Wed, Jul 04, 2018 at 12:55:41PM +0200, Håkon Bugge wrote:
> > >    Is:
> > >
> > >    if (ret < (int)nents) {
> > >
> > >    a more intuitive fix?
> >
> > That could lead to truncation/force negativeness of nents :(
> 
> IS_ERROR_VALUE() from include/linux/err.h?
> 

Heh.  Please, no.  First of all that doesn't work, but also that's not
how IS_ERROR_VALUE() is supposed to be used.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/core: type promotion bug in rdma_rw_init_one_mr()

[Jason Gunthorpe]

On Wed, Jul 04, 2018 at 08:01:57PM +0300, Dan Carpenter wrote:
> On Wed, Jul 04, 2018 at 08:49:47AM -0600, Jason Gunthorpe wrote:
> > On Wed, Jul 04, 2018 at 12:55:41PM +0200, HÃ¥kon Bugge wrote:
> > >    Is:
> > > 
> > >    if (ret < (int)nents) {
> > > 
> > >    a more intuitive fix?
> > 
> > That could lead to truncation/force negativeness of nents :(
> > 
> 
> In this case, if nents is over INT_MAX we're already toasted.

Ugh, yes, functions accepting int for unsigned values is any
alarmingly common mistake too.

> > I wonder how many bugs like this we have.
> 
> This is a static checker fix, so Julia fixed 3 and I fixed 6...

Leon found another case of implicit casting creating a user space
triggerable bug last week..

Jason
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/core: type promotion bug in rdma_rw_init_one_mr()

[Jason Gunthorpe]

On Wed, Jul 04, 2018 at 12:32:12PM +0300, Dan Carpenter wrote:
> "nents" is an unsigned int, so if ib_map_mr_sg() returns a negative
> error code then it's type promoted to a high unsigned int which is
> treated as success.
> 
> Fixes: a060b5629ab0 ("IB/core: generic RDMA READ/WRITE API")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Applied to for-next

Thanks,
Jason

Re: [PATCH] IB/core: type promotion bug in rdma_rw_init_one_mr()

[Christoph Hellwig]

On Wed, Jul 04, 2018 at 12:32:12PM +0300, Dan Carpenter wrote:
> "nents" is an unsigned int, so if ib_map_mr_sg() returns a negative
> error code then it's type promoted to a high unsigned int which is
> treated as success.
> 
> Fixes: a060b5629ab0 ("IB/core: generic RDMA READ/WRITE API")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>