Skip to contentSkip to conversation

IB/mthca: Check validity of output parameter pointer

PATCH Applied Leon Romanovsky 3 participants 4 messages Apr 15, 2017 – Apr 24, 2017 1 message not in the archive InfiniBand

lore
Share & tools
lore

Leon Romanovsky

author
#0 lore raw

PATCH 1/1 IB/mthca: Check validity of output parameter pointer

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 <[email protected]>

---
Based on k.o/for-4.12 branch.

---
 drivers/infiniband/hw/mthca/mthca_cmd.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
drivers/infiniband/hw/mthca/mthca_cmd.c +10 -2
index c7f49bbb0c72..9d83a53c0c67 100644
@@ -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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20170415154725.17559-1-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org

#2 lore raw
Quoted 26 lines · 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 <[email protected]>

---
Based on k.o/for-4.12 branch.

---
 drivers/infiniband/hw/mthca/mthca_cmd.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
drivers/infiniband/hw/mthca/mthca_cmd.c +0 -0
index c7f49bbb0c72..9d83a53c0c67 100644
@@ -367,12 +367,16 @@ static int mthca_cmd_poll(struct mthca_dev *dev,
 		goto out;
 	}

drivers/infiniband/hw/mthca/mthca_cmd.c static int mthca_cmd_poll(struct mthca_dev *dev, · line 378

Quoted 10 lines
drivers/infiniband/hw/mthca/mthca_cmd.c +4 -1
-	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)?

drivers/infiniband/hw/mthca/mthca_cmd.c static int mthca_cmd_wait(struct mthca_dev *dev, · line 460

Quoted 13 lines
drivers/infiniband/hw/mthca/mthca_cmd.c +4 -1
+	}
 
 	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

Quoted 8 lines
drivers/infiniband/hw/mthca/mthca_cmd.c +2 -1
+		goto out;
+	}
 
 out:
 	spin_lock(&dev->cmd.context_lock);
--

2.12.2

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#3 lore raw
Quoted 27 lines · Yuval Shaia
Quoted 26 lines · 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 <[email protected]>

---
Based on k.o/for-4.12 branch.

---
 drivers/infiniband/hw/mthca/mthca_cmd.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
drivers/infiniband/hw/mthca/mthca_cmd.c +0 -0
index c7f49bbb0c72..9d83a53c0c67 100644
@@ -367,12 +367,16 @@ static int mthca_cmd_poll(struct mthca_dev *dev,
 		goto out;
 	}
Quoted 13 lines

drivers/infiniband/hw/mthca/mthca_cmd.c static int mthca_cmd_poll(struct mthca_dev *dev, · line 378

Quoted 10 lines
drivers/infiniband/hw/mthca/mthca_cmd.c +4 -1
-	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.

Quoted 29 lines
Quoted 6 lines
drivers/infiniband/hw/mthca/mthca_cmd.c +1 -0
+	}
 
 	status = be32_to_cpu((__force __be32) __raw_readl(dev->hcr + HCR_STATUS_OFFSET)) >> 24;
 	if (status) {

drivers/infiniband/hw/mthca/mthca_cmd.c static int mthca_cmd_wait(struct mthca_dev *dev, · line 460

Quoted 8 lines
drivers/infiniband/hw/mthca/mthca_cmd.c +3 -1
@@ -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

Quoted 12 lines
drivers/infiniband/hw/mthca/mthca_cmd.c +2 -1
+		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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Applied
#4 lore raw
In reply to a message not in the archive
Quoted 9 lines · 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)

Quoted 5 lines

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 <[email protected]>

Thanks, applied.

Signature · 9 lines
Doug Ledford <[email protected]>
    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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html