[PATCH rdma-next] IB/mthca: Check validity of output parameter pointer

[PATCH rdma-next] IB/mthca: Check validity of output parameter pointer

[Leon Romanovsky]

The mthca driver didn't check supplied pointer to functions
mthca_cmd_poll() and mthca_cmd_wait(). This caused to the following
smatch errors:

drivers/infiniband/hw/mthca/mthca_cmd.c:371 mthca_cmd_poll() error: we previously assumed 'out_param' could be null (see line 353)
drivers/infiniband/hw/mthca/mthca_cmd.c:454 mthca_cmd_wait() error: we previously assumed 'out_param' could be null (see line 432)

In reality all callers of these functions are setting out_is_imm
flag are providing pointer too. However it is better to check
again to remove smatch errors to achieve warning free subsystem.

Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 Based on k.o/for-4.12 branch.
---
 drivers/infiniband/hw/mthca/mthca_cmd.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/hw/mthca/mthca_cmd.c b/drivers/infiniband/hw/mthca/mthca_cmd.c
index c7f49bbb0c72..9d83a53c0c67 100644
--- a/drivers/infiniband/hw/mthca/mthca_cmd.c
+++ b/drivers/infiniband/hw/mthca/mthca_cmd.c
@@ -367,12 +367,16 @@ static int mthca_cmd_poll(struct mthca_dev *dev,
 		goto out;
 	}

-	if (out_is_imm)
+	if (out_is_imm && out_param) {
 		*out_param =
 			(u64) be32_to_cpu((__force __be32)
 					  __raw_readl(dev->hcr + HCR_OUT_PARAM_OFFSET)) << 32 |
 			(u64) be32_to_cpu((__force __be32)
 					  __raw_readl(dev->hcr + HCR_OUT_PARAM_OFFSET + 4));
+	} else if (out_is_imm) {
+		err = -EINVAL;
+		goto out;
+	}

 	status = be32_to_cpu((__force __be32) __raw_readl(dev->hcr + HCR_STATUS_OFFSET)) >> 24;
 	if (status) {
@@ -450,8 +454,12 @@ static int mthca_cmd_wait(struct mthca_dev *dev,
 		err = mthca_status_to_errno(context->status);
 	}

-	if (out_is_imm)
+	if (out_is_imm && out_param) {
 		*out_param = context->out_param;
+	} else if (out_is_imm) {
+		err = -EINVAL;
+		goto out;
+	}

 out:
 	spin_lock(&dev->cmd.context_lock);
--
2.12.2

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

Re: [PATCH rdma-next] IB/mthca: Check validity of output parameter pointer

[Yuval Shaia]

On Sat, Apr 15, 2017 at 06:47:25PM +0300, Leon Romanovsky wrote:
> The mthca driver didn't check supplied pointer to functions
> mthca_cmd_poll() and mthca_cmd_wait(). This caused to the following
> smatch errors:
> 
> drivers/infiniband/hw/mthca/mthca_cmd.c:371 mthca_cmd_poll() error: we previously assumed 'out_param' could be null (see line 353)
> drivers/infiniband/hw/mthca/mthca_cmd.c:454 mthca_cmd_wait() error: we previously assumed 'out_param' could be null (see line 432)
> 
> In reality all callers of these functions are setting out_is_imm
> flag are providing pointer too. However it is better to check
> again to remove smatch errors to achieve warning free subsystem.
> 
> Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>  Based on k.o/for-4.12 branch.
> ---
>  drivers/infiniband/hw/mthca/mthca_cmd.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/mthca/mthca_cmd.c b/drivers/infiniband/hw/mthca/mthca_cmd.c
> index c7f49bbb0c72..9d83a53c0c67 100644
> --- a/drivers/infiniband/hw/mthca/mthca_cmd.c
> +++ b/drivers/infiniband/hw/mthca/mthca_cmd.c
> @@ -367,12 +367,16 @@ static int mthca_cmd_poll(struct mthca_dev *dev,
>  		goto out;
>  	}
> 
> -	if (out_is_imm)
> +	if (out_is_imm && out_param) {
>  		*out_param =
>  			(u64) be32_to_cpu((__force __be32)
>  					  __raw_readl(dev->hcr + HCR_OUT_PARAM_OFFSET)) << 32 |
>  			(u64) be32_to_cpu((__force __be32)
>  					  __raw_readl(dev->hcr + HCR_OUT_PARAM_OFFSET + 4));
> +	} else if (out_is_imm) {
> +		err = -EINVAL;
> +		goto out;

So if this is a good reason to fail the function - how about doing this
validation before triggering HW operation (mthca_cmd_post)?

> +	}
> 
>  	status = be32_to_cpu((__force __be32) __raw_readl(dev->hcr + HCR_STATUS_OFFSET)) >> 24;
>  	if (status) {
> @@ -450,8 +454,12 @@ static int mthca_cmd_wait(struct mthca_dev *dev,
>  		err = mthca_status_to_errno(context->status);
>  	}
> 
> -	if (out_is_imm)
> +	if (out_is_imm && out_param) {
>  		*out_param = context->out_param;
> +	} else if (out_is_imm) {
> +		err = -EINVAL;

Ditto

> +		goto out;
> +	}
> 
>  out:
>  	spin_lock(&dev->cmd.context_lock);
> --
> 2.12.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next] IB/mthca: Check validity of output parameter pointer

[Leon Romanovsky]

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

On Tue, Apr 18, 2017 at 04:48:56PM +0300, Yuval Shaia wrote:
> On Sat, Apr 15, 2017 at 06:47:25PM +0300, Leon Romanovsky wrote:
> > The mthca driver didn't check supplied pointer to functions
> > mthca_cmd_poll() and mthca_cmd_wait(). This caused to the following
> > smatch errors:
> >
> > drivers/infiniband/hw/mthca/mthca_cmd.c:371 mthca_cmd_poll() error: we previously assumed 'out_param' could be null (see line 353)
> > drivers/infiniband/hw/mthca/mthca_cmd.c:454 mthca_cmd_wait() error: we previously assumed 'out_param' could be null (see line 432)
> >
> > In reality all callers of these functions are setting out_is_imm
> > flag are providing pointer too. However it is better to check
> > again to remove smatch errors to achieve warning free subsystem.
> >
> > Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > ---
> >  Based on k.o/for-4.12 branch.
> > ---
> >  drivers/infiniband/hw/mthca/mthca_cmd.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/infiniband/hw/mthca/mthca_cmd.c b/drivers/infiniband/hw/mthca/mthca_cmd.c
> > index c7f49bbb0c72..9d83a53c0c67 100644
> > --- a/drivers/infiniband/hw/mthca/mthca_cmd.c
> > +++ b/drivers/infiniband/hw/mthca/mthca_cmd.c
> > @@ -367,12 +367,16 @@ static int mthca_cmd_poll(struct mthca_dev *dev,
> >  		goto out;
> >  	}
> >
> > -	if (out_is_imm)
> > +	if (out_is_imm && out_param) {
> >  		*out_param =
> >  			(u64) be32_to_cpu((__force __be32)
> >  					  __raw_readl(dev->hcr + HCR_OUT_PARAM_OFFSET)) << 32 |
> >  			(u64) be32_to_cpu((__force __be32)
> >  					  __raw_readl(dev->hcr + HCR_OUT_PARAM_OFFSET + 4));
> > +	} else if (out_is_imm) {
> > +		err = -EINVAL;
> > +		goto out;
>
> So if this is a good reason to fail the function - how about doing this
> validation before triggering HW operation (mthca_cmd_post)?

It doesn't matter, it will give the same bytecode.

>
> > +	}
> >
> >  	status = be32_to_cpu((__force __be32) __raw_readl(dev->hcr + HCR_STATUS_OFFSET)) >> 24;
> >  	if (status) {
> > @@ -450,8 +454,12 @@ static int mthca_cmd_wait(struct mthca_dev *dev,
> >  		err = mthca_status_to_errno(context->status);
> >  	}
> >
> > -	if (out_is_imm)
> > +	if (out_is_imm && out_param) {
> >  		*out_param = context->out_param;
> > +	} else if (out_is_imm) {
> > +		err = -EINVAL;
>
> Ditto
>
> > +		goto out;
> > +	}
> >
> >  out:
> >  	spin_lock(&dev->cmd.context_lock);
> > --
> > 2.12.2
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [PATCH rdma-next] IB/mthca: Check validity of output parameter pointer

[Doug Ledford]

On Sat, 2017-04-15 at 18:47 +0300, Leon Romanovsky wrote:
> The mthca driver didn't check supplied pointer to functions
> mthca_cmd_poll() and mthca_cmd_wait(). This caused to the following
> smatch errors:
> 
> drivers/infiniband/hw/mthca/mthca_cmd.c:371 mthca_cmd_poll() error:
> we previously assumed 'out_param' could be null (see line 353)
> drivers/infiniband/hw/mthca/mthca_cmd.c:454 mthca_cmd_wait() error:
> we previously assumed 'out_param' could be null (see line 432)
> 
> In reality all callers of these functions are setting out_is_imm
> flag are providing pointer too. However it is better to check
> again to remove smatch errors to achieve warning free subsystem.
> 
> Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Thanks, applied.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG KeyID: B826A3330E572FDD
   
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

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