From 62412a9357b16a4e39dc582deb2e2a682b92524c Mon Sep 17 00:00:00 2001 From: Richard Fitzgerald Date: Tue, 2 Jul 2024 12:08:09 +0100 Subject: firmware: cs_dsp: Don't allow writes to read-only controls Add a check to cs_dsp_coeff_write_ctrl() to abort if the control is not writeable. The cs_dsp code originated as an ASoC driver (wm_adsp) where all controls were exported as ALSA controls. It relied on ALSA to enforce the read-only permission. Now that the code has been separated from ALSA/ASoC it must perform its own permission check. This isn't currently causing any problems so there shouldn't be any need to backport this. If the client of cs_dsp exposes the control as an ALSA control, it should set permissions on that ALSA control to protect it. The few uses of cs_dsp_coeff_write_ctrl() inside drivers are for writable controls. Signed-off-by: Richard Fitzgerald Link: https://patch.msgid.link/20240702110809.16836-1-rf@opensource.cirrus.com Signed-off-by: Mark Brown --- drivers/firmware/cirrus/cs_dsp.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'drivers/firmware') diff --git a/drivers/firmware/cirrus/cs_dsp.c b/drivers/firmware/cirrus/cs_dsp.c index 0d139e4de37c..d2aa0980ed78 100644 --- a/drivers/firmware/cirrus/cs_dsp.c +++ b/drivers/firmware/cirrus/cs_dsp.c @@ -796,6 +796,9 @@ int cs_dsp_coeff_write_ctrl(struct cs_dsp_coeff_ctl *ctl, lockdep_assert_held(&ctl->dsp->pwr_lock); + if (ctl->flags && !(ctl->flags & WMFW_CTL_FLAG_WRITEABLE)) + return -EPERM; + if (len + off * sizeof(u32) > ctl->len) return -EINVAL; -- cgit v1.2.3 From bff92858dfb9897e4c06c11aab0322e1aab822f7 Mon Sep 17 00:00:00 2001 From: Richard Fitzgerald Date: Wed, 10 Jul 2024 11:36:37 +0100 Subject: firmware: cs_dsp: Don't allocate temporary buffer for info text Don't allocate a temporary buffer to hold a NUL-terminated copy of the NAME/INFO string from the wmfw/bin. It can be printed directly to the log. Also limit the maximum number of characters that will be logged from this string. The NAME/INFO blocks in the firmware files are an array of characters with a length, not a NUL-terminated C string. The original code allocated a temporary buffer to make a NUL-terminated copy of the string and then passed that to dev_info(). There's no need for this: printf formatting can use "%.*s" to print a character array of a given length. Signed-off-by: Richard Fitzgerald Reviewed-by: Charles Keepax Link: https://lore.kernel.org/r/20240710103640.78197-2-rf@opensource.cirrus.com Signed-off-by: Mark Brown --- drivers/firmware/cirrus/cs_dsp.c | 35 +++++++---------------------------- 1 file changed, 7 insertions(+), 28 deletions(-) (limited to 'drivers/firmware') diff --git a/drivers/firmware/cirrus/cs_dsp.c b/drivers/firmware/cirrus/cs_dsp.c index 8a347b938406..9bd3f78396ea 100644 --- a/drivers/firmware/cirrus/cs_dsp.c +++ b/drivers/firmware/cirrus/cs_dsp.c @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -1470,7 +1471,6 @@ static int cs_dsp_load(struct cs_dsp *dsp, const struct firmware *firmware, const struct wmfw_region *region; const struct cs_dsp_region *mem; const char *region_name; - char *text = NULL; struct cs_dsp_buf *buf; unsigned int reg; int regions = 0; @@ -1542,15 +1542,15 @@ static int cs_dsp_load(struct cs_dsp *dsp, const struct firmware *firmware, region_name = "Unknown"; reg = 0; - text = NULL; offset = le32_to_cpu(region->offset) & 0xffffff; type = be32_to_cpu(region->type) & 0xff; switch (type) { + case WMFW_INFO_TEXT: case WMFW_NAME_TEXT: - region_name = "Firmware name"; - text = kzalloc(le32_to_cpu(region->len) + 1, - GFP_KERNEL); + region_name = "Info/Name"; + cs_dsp_info(dsp, "%s: %.*s\n", file, + min(le32_to_cpu(region->len), 100), region->data); break; case WMFW_ALGORITHM_DATA: region_name = "Algorithm"; @@ -1558,11 +1558,6 @@ static int cs_dsp_load(struct cs_dsp *dsp, const struct firmware *firmware, if (ret != 0) goto out_fw; break; - case WMFW_INFO_TEXT: - region_name = "Information"; - text = kzalloc(le32_to_cpu(region->len) + 1, - GFP_KERNEL); - break; case WMFW_ABSOLUTE: region_name = "Absolute"; reg = offset; @@ -1596,13 +1591,6 @@ static int cs_dsp_load(struct cs_dsp *dsp, const struct firmware *firmware, regions, le32_to_cpu(region->len), offset, region_name); - if (text) { - memcpy(text, region->data, le32_to_cpu(region->len)); - cs_dsp_info(dsp, "%s: %s\n", file, text); - kfree(text); - text = NULL; - } - if (reg) { buf = cs_dsp_buf_alloc(region->data, le32_to_cpu(region->len), @@ -1644,7 +1632,6 @@ static int cs_dsp_load(struct cs_dsp *dsp, const struct firmware *firmware, out_fw: regmap_async_complete(regmap); cs_dsp_buf_free(&buf_list); - kfree(text); if (ret == -EOVERFLOW) cs_dsp_err(dsp, "%s: file content overflows file data\n", file); @@ -2177,7 +2164,6 @@ static int cs_dsp_load_coeff(struct cs_dsp *dsp, const struct firmware *firmware struct cs_dsp_alg_region *alg_region; const char *region_name; int ret, pos, blocks, type, offset, reg, version; - char *text = NULL; struct cs_dsp_buf *buf; if (!firmware) @@ -2246,7 +2232,8 @@ static int cs_dsp_load_coeff(struct cs_dsp *dsp, const struct firmware *firmware region_name = "Unknown"; switch (type) { case (WMFW_NAME_TEXT << 8): - text = kzalloc(le32_to_cpu(blk->len) + 1, GFP_KERNEL); + cs_dsp_info(dsp, "%s: %.*s\n", dsp->fw_name, + min(le32_to_cpu(blk->len), 100), blk->data); break; case (WMFW_INFO_TEXT << 8): case (WMFW_METADATA << 8): @@ -2318,13 +2305,6 @@ static int cs_dsp_load_coeff(struct cs_dsp *dsp, const struct firmware *firmware break; } - if (text) { - memcpy(text, blk->data, le32_to_cpu(blk->len)); - cs_dsp_info(dsp, "%s: %s\n", dsp->fw_name, text); - kfree(text); - text = NULL; - } - if (reg) { buf = cs_dsp_buf_alloc(blk->data, le32_to_cpu(blk->len), @@ -2364,7 +2344,6 @@ static int cs_dsp_load_coeff(struct cs_dsp *dsp, const struct firmware *firmware out_fw: regmap_async_complete(regmap); cs_dsp_buf_free(&buf_list); - kfree(text); if (ret == -EOVERFLOW) cs_dsp_err(dsp, "%s: file content overflows file data\n", file); -- cgit v1.2.3 From 584e86e14c59d36688633002613792923620d8c0 Mon Sep 17 00:00:00 2001 From: Richard Fitzgerald Date: Wed, 10 Jul 2024 11:36:38 +0100 Subject: firmware: cs_dsp: Make wmfw and bin filename arguments const char * The wmfw_filename and bin_filename strings passed into cs_dsp_power_up() and cs_dsp_adsp1_power_up() should be const char *. Signed-off-by: Richard Fitzgerald Reviewed-by: Charles Keepax Link: https://lore.kernel.org/r/20240710103640.78197-3-rf@opensource.cirrus.com Signed-off-by: Mark Brown --- drivers/firmware/cirrus/cs_dsp.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'drivers/firmware') diff --git a/drivers/firmware/cirrus/cs_dsp.c b/drivers/firmware/cirrus/cs_dsp.c index 9bd3f78396ea..fda6faa8c835 100644 --- a/drivers/firmware/cirrus/cs_dsp.c +++ b/drivers/firmware/cirrus/cs_dsp.c @@ -2410,8 +2410,8 @@ EXPORT_SYMBOL_NS_GPL(cs_dsp_adsp1_init, FW_CS_DSP); * Return: Zero for success, a negative number on error. */ int cs_dsp_adsp1_power_up(struct cs_dsp *dsp, - const struct firmware *wmfw_firmware, char *wmfw_filename, - const struct firmware *coeff_firmware, char *coeff_filename, + const struct firmware *wmfw_firmware, const char *wmfw_filename, + const struct firmware *coeff_firmware, const char *coeff_filename, const char *fw_name) { unsigned int val; @@ -2704,8 +2704,8 @@ static void cs_dsp_halo_stop_watchdog(struct cs_dsp *dsp) * Return: Zero for success, a negative number on error. */ int cs_dsp_power_up(struct cs_dsp *dsp, - const struct firmware *wmfw_firmware, char *wmfw_filename, - const struct firmware *coeff_firmware, char *coeff_filename, + const struct firmware *wmfw_firmware, const char *wmfw_filename, + const struct firmware *coeff_firmware, const char *coeff_filename, const char *fw_name) { int ret; -- cgit v1.2.3 From a4939119055d5c97aff2cea040f1af8c4606bfa1 Mon Sep 17 00:00:00 2001 From: Richard Fitzgerald Date: Wed, 10 Jul 2024 11:36:39 +0100 Subject: firmware: cs_dsp: Clarify wmfw format version log message Change the log message of the wmfw format version to include the file name, and change the message to say "format" instead of "Firmware version". Merge this with the message that logs the timestamp. The wmfw format version is information that is useful to have logged because the behaviour of firmware controls depends on the wmfw format. So "unexpected" behaviour could be caused by having expectations based on one format of wmfw when a different format has been loaded. But the original message was confusing. It reported the file format version but didn't actually log the name of the file it referred to. It also called it "Firmware version", which is confusing when a later message also logs a firmware version that is the version of the actual firmware within the wmfw. The logging of the firmware timestamp has been merged into this. That was originally a dbg-only message, but as we are already logging a line of info, we might as well add a few extra characters to log the timestamp. The timestamp is now logged in hexadecimal - it's not particularly useful as a decimal value. Signed-off-by: Richard Fitzgerald Reviewed-by: Charles Keepax Link: https://lore.kernel.org/r/20240710103640.78197-4-rf@opensource.cirrus.com Signed-off-by: Mark Brown --- drivers/firmware/cirrus/cs_dsp.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'drivers/firmware') diff --git a/drivers/firmware/cirrus/cs_dsp.c b/drivers/firmware/cirrus/cs_dsp.c index fda6faa8c835..865db8defc56 100644 --- a/drivers/firmware/cirrus/cs_dsp.c +++ b/drivers/firmware/cirrus/cs_dsp.c @@ -1499,7 +1499,6 @@ static int cs_dsp_load(struct cs_dsp *dsp, const struct firmware *firmware, goto out_fw; } - cs_dsp_info(dsp, "Firmware version: %d\n", header->ver); dsp->fw_ver = header->ver; if (header->core != dsp->type) { @@ -1523,8 +1522,8 @@ static int cs_dsp_load(struct cs_dsp *dsp, const struct firmware *firmware, goto out_fw; } - cs_dsp_dbg(dsp, "%s: timestamp %llu\n", file, - le64_to_cpu(footer->timestamp)); + cs_dsp_info(dsp, "%s: format %d timestamp %#llx\n", file, header->ver, + le64_to_cpu(footer->timestamp)); while (pos < firmware->size) { /* Is there enough data for a complete block header? */ -- cgit v1.2.3 From dc0e5ca8856dc6a97e3b117879dfb2b52bda06b6 Mon Sep 17 00:00:00 2001 From: Richard Fitzgerald Date: Wed, 10 Jul 2024 11:36:40 +0100 Subject: firmware: cs_dsp: Rename fw_ver to wmfw_ver Rename the confusingly named struct member fw_ver to wmfw_ver. It contains the wmfw format version of the loaded wmfw file. This commit also contains an update to wm_adsp for the new name. Signed-off-by: Richard Fitzgerald Reviewed-by: Charles Keepax Link: https://lore.kernel.org/r/20240710103640.78197-5-rf@opensource.cirrus.com Signed-off-by: Mark Brown --- drivers/firmware/cirrus/cs_dsp.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) (limited to 'drivers/firmware') diff --git a/drivers/firmware/cirrus/cs_dsp.c b/drivers/firmware/cirrus/cs_dsp.c index 865db8defc56..66c2f09b0be7 100644 --- a/drivers/firmware/cirrus/cs_dsp.c +++ b/drivers/firmware/cirrus/cs_dsp.c @@ -1048,7 +1048,7 @@ static int cs_dsp_create_control(struct cs_dsp *dsp, ctl->fw_name = dsp->fw_name; ctl->alg_region = *alg_region; - if (subname && dsp->fw_ver >= 2) { + if (subname && dsp->wmfw_ver >= 2) { ctl->subname_len = subname_len; ctl->subname = kasprintf(GFP_KERNEL, "%.*s", subname_len, subname); if (!ctl->subname) { @@ -1175,7 +1175,7 @@ static int cs_dsp_coeff_parse_alg(struct cs_dsp *dsp, raw = (const struct wmfw_adsp_alg_data *)region->data; - switch (dsp->fw_ver) { + switch (dsp->wmfw_ver) { case 0: case 1: if (sizeof(*raw) > data_len) @@ -1252,7 +1252,7 @@ static int cs_dsp_coeff_parse_coeff(struct cs_dsp *dsp, blk->offset = le16_to_cpu(raw->hdr.offset); blk->mem_type = le16_to_cpu(raw->hdr.type); - switch (dsp->fw_ver) { + switch (dsp->wmfw_ver) { case 0: case 1: if (sizeof(*raw) > (data_len - pos)) @@ -1499,7 +1499,7 @@ static int cs_dsp_load(struct cs_dsp *dsp, const struct firmware *firmware, goto out_fw; } - dsp->fw_ver = header->ver; + dsp->wmfw_ver = header->ver; if (header->core != dsp->type) { cs_dsp_err(dsp, "%s: invalid core %d != %d\n", @@ -1779,7 +1779,7 @@ static struct cs_dsp_alg_region *cs_dsp_create_region(struct cs_dsp *dsp, list_add_tail(&alg_region->list, &dsp->alg_regions); - if (dsp->fw_ver > 0) + if (dsp->wmfw_ver > 0) cs_dsp_ctl_fixup_base(dsp, alg_region); return alg_region; @@ -1902,7 +1902,7 @@ static int cs_dsp_adsp1_setup_algs(struct cs_dsp *dsp) ret = PTR_ERR(alg_region); goto out; } - if (dsp->fw_ver == 0) { + if (dsp->wmfw_ver == 0) { if (i + 1 < n_algs) { len = be32_to_cpu(adsp1_alg[i + 1].dm); len -= be32_to_cpu(adsp1_alg[i].dm); @@ -1924,7 +1924,7 @@ static int cs_dsp_adsp1_setup_algs(struct cs_dsp *dsp) ret = PTR_ERR(alg_region); goto out; } - if (dsp->fw_ver == 0) { + if (dsp->wmfw_ver == 0) { if (i + 1 < n_algs) { len = be32_to_cpu(adsp1_alg[i + 1].zm); len -= be32_to_cpu(adsp1_alg[i].zm); @@ -2015,7 +2015,7 @@ static int cs_dsp_adsp2_setup_algs(struct cs_dsp *dsp) ret = PTR_ERR(alg_region); goto out; } - if (dsp->fw_ver == 0) { + if (dsp->wmfw_ver == 0) { if (i + 1 < n_algs) { len = be32_to_cpu(adsp2_alg[i + 1].xm); len -= be32_to_cpu(adsp2_alg[i].xm); @@ -2037,7 +2037,7 @@ static int cs_dsp_adsp2_setup_algs(struct cs_dsp *dsp) ret = PTR_ERR(alg_region); goto out; } - if (dsp->fw_ver == 0) { + if (dsp->wmfw_ver == 0) { if (i + 1 < n_algs) { len = be32_to_cpu(adsp2_alg[i + 1].ym); len -= be32_to_cpu(adsp2_alg[i].ym); @@ -2059,7 +2059,7 @@ static int cs_dsp_adsp2_setup_algs(struct cs_dsp *dsp) ret = PTR_ERR(alg_region); goto out; } - if (dsp->fw_ver == 0) { + if (dsp->wmfw_ver == 0) { if (i + 1 < n_algs) { len = be32_to_cpu(adsp2_alg[i + 1].zm); len -= be32_to_cpu(adsp2_alg[i].zm); -- cgit v1.2.3