iv/linux
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Arm SCMI firmware driver fixes for v5.19

Browse Source 
Bunch of fixes to address:
 1. Issues reported on RK3568 EVB1 and BPI-R2 pro platforms using SCMI.
    More checks were added to validate the firmware response but that
    resulted in breaking above platforms, so the checks are relaxed when
    for cases where there is no potential memory corruption issues.
 
 2. Possible data leak by reading more than required length from the firmware.
    Recent addition of support for v3.1 extended names used larger buffers
    in the kernel and used their size to read response from the firmware even
    for cases where shorter formats are used. While that is mostly harmless
    except when firmware sends malformed non-NULL terminated buffers.
 
 3. Possible issues sending unsupported commands to the firmware.
    SENSOR_AXIS_NAME_GET added in v3.1 needs to be used only if the firmware
    supports it. While the firmware conformant to the spec must return not
    supported error for any unsupported features, it is always safer to
    avoid issuing commands that are known to be unsupported.
 
 4. Incorrect error propagation in scmi_voltage_descriptors_get.
    Since the return value is not reset for each iteration of the loop, the
    error value in the previous iteration will be carried for the current one.
    Fix that by not saving the return values into local variable.
 
 5. Some warnings reported by cppcheck
 -----BEGIN PGP SIGNATURE-----
 
 iQIzBAABCAAdFiEEunHlEgbzHrJD3ZPhAEG6vDF+4pgFAmKmORcACgkQAEG6vDF+
 4pgQkQ//Q7GnKFQdxm5r1fpphkFFPC3WKnQk2EU/+AHXXyLdxpg88CHKcWc1k+07
 PBwI0gOSLwVERS21m9zRrXUxA7gz6kdBKs6ycMpRt8LNXUYbV3Yxit8F7orKW6oK
 oOkCY6dC44Krg7ZXYaWsduoTECgvivoef6JduQru1/1PUBa6Cu5PrviF9sutYuB4
 qnHxoTykp2f0jQ9/9KOAcIDHCJGPccV14OK0AAXDcWBZgswnajoZk663Je1g+fxn
 0+b7B27DpmtArtTbcyxIgOUHSWP22HoLEoow4JELUXebtQ1CGcxb9I+FBAFzdyPq
 hyV/kchkr2hn/KaCzr7aQCDzmbleVxtr+6Ebe6ysokU+g2R/wvexpCVnogu3Pt4I
 D9bQ+uX3iLHFTbXj8j0w6pNE4+/FgytVKI2VOv+OM7regDXVsOKPAdIha4n6UoU1
 6gaeTjW0ovlylWFFx4etnVJmPh/pF6Ph38hyPlkro/JaI8wylXBSkdkDBPUz5AOc
 96Gg1QzTCETGHgKo0If1o+BnByxGOuNoXRFoC82TAxrHdzziLSNpRPj0vcvPm1hZ
 Q8e4eKE83bw1xZLFObJQJUf0xAy3cSEdASZfMS0z1KCz5uenih+FeOdJJ3VJunES
 1fpfPSA0Px5OUcCzT2J/JJ31I/cnbKrfTJhwmP7An8Iu2AZb9Oc=
 =NOFn
 -----END PGP SIGNATURE-----
gpgsig -----BEGIN PGP SIGNATURE-----
 
 iQIzBAABCgAdFiEEo6/YBQwIrVS28WGKmmx57+YAGNkFAmKoYAMACgkQmmx57+YA
 GNl+/A//YPMaYMe7YeJaFd784PJl/h2wM+gl6/YP64aal2weT3XJmsuv+woYf4mN
 3cJdqBkYVD43RxOy6tzwZ8zV6G1yaD5EVCpZT9NjZZOClQABeIYeE6lo66L6f3FO
 SD+a/i+wfibiGUq3urHhOLCNKCP/62GV1OhC+S5A1Fx1GQCuohd5xtDA+3TXn66Q
 NPNMuZnOcT+XqujWX0fMri4CKg96oTJbu5+kgtF0PilP4Tr17DEITiNkBRfRV3ll
 mvcV1Og3w44wuziJ6Qdaru+aKyDoxP34GCIjR9khJGp9sVzmul4pakYmt8yTzhhc
 7crsMEpSh4gp80vLM9i3Xw9ZGB3af5/+s4pR4T2AqLxhs1sdqcYG5mLoSWO+nsld
 V/O6Cugmkwnyn3Nw44ohpSnrV+0MnVMO+UmsYGZPZ7zz0GoAqGW4OnXyByC+XD25
 xnBpMyThwk3OdIVq9tViyOMUaQVgg3km2HE6TsqmEn+bHF1dl1FwgwrP/lJrkCVQ
 pqAwl5A25UAOhajOwqvzu5MFQohzzFMKRLQR5fwU0qT7MzPS9ZTNUWXHv+rUapzN
 dhtZ/4UquFx3HDGH9Gm4mHGQ8mi0LmoUnRBNwiJnnDhaC04ipEVcUBq4FsLmwET5
 9sjmXTsg3sLAdLw0m6N9+ZQjxNccOC1jvKGFf2eaqC4sdJV4Nzo=
 =Y1Nw
 -----END PGP SIGNATURE-----

Merge tag 'scmi-fixes-5.19' of git://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux into arm/fixes

Arm SCMI firmware driver fixes for v5.19

Bunch of fixes to address:
1. Issues reported on RK3568 EVB1 and BPI-R2 pro platforms using SCMI.
   More checks were added to validate the firmware response but that
   resulted in breaking above platforms, so the checks are relaxed when
   for cases where there is no potential memory corruption issues.

2. Possible data leak by reading more than required length from the firmware.
   Recent addition of support for v3.1 extended names used larger buffers
   in the kernel and used their size to read response from the firmware even
   for cases where shorter formats are used. While that is mostly harmless
   except when firmware sends malformed non-NULL terminated buffers.

3. Possible issues sending unsupported commands to the firmware.
   SENSOR_AXIS_NAME_GET added in v3.1 needs to be used only if the firmware
   supports it. While the firmware conformant to the spec must return not
   supported error for any unsupported features, it is always safer to
   avoid issuing commands that are known to be unsupported.

4. Incorrect error propagation in scmi_voltage_descriptors_get.
   Since the return value is not reset for each iteration of the loop, the
   error value in the previous iteration will be carried for the current one.
   Fix that by not saving the return values into local variable.

5. Some warnings reported by cppcheck

* tag 'scmi-fixes-5.19' of git://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux:
  firmware: arm_scmi: Fix incorrect error propagation in scmi_voltage_descriptors_get
  firmware: arm_scmi: Avoid using extended string-buffers sizes if not necessary
  firmware: arm_scmi: Fix SENSOR_AXIS_NAME_GET behaviour when unsupported
  firmware: arm_scmi: Remove all the unused local variables
  firmware: arm_scmi: Relax base protocol sanity checks on the protocol list

Link: https://lore.kernel.org/r/20220614100007.1029881-1-sudeep.holla@arm.com
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
This commit is contained in:
Arnd Bergmann 2022-06-14 12:16:34 +02:00
commit 002ec15747
9 changed files with 85 additions and 50 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

24
drivers/firmware/arm_scmi/base.c
View File

@ -36,7 +36,7 @@ struct scmi_msg_resp_base_attributes {
struct scmi_msg_resp_base_discover_agent {
__le32 agent_id;
u8 name[SCMI_MAX_STR_SIZE];
u8 name[SCMI_SHORT_NAME_MAX_SIZE];
};
@ -119,7 +119,7 @@ scmi_base_vendor_id_get(const struct scmi_protocol_handle *ph, bool sub_vendor)
ret = ph->xops->do_xfer(ph, t);
if (!ret)
memcpy(vendor_id, t->rx.buf, size);
strscpy(vendor_id, t->rx.buf, size);
ph->xops->xfer_put(ph, t);
@ -221,11 +221,17 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph,
calc_list_sz = (1 + (loop_num_ret - 1) / sizeof(u32)) *
sizeof(u32);
if (calc_list_sz != real_list_sz) {
dev_err(dev,
"Malformed reply - real_sz:%zd calc_sz:%u\n",
real_list_sz, calc_list_sz);
ret = -EPROTO;
break;
dev_warn(dev,
"Malformed reply - real_sz:%zd calc_sz:%u (loop_num_ret:%d)\n",
real_list_sz, calc_list_sz, loop_num_ret);
/*
* Bail out if the expected list size is bigger than the
* total payload size of the received reply.
*/
if (calc_list_sz > real_list_sz) {
ret = -EPROTO;
break;
}
}
for (loop = 0; loop < loop_num_ret; loop++)
@ -270,7 +276,7 @@ static int scmi_base_discover_agent_get(const struct scmi_protocol_handle *ph,
ret = ph->xops->do_xfer(ph, t);
if (!ret) {
agent_info = t->rx.buf;
strlcpy(name, agent_info->name, SCMI_MAX_STR_SIZE);
strscpy(name, agent_info->name, SCMI_SHORT_NAME_MAX_SIZE);
}
ph->xops->xfer_put(ph, t);
@ -369,7 +375,7 @@ static int scmi_base_protocol_init(const struct scmi_protocol_handle *ph)
int id, ret;
u8 *prot_imp;
u32 version;
char name[SCMI_MAX_STR_SIZE];
char name[SCMI_SHORT_NAME_MAX_SIZE];
struct device *dev = ph->dev;
struct scmi_revision_info *rev = scmi_revision_area_get(ph);

7
drivers/firmware/arm_scmi/clock.c
View File

@ -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);
@ -266,9 +266,7 @@ scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph, u32 clk_id,
struct scmi_clock_info *clk)
{
int ret;
void *iter;
struct scmi_msg_clock_describe_rates *msg;
struct scmi_iterator_ops ops = {
.prepare_message = iter_clk_describe_prepare_message,
.update_state = iter_clk_describe_update_state,
@ -281,7 +279,8 @@ scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph, u32 clk_id,
iter = ph->hops->iter_response_init(ph, &ops, SCMI_MAX_NUM_RATES,
CLOCK_DESCRIBE_RATES,
sizeof(*msg), &cpriv);
sizeof(struct scmi_msg_clock_describe_rates),
&cpriv);
if (IS_ERR(iter))
return PTR_ERR(iter);

6
drivers/firmware/arm_scmi/perf.c
View File

@ -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);
@ -332,7 +332,6 @@ scmi_perf_describe_levels_get(const struct scmi_protocol_handle *ph, u32 domain,
{
int ret;
void *iter;
struct scmi_msg_perf_describe_levels *msg;
struct scmi_iterator_ops ops = {
.prepare_message = iter_perf_levels_prepare_message,
.update_state = iter_perf_levels_update_state,
@ -345,7 +344,8 @@ scmi_perf_describe_levels_get(const struct scmi_protocol_handle *ph, u32 domain,
iter = ph->hops->iter_response_init(ph, &ops, MAX_OPPS,
PERF_DESCRIBE_LEVELS,
sizeof(*msg), &ppriv);
sizeof(struct scmi_msg_perf_describe_levels),
&ppriv);
if (IS_ERR(iter))
return PTR_ERR(iter);

2
drivers/firmware/arm_scmi/power.c
View File

@ -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);

2
drivers/firmware/arm_scmi/protocols.h
View File

@ -24,8 +24,6 @@
#include <asm/unaligned.h>
#define SCMI_SHORT_NAME_MAX_SIZE 16
#define PROTOCOL_REV_MINOR_MASK GENMASK(15, 0)
#define PROTOCOL_REV_MAJOR_MASK GENMASK(31, 16)
#define PROTOCOL_REV_MAJOR(x) ((u16)(FIELD_GET(PROTOCOL_REV_MAJOR_MASK, (x))))

2
drivers/firmware/arm_scmi/reset.c
View File

@ -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);

68
drivers/firmware/arm_scmi/sensors.c
View File

@ -338,7 +338,6 @@ static int scmi_sensor_update_intervals(const struct scmi_protocol_handle *ph,
struct scmi_sensor_info *s)
{
void *iter;
struct scmi_msg_sensor_list_update_intervals *msg;
struct scmi_iterator_ops ops = {
.prepare_message = iter_intervals_prepare_message,
.update_state = iter_intervals_update_state,
@ -351,22 +350,28 @@ static int scmi_sensor_update_intervals(const struct scmi_protocol_handle *ph,
iter = ph->hops->iter_response_init(ph, &ops, s->intervals.count,
SENSOR_LIST_UPDATE_INTERVALS,
sizeof(*msg), &upriv);
sizeof(struct scmi_msg_sensor_list_update_intervals),
&upriv);
if (IS_ERR(iter))
return PTR_ERR(iter);
return ph->hops->iter_response_run(iter);
}
struct scmi_apriv {
bool any_axes_support_extended_names;
struct scmi_sensor_info *s;
};
static void iter_axes_desc_prepare_message(void *message,
const unsigned int desc_index,
const void *priv)
{
struct scmi_msg_sensor_axis_description_get *msg = message;
const struct scmi_sensor_info *s = priv;
const struct scmi_apriv *apriv = priv;
/* Set the number of sensors to be skipped/already read */
msg->id = cpu_to_le32(s->id);
msg->id = cpu_to_le32(apriv->s->id);
msg->axis_desc_index = cpu_to_le32(desc_index);
}
@ -393,19 +398,21 @@ iter_axes_desc_process_response(const struct scmi_protocol_handle *ph,
u32 attrh, attrl;
struct scmi_sensor_axis_info *a;
size_t dsize = SCMI_MSG_RESP_AXIS_DESCR_BASE_SZ;
struct scmi_sensor_info *s = priv;
struct scmi_apriv *apriv = priv;
const struct scmi_axis_descriptor *adesc = st->priv;
attrl = le32_to_cpu(adesc->attributes_low);
if (SUPPORTS_EXTENDED_AXIS_NAMES(attrl))
apriv->any_axes_support_extended_names = true;
a = &s->axis[st->desc_index + st->loop_idx];
a = &apriv->s->axis[st->desc_index + st->loop_idx];
a->id = le32_to_cpu(adesc->id);
a->extended_attrs = SUPPORTS_EXTEND_ATTRS(attrl);
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);
@ -444,10 +451,19 @@ iter_axes_extended_name_process_response(const struct scmi_protocol_handle *ph,
void *priv)
{
struct scmi_sensor_axis_info *a;
const struct scmi_sensor_info *s = priv;
const struct scmi_apriv *apriv = priv;
struct scmi_sensor_axis_name_descriptor *adesc = st->priv;
u32 axis_id = le32_to_cpu(adesc->axis_id);
a = &s->axis[st->desc_index + st->loop_idx];
if (axis_id >= st->max_resources)
return -EPROTO;
/*
* Pick the corresponding descriptor based on the axis_id embedded
* in the reply since the list of axes supporting extended names
* can be a subset of all the axes.
*/
a = &apriv->s->axis[axis_id];
strscpy(a->name, adesc->name, SCMI_MAX_STR_SIZE);
st->priv = ++adesc;
@ -458,21 +474,36 @@ static int
scmi_sensor_axis_extended_names_get(const struct scmi_protocol_handle *ph,
struct scmi_sensor_info *s)
{
int ret;
void *iter;
struct scmi_msg_sensor_axis_description_get *msg;
struct scmi_iterator_ops ops = {
.prepare_message = iter_axes_desc_prepare_message,
.update_state = iter_axes_extended_name_update_state,
.process_response = iter_axes_extended_name_process_response,
};
struct scmi_apriv apriv = {
.any_axes_support_extended_names = false,
.s = s,
};
iter = ph->hops->iter_response_init(ph, &ops, s->num_axis,
SENSOR_AXIS_NAME_GET,
sizeof(*msg), s);
sizeof(struct scmi_msg_sensor_axis_description_get),
&apriv);
if (IS_ERR(iter))
return PTR_ERR(iter);
return ph->hops->iter_response_run(iter);
/*
* Do not cause whole protocol initialization failure when failing to
* get extended names for axes.
*/
ret = ph->hops->iter_response_run(iter);
if (ret)
dev_warn(ph->dev,
"Failed to get axes extended names for %s (ret:%d).\n",
s->name, ret);
return 0;
}
static int scmi_sensor_axis_description(const struct scmi_protocol_handle *ph,
@ -481,12 +512,15 @@ static int scmi_sensor_axis_description(const struct scmi_protocol_handle *ph,
{
int ret;
void *iter;
struct scmi_msg_sensor_axis_description_get *msg;
struct scmi_iterator_ops ops = {
.prepare_message = iter_axes_desc_prepare_message,
.update_state = iter_axes_desc_update_state,
.process_response = iter_axes_desc_process_response,
};
struct scmi_apriv apriv = {
.any_axes_support_extended_names = false,
.s = s,
};
s->axis = devm_kcalloc(ph->dev, s->num_axis,
sizeof(*s->axis), GFP_KERNEL);
@ -495,7 +529,8 @@ static int scmi_sensor_axis_description(const struct scmi_protocol_handle *ph,
iter = ph->hops->iter_response_init(ph, &ops, s->num_axis,
SENSOR_AXIS_DESCRIPTION_GET,
sizeof(*msg), s);
sizeof(struct scmi_msg_sensor_axis_description_get),
&apriv);
if (IS_ERR(iter))
return PTR_ERR(iter);
@ -503,7 +538,8 @@ static int scmi_sensor_axis_description(const struct scmi_protocol_handle *ph,
if (ret)
return ret;
if (PROTOCOL_REV_MAJOR(version) >= 0x3)
if (PROTOCOL_REV_MAJOR(version) >= 0x3 &&
apriv.any_axes_support_extended_names)
ret = scmi_sensor_axis_extended_names_get(ph, s);
return ret;
@ -598,7 +634,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

15
drivers/firmware/arm_scmi/voltage.c
View File

@ -180,7 +180,6 @@ static int scmi_voltage_levels_get(const struct scmi_protocol_handle *ph,
{
int ret;
void *iter;
struct scmi_msg_cmd_describe_levels *msg;
struct scmi_iterator_ops ops = {
.prepare_message = iter_volt_levels_prepare_message,
.update_state = iter_volt_levels_update_state,
@ -193,7 +192,8 @@ static int scmi_voltage_levels_get(const struct scmi_protocol_handle *ph,
iter = ph->hops->iter_response_init(ph, &ops, v->num_levels,
VOLTAGE_DESCRIBE_LEVELS,
sizeof(*msg), &vpriv);
sizeof(struct scmi_msg_cmd_describe_levels),
&vpriv);
if (IS_ERR(iter))
return PTR_ERR(iter);
@ -225,15 +225,14 @@ static int scmi_voltage_descriptors_get(const struct scmi_protocol_handle *ph,
/* Retrieve domain attributes at first ... */
put_unaligned_le32(dom, td->tx.buf);
ret = ph->xops->do_xfer(ph, td);
/* Skip domain on comms error */
if (ret)
if (ph->xops->do_xfer(ph, td))
continue;
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;
@ -249,12 +248,8 @@ static int scmi_voltage_descriptors_get(const struct scmi_protocol_handle *ph,
v->async_level_set = true;
}
ret = scmi_voltage_levels_get(ph, v);
/* Skip invalid voltage descriptors */
if (ret)
continue;
ph->xops->reset_rx_to_maxsz(ph, td);
scmi_voltage_levels_get(ph, v);
}
ph->xops->xfer_put(ph, td);

9
include/linux/scmi_protocol.h
View File

@ -13,8 +13,9 @@
#include <linux/notifier.h>
#include <linux/types.h>
#define SCMI_MAX_STR_SIZE 64
#define SCMI_MAX_NUM_RATES 16
#define SCMI_MAX_STR_SIZE 64
#define SCMI_SHORT_NAME_MAX_SIZE 16
#define SCMI_MAX_NUM_RATES 16
/**
* struct scmi_revision_info - version information structure
@ -36,8 +37,8 @@ struct scmi_revision_info {
u8 num_protocols;
u8 num_agents;
u32 impl_ver;
char vendor_id[SCMI_MAX_STR_SIZE];
char sub_vendor_id[SCMI_MAX_STR_SIZE];
char vendor_id[SCMI_SHORT_NAME_MAX_SIZE];
char sub_vendor_id[SCMI_SHORT_NAME_MAX_SIZE];
};
struct scmi_clock_info {