[PATCH 0/2] firmware: stratix10-svc: fix kaddr dangling pointers and populate kaddr1

[PATCH 0/2] firmware: stratix10-svc: fix kaddr dangling pointers and populate kaddr1

[Adrian Ng Ho Yin]

kaddr1/2/3 in the stratix10 service layer are set to addresses of local
arm_smccc_res fields that go out of scope when the setting function returns,
leaving dangling pointers in the callback data.

Patch 1 fixes this by adding a1/a2/a3 scratch fields to struct
stratix10_svc_cb_data, which lives for the SVC thread lifetime, and
pointing kaddrN to those fields instead of the stack.

Patch 2 uses the same mechanism to populate the previously unset kaddr1 in
svc_thread_cmd_config_status(), exposing firmware status details to callers.

Adrian Ng Ho Yin (2):
  firmware: stratix10-svc: fix dangling kaddr pointers in callback data
  firmware: stratix10-svc: populate kaddr1 from config status response

 drivers/firmware/stratix10-svc.c              | 45 ++++++++++++-------
 .../firmware/intel/stratix10-svc-client.h     | 17 +++++++
 2 files changed, 47 insertions(+), 15 deletions(-)

-- 
2.49.GIT

[PATCH 1/2] firmware: stratix10-svc: fix dangling kaddr pointers in callback data

[Adrian Ng Ho Yin]

svc_thread_recv_status_ok() and svc_thread_cmd_config_status() store
addresses of fields from a stack-local struct arm_smccc_res into
cb_data->kaddr1/2/3, then return. Once the stack frame is released
those pointers are dangling; any client that reads them after
receive_cb() returns accesses freed stack memory.

Add a1/a2/a3 storage fields to struct stratix10_svc_cb_data, which lives
for the lifetime of the SVC thread, and copy the SMC return values into
them instead of taking their stack address.  Apply the same pattern to
the STATUS_ERROR path in svc_normal_to_secure_thread() for consistency.

Signed-off-by: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
---
 drivers/firmware/stratix10-svc.c              | 43 ++++++++++++-------
 .../firmware/intel/stratix10-svc-client.h     | 17 ++++++++
 2 files changed, 45 insertions(+), 15 deletions(-)

diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
index 39eb78f5905b..32f09478e1f8 100644
--- a/drivers/firmware/stratix10-svc.c
+++ b/drivers/firmware/stratix10-svc.c
@@ -428,13 +428,16 @@ static void svc_thread_cmd_config_status(struct stratix10_svc_controller *ctrl,
 		cb_data->status = BIT(SVC_STATUS_COMPLETED);
 		cb_data->kaddr2 = (res.a2) ?
 				  svc_pa_to_va(res.a2) : NULL;
-		cb_data->kaddr3 = (res.a3) ? &res.a3 : NULL;
+		cb_data->a3 = res.a3;
+		cb_data->kaddr3 = (res.a3) ? &cb_data->a3 : NULL;
 	} else {
 		pr_err("%s: poll status error\n", __func__);
-		cb_data->kaddr1 = &res.a1;
+		cb_data->a1 = res.a1;
+		cb_data->kaddr1 = &cb_data->a1;
 		cb_data->kaddr2 = (res.a2) ?
 				  svc_pa_to_va(res.a2) : NULL;
-		cb_data->kaddr3 = (res.a3) ? &res.a3 : NULL;
+		cb_data->a3 = res.a3;
+		cb_data->kaddr3 = (res.a3) ? &cb_data->a3 : NULL;
 		cb_data->status = BIT(SVC_STATUS_ERROR);
 	}
 
@@ -480,32 +483,40 @@ static void svc_thread_recv_status_ok(struct stratix10_svc_data *p_data,
 	case COMMAND_HWMON_READTEMP:
 	case COMMAND_HWMON_READVOLT:
 		cb_data->status = BIT(SVC_STATUS_OK);
-		cb_data->kaddr1 = &res.a1;
+		cb_data->a1 = res.a1;
+		cb_data->kaddr1 = &cb_data->a1;
 		break;
 	case COMMAND_SMC_SVC_VERSION:
 		cb_data->status = BIT(SVC_STATUS_OK);
-		cb_data->kaddr1 = &res.a1;
-		cb_data->kaddr2 = &res.a2;
+		cb_data->a1 = res.a1;
+		cb_data->kaddr1 = &cb_data->a1;
+		cb_data->a2 = res.a2;
+		cb_data->kaddr2 = &cb_data->a2;
 		break;
 	case COMMAND_RSU_DCMF_VERSION:
 		cb_data->status = BIT(SVC_STATUS_OK);
-		cb_data->kaddr1 = &res.a1;
-		cb_data->kaddr2 = &res.a2;
+		cb_data->a1 = res.a1;
+		cb_data->kaddr1 = &cb_data->a1;
+		cb_data->a2 = res.a2;
+		cb_data->kaddr2 = &cb_data->a2;
 		break;
 	case COMMAND_FCS_RANDOM_NUMBER_GEN:
 	case COMMAND_FCS_GET_PROVISION_DATA:
 	case COMMAND_POLL_SERVICE_STATUS:
 		cb_data->status = BIT(SVC_STATUS_OK);
-		cb_data->kaddr1 = &res.a1;
+		cb_data->a1 = res.a1;
+		cb_data->kaddr1 = &cb_data->a1;
 		cb_data->kaddr2 = svc_pa_to_va(res.a2);
-		cb_data->kaddr3 = &res.a3;
+		cb_data->a3 = res.a3;
+		cb_data->kaddr3 = &cb_data->a3;
 		break;
 	case COMMAND_MBOX_SEND_CMD:
 		cb_data->status = BIT(SVC_STATUS_OK);
-		cb_data->kaddr1 = &res.a1;
+		cb_data->a1 = res.a1;
+		cb_data->kaddr1 = &cb_data->a1;
 		/* SDM return size in u8. Convert size to u32 word */
-		res.a2 = res.a2 * BYTE_TO_WORD_SIZE;
-		cb_data->kaddr2 = &res.a2;
+		cb_data->a2 = res.a2 * BYTE_TO_WORD_SIZE;
+		cb_data->kaddr2 = &cb_data->a2;
 		break;
 	default:
 		pr_warn("it shouldn't happen\n");
@@ -794,10 +805,12 @@ static int svc_normal_to_secure_thread(void *data)
 		case INTEL_SIP_SMC_RSU_ERROR:
 			pr_err("%s: STATUS_ERROR\n", __func__);
 			cbdata->status = BIT(SVC_STATUS_ERROR);
-			cbdata->kaddr1 = &res.a1;
+			cbdata->a1 = res.a1;
+			cbdata->kaddr1 = &cbdata->a1;
 			cbdata->kaddr2 = (res.a2) ?
 				svc_pa_to_va(res.a2) : NULL;
-			cbdata->kaddr3 = (res.a3) ? &res.a3 : NULL;
+			cbdata->a3 = res.a3;
+			cbdata->kaddr3 = (res.a3) ? &cbdata->a3 : NULL;
 			pdata->chan->scl->receive_cb(pdata->chan->scl, cbdata);
 			break;
 		default:
diff --git a/include/linux/firmware/intel/stratix10-svc-client.h b/include/linux/firmware/intel/stratix10-svc-client.h
index 91013161e9db..5c2b24b20359 100644
--- a/include/linux/firmware/intel/stratix10-svc-client.h
+++ b/include/linux/firmware/intel/stratix10-svc-client.h
@@ -222,12 +222,29 @@ struct stratix10_svc_command_config_type {
  * @kaddr1: address of 1st completed data block
  * @kaddr2: address of 2nd completed data block
  * @kaddr3: address of 3rd completed data block
+ * @a1: internal scratch storage; kaddr1 points here when the firmware
+ *      returns a scalar in res.a1 rather than a buffer address
+ * @a2: internal scratch storage; kaddr2 points here when the firmware
+ *      returns a scalar in res.a2 rather than a buffer address; the stored
+ *      value may be derived from res.a2 rather than copied verbatim
+ *      (e.g. byte-to-word unit conversion for MBOX_SEND_CMD)
+ * @a3: internal scratch storage; kaddr3 points here when the firmware
+ *      returns a scalar in res.a3 rather than a buffer address
+ *
+ * a1/a2/a3 are only used when the corresponding kaddrN field carries a
+ * scalar SMC/HVC return value.  When kaddrN points to a persistent mapped
+ * buffer obtained via svc_pa_to_va(), these fields are not used.  Storing
+ * the scalar copies here rather than in a stack-local arm_smccc_res ensures
+ * the pointer remains valid for the lifetime of this structure.
  */
 struct stratix10_svc_cb_data {
 	u32 status;
 	void *kaddr1;
 	void *kaddr2;
 	void *kaddr3;
+	unsigned long a1;
+	unsigned long a2;
+	unsigned long a3;
 };
 
 /**
-- 
2.49.GIT

[PATCH 2/2] firmware: stratix10-svc: populate kaddr1 from config status response

[Adrian Ng Ho Yin]

Set kaddr1 from res.a1 in svc_thread_cmd_config_status() on both the
completed and error paths, allowing callers to retrieve firmware-returned
status details via the standard kaddr1 interface.

Signed-off-by: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
---
 drivers/firmware/stratix10-svc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
index 32f09478e1f8..8dda11afd6a2 100644
--- a/drivers/firmware/stratix10-svc.c
+++ b/drivers/firmware/stratix10-svc.c
@@ -426,6 +426,8 @@ static void svc_thread_cmd_config_status(struct stratix10_svc_controller *ctrl,
 		cb_data->status = BIT(SVC_STATUS_BUSY);
 	} else if (res.a0 == INTEL_SIP_SMC_STATUS_OK) {
 		cb_data->status = BIT(SVC_STATUS_COMPLETED);
+		cb_data->a1 = res.a1;
+		cb_data->kaddr1 = &cb_data->a1;
 		cb_data->kaddr2 = (res.a2) ?
 				  svc_pa_to_va(res.a2) : NULL;
 		cb_data->a3 = res.a3;
-- 
2.49.GIT