2022-06-08 10:46:17

by Cristian Marussi

[permalink] [raw]
Subject: [PATCH 3/3] firmware: arm_scmi: Use preferred strscpy to handle strings

Remove strlcpy calls used to collect short domain names of SCMI resources
in favour of the preferred strscpy; while doing that change also the used
count argument of strscpy to make always use of SCMI_SHORT_NAME_MAX_SIZE
while dealing with short domain names, so as to limit the possibility that
an ill-formed non-NULL terminated short reply from the SCMI platform
firmware can leak stale content laying in the underlying transport shared
memory area.

Cc: Peter Hilber <[email protected]>
Cc: Sudeep Holla <[email protected]>
Fixes: ca64b719a1e66 ("firmware: arm_scmi: use strlcpy to ensure NULL-terminated strings")
Signed-off-by: Cristian Marussi <[email protected]>
---
drivers/firmware/arm_scmi/clock.c | 2 +-
drivers/firmware/arm_scmi/perf.c | 2 +-
drivers/firmware/arm_scmi/power.c | 2 +-
drivers/firmware/arm_scmi/reset.c | 2 +-
drivers/firmware/arm_scmi/sensors.c | 4 ++--
drivers/firmware/arm_scmi/voltage.c | 2 +-
6 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 1a718faa4192..c7a83f6e38e5 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -153,7 +153,7 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
if (!ret) {
u32 latency = 0;
attributes = le32_to_cpu(attr->attributes);
- strlcpy(clk->name, attr->name, SCMI_MAX_STR_SIZE);
+ strscpy(clk->name, attr->name, SCMI_SHORT_NAME_MAX_SIZE);
/* clock_enable_latency field is present only since SCMI v3.1 */
if (PROTOCOL_REV_MAJOR(version) >= 0x2)
latency = le32_to_cpu(attr->clock_enable_latency);
diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index c1f701623058..bbb0331801ff 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -252,7 +252,7 @@ scmi_perf_domain_attributes_get(const struct scmi_protocol_handle *ph,
dom_info->mult_factor =
(dom_info->sustained_freq_khz * 1000) /
dom_info->sustained_perf_level;
- strlcpy(dom_info->name, attr->name, SCMI_MAX_STR_SIZE);
+ strscpy(dom_info->name, attr->name, SCMI_SHORT_NAME_MAX_SIZE);
}

ph->xops->xfer_put(ph, t);
diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c
index 964882cc8747..356e83631664 100644
--- a/drivers/firmware/arm_scmi/power.c
+++ b/drivers/firmware/arm_scmi/power.c
@@ -122,7 +122,7 @@ scmi_power_domain_attributes_get(const struct scmi_protocol_handle *ph,
dom_info->state_set_notify = SUPPORTS_STATE_SET_NOTIFY(flags);
dom_info->state_set_async = SUPPORTS_STATE_SET_ASYNC(flags);
dom_info->state_set_sync = SUPPORTS_STATE_SET_SYNC(flags);
- strlcpy(dom_info->name, attr->name, SCMI_MAX_STR_SIZE);
+ strscpy(dom_info->name, attr->name, SCMI_SHORT_NAME_MAX_SIZE);
}
ph->xops->xfer_put(ph, t);

diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
index a420a9102094..673f3eb498f4 100644
--- a/drivers/firmware/arm_scmi/reset.c
+++ b/drivers/firmware/arm_scmi/reset.c
@@ -116,7 +116,7 @@ scmi_reset_domain_attributes_get(const struct scmi_protocol_handle *ph,
dom_info->latency_us = le32_to_cpu(attr->latency);
if (dom_info->latency_us == U32_MAX)
dom_info->latency_us = 0;
- strlcpy(dom_info->name, attr->name, SCMI_MAX_STR_SIZE);
+ strscpy(dom_info->name, attr->name, SCMI_SHORT_NAME_MAX_SIZE);
}

ph->xops->xfer_put(ph, t);
diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index 58fe4f0175be..42efaee27b7c 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -412,7 +412,7 @@ iter_axes_desc_process_response(const struct scmi_protocol_handle *ph,
attrh = le32_to_cpu(adesc->attributes_high);
a->scale = S32_EXT(SENSOR_SCALE(attrh));
a->type = SENSOR_TYPE(attrh);
- strscpy(a->name, adesc->name, SCMI_MAX_STR_SIZE);
+ strscpy(a->name, adesc->name, SCMI_SHORT_NAME_MAX_SIZE);

if (a->extended_attrs) {
unsigned int ares = le32_to_cpu(adesc->resolution);
@@ -633,7 +633,7 @@ iter_sens_descr_process_response(const struct scmi_protocol_handle *ph,
SUPPORTS_AXIS(attrh) ?
SENSOR_AXIS_NUMBER(attrh) : 0,
SCMI_MAX_NUM_SENSOR_AXIS);
- strscpy(s->name, sdesc->name, SCMI_MAX_STR_SIZE);
+ strscpy(s->name, sdesc->name, SCMI_SHORT_NAME_MAX_SIZE);

/*
* If supported overwrite short name with the extended
diff --git a/drivers/firmware/arm_scmi/voltage.c b/drivers/firmware/arm_scmi/voltage.c
index 97df6d3dd131..5de93f637bd4 100644
--- a/drivers/firmware/arm_scmi/voltage.c
+++ b/drivers/firmware/arm_scmi/voltage.c
@@ -233,7 +233,7 @@ static int scmi_voltage_descriptors_get(const struct scmi_protocol_handle *ph,
v = vinfo->domains + dom;
v->id = dom;
attributes = le32_to_cpu(resp_dom->attr);
- strlcpy(v->name, resp_dom->name, SCMI_MAX_STR_SIZE);
+ strscpy(v->name, resp_dom->name, SCMI_SHORT_NAME_MAX_SIZE);

/*
* If supported overwrite short name with the extended one;
--
2.32.0


2022-06-08 15:52:51

by Peter Hilber

[permalink] [raw]
Subject: Re: [PATCH 3/3] firmware: arm_scmi: Use preferred strscpy to handle strings

On 08.06.22 11:55, Cristian Marussi wrote:
> Remove strlcpy calls used to collect short domain names of SCMI resources
> in favour of the preferred strscpy; while doing that change also the used
> count argument of strscpy to make always use of SCMI_SHORT_NAME_MAX_SIZE
> while dealing with short domain names, so as to limit the possibility that
> an ill-formed non-NULL terminated short reply from the SCMI platform
> firmware can leak stale content laying in the underlying transport shared
> memory area.
>
> Cc: Peter Hilber <[email protected]>
> Cc: Sudeep Holla <[email protected]>
> Fixes: ca64b719a1e66 ("firmware: arm_scmi: use strlcpy to ensure NULL-terminated strings")
> Signed-off-by: Cristian Marussi <[email protected]>

Reviewed-by: Peter Hilber <[email protected]>

> ---
> drivers/firmware/arm_scmi/clock.c | 2 +-
> drivers/firmware/arm_scmi/perf.c | 2 +-
> drivers/firmware/arm_scmi/power.c | 2 +-
> drivers/firmware/arm_scmi/reset.c | 2 +-
> drivers/firmware/arm_scmi/sensors.c | 4 ++--
> drivers/firmware/arm_scmi/voltage.c | 2 +-
> 6 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> index 1a718faa4192..c7a83f6e38e5 100644
> --- a/drivers/firmware/arm_scmi/clock.c
> +++ b/drivers/firmware/arm_scmi/clock.c
> @@ -153,7 +153,7 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
> if (!ret) {
> u32 latency = 0;
> attributes = le32_to_cpu(attr->attributes);
> - strlcpy(clk->name, attr->name, SCMI_MAX_STR_SIZE);
> + strscpy(clk->name, attr->name, SCMI_SHORT_NAME_MAX_SIZE);
> /* clock_enable_latency field is present only since SCMI v3.1 */
> if (PROTOCOL_REV_MAJOR(version) >= 0x2)
> latency = le32_to_cpu(attr->clock_enable_latency);
> diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
> index c1f701623058..bbb0331801ff 100644
> --- a/drivers/firmware/arm_scmi/perf.c
> +++ b/drivers/firmware/arm_scmi/perf.c
> @@ -252,7 +252,7 @@ scmi_perf_domain_attributes_get(const struct scmi_protocol_handle *ph,
> dom_info->mult_factor =
> (dom_info->sustained_freq_khz * 1000) /
> dom_info->sustained_perf_level;
> - strlcpy(dom_info->name, attr->name, SCMI_MAX_STR_SIZE);
> + strscpy(dom_info->name, attr->name, SCMI_SHORT_NAME_MAX_SIZE);
> }
>
> ph->xops->xfer_put(ph, t);
> diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c
> index 964882cc8747..356e83631664 100644
> --- a/drivers/firmware/arm_scmi/power.c
> +++ b/drivers/firmware/arm_scmi/power.c
> @@ -122,7 +122,7 @@ scmi_power_domain_attributes_get(const struct scmi_protocol_handle *ph,
> dom_info->state_set_notify = SUPPORTS_STATE_SET_NOTIFY(flags);
> dom_info->state_set_async = SUPPORTS_STATE_SET_ASYNC(flags);
> dom_info->state_set_sync = SUPPORTS_STATE_SET_SYNC(flags);
> - strlcpy(dom_info->name, attr->name, SCMI_MAX_STR_SIZE);
> + strscpy(dom_info->name, attr->name, SCMI_SHORT_NAME_MAX_SIZE);
> }
> ph->xops->xfer_put(ph, t);
>
> diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
> index a420a9102094..673f3eb498f4 100644
> --- a/drivers/firmware/arm_scmi/reset.c
> +++ b/drivers/firmware/arm_scmi/reset.c
> @@ -116,7 +116,7 @@ scmi_reset_domain_attributes_get(const struct scmi_protocol_handle *ph,
> dom_info->latency_us = le32_to_cpu(attr->latency);
> if (dom_info->latency_us == U32_MAX)
> dom_info->latency_us = 0;
> - strlcpy(dom_info->name, attr->name, SCMI_MAX_STR_SIZE);
> + strscpy(dom_info->name, attr->name, SCMI_SHORT_NAME_MAX_SIZE);
> }
>
> ph->xops->xfer_put(ph, t);
> diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
> index 58fe4f0175be..42efaee27b7c 100644
> --- a/drivers/firmware/arm_scmi/sensors.c
> +++ b/drivers/firmware/arm_scmi/sensors.c
> @@ -412,7 +412,7 @@ iter_axes_desc_process_response(const struct scmi_protocol_handle *ph,
> attrh = le32_to_cpu(adesc->attributes_high);
> a->scale = S32_EXT(SENSOR_SCALE(attrh));
> a->type = SENSOR_TYPE(attrh);
> - strscpy(a->name, adesc->name, SCMI_MAX_STR_SIZE);
> + strscpy(a->name, adesc->name, SCMI_SHORT_NAME_MAX_SIZE);
>
> if (a->extended_attrs) {
> unsigned int ares = le32_to_cpu(adesc->resolution);
> @@ -633,7 +633,7 @@ iter_sens_descr_process_response(const struct scmi_protocol_handle *ph,
> SUPPORTS_AXIS(attrh) ?
> SENSOR_AXIS_NUMBER(attrh) : 0,
> SCMI_MAX_NUM_SENSOR_AXIS);
> - strscpy(s->name, sdesc->name, SCMI_MAX_STR_SIZE);
> + strscpy(s->name, sdesc->name, SCMI_SHORT_NAME_MAX_SIZE);
>
> /*
> * If supported overwrite short name with the extended
> diff --git a/drivers/firmware/arm_scmi/voltage.c b/drivers/firmware/arm_scmi/voltage.c
> index 97df6d3dd131..5de93f637bd4 100644
> --- a/drivers/firmware/arm_scmi/voltage.c
> +++ b/drivers/firmware/arm_scmi/voltage.c
> @@ -233,7 +233,7 @@ static int scmi_voltage_descriptors_get(const struct scmi_protocol_handle *ph,
> v = vinfo->domains + dom;
> v->id = dom;
> attributes = le32_to_cpu(resp_dom->attr);
> - strlcpy(v->name, resp_dom->name, SCMI_MAX_STR_SIZE);
> + strscpy(v->name, resp_dom->name, SCMI_SHORT_NAME_MAX_SIZE);
>
> /*
> * If supported overwrite short name with the extended one;