[PATCH v2 00/10] topology: decode: Various fixes
This series fixes various problems with topology decoding mechanism. Some of the problems were critical like improper memory management or infinite loops that were causing undefined behaviour or program crashes, while other resulted in losing some data during conversion.
Bugs found while testing with Intel SST topologies.
Changelog: v2: -Divide into more patches, critical fixes are in separate patches now -More specific descriptions -Fix a typo UML to UCM -Add error reporting in topology: decode: fix channel map memory allocation -Remove goto again in topology: Make buffer for saving dynamic size for better readability
Piotr Maziarz (10): topology: decode: Fix channel map memory allocation topology: decode: Fix infinite loop in decoding enum control topology: decode: Remove decoding values for enum control topology: decode: Add enum control texts as separate element topology: decode: Fix printing texts section topology: decode: Change declaration of enum decoding function topology: decode: Fix decoding PCM formats and rates topology: decode: Print sig_bits field in PCM capabilities section topology: decode: Add DAI name printing topology: Make buffer for saving dynamic size
src/topology/ctl.c | 51 ++++++++++++++++++++++------------------------- src/topology/dapm.c | 3 +-- src/topology/pcm.c | 11 +++++++--- src/topology/save.c | 34 ++++++++++++++++++++++++++----- src/topology/text.c | 2 +- src/topology/tplg_local.h | 2 +- 6 files changed, 64 insertions(+), 39 deletions(-)
Memory allocated on the stack was referenced outside of the function scope caused undefined behaviour.
Change-Id: Iac0bfe7dabfd59494b78afba17cf2d3e0b429fef Signed-off-by: Piotr Maziarz piotrx.maziarz@linux.intel.com --- src/topology/ctl.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/src/topology/ctl.c b/src/topology/ctl.c index 90241b6..6e6c1d1 100644 --- a/src/topology/ctl.c +++ b/src/topology/ctl.c @@ -1330,7 +1330,6 @@ int tplg_decode_control_enum1(snd_tplg_t *tplg, void *bin, size_t size) { struct snd_soc_tplg_enum_control *ec = bin; - struct snd_tplg_channel_map_template cmt; int i;
if (size < sizeof(*ec)) { @@ -1375,11 +1374,13 @@ int tplg_decode_control_enum1(snd_tplg_t *tplg, } }
- et->map = &cmt; - memset(&cmt, 0, sizeof(cmt)); - cmt.num_channels = ec->num_channels; - for (i = 0; i < cmt.num_channels; i++) { - struct snd_tplg_channel_elem *channel = &cmt.channel[i]; + et->map = tplg_calloc(heap, sizeof(struct snd_tplg_channel_map_template)); + if (!et->map) + return -ENOMEM; + et->map->num_channels = ec->num_channels; + for (i = 0; i < et->map->num_channels; i++) { + struct snd_tplg_channel_elem *channel = &et->map->channel[i]; + tplg_log(tplg, 'D', pos + ((void *)&ec->channel[i] - (void *)ec), "enum: channel size %d", ec->channel[i].size); channel->reg = ec->channel[i].reg;
Accessing memory outside of allocated boundaries caused segmentation fault.
Change-Id: Ide6b4d0e6ee7801bb7185e286be5d2fbf29695a6 --- src/topology/ctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/topology/ctl.c b/src/topology/ctl.c index 6e6c1d1..0aa49ab 100644 --- a/src/topology/ctl.c +++ b/src/topology/ctl.c @@ -1367,7 +1367,7 @@ int tplg_decode_control_enum1(snd_tplg_t *tplg, et->texts = tplg_calloc(heap, sizeof(char *) * ec->items); if (!et->texts) return -ENOMEM; - for (i = 0; ec->items; i++) { + for (i = 0; i < ec->items; i++) { unsigned int j = i * sizeof(int) * ENUM_VAL_SIZE; et->texts[i] = ec->texts[i]; et->values[i] = (int *)&ec->values[j];
Values have no representation in UCM file, therefore there is no need to populate them. Also memory for values wasn't allocated which was causing undefined behaviour.
Change-Id: I86b990d20808091eb61fb6710b35a0c1f1cbce10 --- src/topology/ctl.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/src/topology/ctl.c b/src/topology/ctl.c index 0aa49ab..02e482e 100644 --- a/src/topology/ctl.c +++ b/src/topology/ctl.c @@ -1367,11 +1367,8 @@ int tplg_decode_control_enum1(snd_tplg_t *tplg, et->texts = tplg_calloc(heap, sizeof(char *) * ec->items); if (!et->texts) return -ENOMEM; - for (i = 0; i < ec->items; i++) { - unsigned int j = i * sizeof(int) * ENUM_VAL_SIZE; + for (i = 0; i < ec->items; i++) et->texts[i] = ec->texts[i]; - et->values[i] = (int *)&ec->values[j]; - } }
et->map = tplg_calloc(heap, sizeof(struct snd_tplg_channel_map_template));
On 7/6/20 4:05 AM, Piotr Maziarz wrote:
Values have no representation in UCM file, therefore there is no need to
Sorry, I don't get the logical assertion here. What is the link or dependency between UCM and topology?
populate them. Also memory for values wasn't allocated which was causing undefined behaviour.
Change-Id: I86b990d20808091eb61fb6710b35a0c1f1cbce10
src/topology/ctl.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/src/topology/ctl.c b/src/topology/ctl.c index 0aa49ab..02e482e 100644 --- a/src/topology/ctl.c +++ b/src/topology/ctl.c @@ -1367,11 +1367,8 @@ int tplg_decode_control_enum1(snd_tplg_t *tplg, et->texts = tplg_calloc(heap, sizeof(char *) * ec->items); if (!et->texts) return -ENOMEM;
for (i = 0; i < ec->items; i++) {unsigned int j = i * sizeof(int) * ENUM_VAL_SIZE;
for (i = 0; i < ec->items; i++) et->texts[i] = ec->texts[i];
et->values[i] = (int *)&ec->values[j];}}
et->map = tplg_calloc(heap, sizeof(struct snd_tplg_channel_map_template));
Texts are separate sections that should referenced by enum control.
Change-Id: I7b97803da13478c642d003b78ce12be1eedbf802 --- src/topology/ctl.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/src/topology/ctl.c b/src/topology/ctl.c index 02e482e..1f39846 100644 --- a/src/topology/ctl.c +++ b/src/topology/ctl.c @@ -1088,11 +1088,19 @@ int tplg_add_enum(snd_tplg_t *tplg, struct snd_tplg_enum_template *enum_ctl, }
if (enum_ctl->texts != NULL) { + struct tplg_elem *texts = tplg_elem_new_common(tplg, NULL, + enum_ctl->hdr.name, SND_TPLG_TYPE_TEXT); + + texts->texts->num_items = num_items; for (i = 0; i < num_items; i++) { - if (enum_ctl->texts[i] != NULL) - snd_strlcpy(ec->texts[i], enum_ctl->texts[i], - SNDRV_CTL_ELEM_ID_NAME_MAXLEN); + if (!enum_ctl->texts[i]) + continue; + snd_strlcpy(ec->texts[i], enum_ctl->texts[i], + SNDRV_CTL_ELEM_ID_NAME_MAXLEN); + snd_strlcpy(texts->texts->items[i], enum_ctl->texts[i], + SNDRV_CTL_ELEM_ID_NAME_MAXLEN); } + tplg_ref_add(elem, SND_TPLG_TYPE_TEXT, enum_ctl->hdr.name); }
if (enum_ctl->values != NULL) {
Change-Id: Ie00ca47b18f527140b63b8c31e948f8850dfe11f --- src/topology/text.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/topology/text.c b/src/topology/text.c index 507c545..b899b28 100644 --- a/src/topology/text.c +++ b/src/topology/text.c @@ -103,7 +103,7 @@ int tplg_save_text(snd_tplg_t *tplg ATTRIBUTE_UNUSED, return 0; err = tplg_save_printf(dst, pfx, "'%s'.values [\n", elem->id); for (i = 0; err >= 0 && i < texts->num_items; i++) - err = tplg_save_printf(dst, pfx, "\t'%s'\n", texts->items[i][0]); + err = tplg_save_printf(dst, pfx, "\t'%s'\n", texts->items[i]); if (err >= 0) err = tplg_save_printf(dst, pfx, "]\n"); return err;
Size constraints are always checked before invoking tplg_decode_control_enum1. There is no need to validate it twice. Alos moved debug print about size to invoking function, since now it's it responsibility to check size.
Change-Id: I8bdd93cb1534ae9217aee87cb2b267e175aa483f --- src/topology/ctl.c | 19 +++++-------------- src/topology/dapm.c | 3 +-- src/topology/tplg_local.h | 2 +- 3 files changed, 7 insertions(+), 17 deletions(-)
diff --git a/src/topology/ctl.c b/src/topology/ctl.c index 1f39846..47db400 100644 --- a/src/topology/ctl.c +++ b/src/topology/ctl.c @@ -1335,22 +1335,10 @@ int tplg_decode_control_enum1(snd_tplg_t *tplg, struct list_head *heap, struct snd_tplg_enum_template *et, size_t pos, - void *bin, size_t size) + struct snd_soc_tplg_enum_control *ec) { - struct snd_soc_tplg_enum_control *ec = bin; int i;
- if (size < sizeof(*ec)) { - SNDERR("enum: small size %d", size); - return -EINVAL; - } - - tplg_log(tplg, 'D', pos, "enum: size %d private size %d", - ec->size, ec->priv.size); - if (size != ec->size + ec->priv.size) { - SNDERR("enum: unexpected element size %d", size); - return -EINVAL; - } if (ec->num_channels > SND_TPLG_MAX_CHAN || ec->num_channels > SND_SOC_TPLG_MAX_CHAN) { SNDERR("enum: unexpected channel count %d", ec->num_channels); @@ -1427,7 +1415,10 @@ next: return -EINVAL; }
- err = tplg_decode_control_enum1(tplg, &heap, &et, pos, bin, size); + tplg_log(tplg, 'D', pos, "enum: size %d private size %d", + ec->size, ec->priv.size); + + err = tplg_decode_control_enum1(tplg, &heap, &et, pos, ec); if (err >= 0) { t.enum_ctl = &et; err = snd_tplg_add_object(tplg, &t); diff --git a/src/topology/dapm.c b/src/topology/dapm.c index cd1a877..73a9390 100644 --- a/src/topology/dapm.c +++ b/src/topology/dapm.c @@ -972,8 +972,7 @@ next: err = -EINVAL; goto retval; } - err = tplg_decode_control_enum1(tplg, &heap, et, pos, - bin, size2); + err = tplg_decode_control_enum1(tplg, &heap, et, pos, ec); break; case SND_SOC_TPLG_TYPE_BYTES: bt = tplg_calloc(&heap, sizeof(*bt)); diff --git a/src/topology/tplg_local.h b/src/topology/tplg_local.h index 5ace0d1..acb01a8 100644 --- a/src/topology/tplg_local.h +++ b/src/topology/tplg_local.h @@ -398,7 +398,7 @@ int tplg_decode_control_enum1(snd_tplg_t *tplg, struct list_head *heap, struct snd_tplg_enum_template *et, size_t pos, - void *bin, size_t size); + struct snd_soc_tplg_enum_control *ec); int tplg_decode_control_enum(snd_tplg_t *tplg, size_t pos, struct snd_soc_tplg_hdr *hdr, void *bin, size_t size);
Not checking _LAST format and rate, which are valid indexes in arrays, makes data loss while converting binary to UCM.
Change-Id: I71ac42b65e8b1d79f2a061f805c62e29a758ad74 --- src/topology/pcm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/topology/pcm.c b/src/topology/pcm.c index b15b950..db40114 100644 --- a/src/topology/pcm.c +++ b/src/topology/pcm.c @@ -549,7 +549,7 @@ int tplg_save_stream_caps(snd_tplg_t *tplg ATTRIBUTE_UNUSED, if (err >= 0 && sc->formats) { err = tplg_save_printf(dst, pfx, "\tformats '"); first = 1; - for (i = 0; err >= 0 && i < SND_PCM_FORMAT_LAST; i++) { + for (i = 0; err >= 0 && i <= SND_PCM_FORMAT_LAST; i++) { if (sc->formats & (1ULL << i)) { s = snd_pcm_format_name(i); err = tplg_save_printf(dst, NULL, "%s%s", @@ -563,7 +563,7 @@ int tplg_save_stream_caps(snd_tplg_t *tplg ATTRIBUTE_UNUSED, if (err >= 0 && sc->rates) { err = tplg_save_printf(dst, pfx, "\trates '"); first = 1; - for (i = 0; err >= 0 && i < SND_PCM_RATE_LAST; i++) { + for (i = 0; err >= 0 && i <= SND_PCM_RATE_LAST; i++) { if (sc->rates & (1ULL << i)) { s = get_rate_name(i); err = tplg_save_printf(dst, NULL, "%s%s",
On 7/6/20 4:06 AM, Piotr Maziarz wrote:
Not checking _LAST format and rate, which are valid indexes in arrays, makes data loss while converting binary to UCM.
In the previous version you were decoding to UML. I presume this was typo, but now what does decoding a topology binary to UCM means? Did you really mean UCM as in Use Case Management (https://www.alsa-project.org/alsa-doc/alsa-lib/group__ucm.html)
Change-Id: I71ac42b65e8b1d79f2a061f805c62e29a758ad74
src/topology/pcm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/topology/pcm.c b/src/topology/pcm.c index b15b950..db40114 100644 --- a/src/topology/pcm.c +++ b/src/topology/pcm.c @@ -549,7 +549,7 @@ int tplg_save_stream_caps(snd_tplg_t *tplg ATTRIBUTE_UNUSED, if (err >= 0 && sc->formats) { err = tplg_save_printf(dst, pfx, "\tformats '"); first = 1;
for (i = 0; err >= 0 && i < SND_PCM_FORMAT_LAST; i++) {
for (i = 0; err >= 0 && i <= SND_PCM_FORMAT_LAST; i++) { if (sc->formats & (1ULL << i)) { s = snd_pcm_format_name(i); err = tplg_save_printf(dst, NULL, "%s%s",@@ -563,7 +563,7 @@ int tplg_save_stream_caps(snd_tplg_t *tplg ATTRIBUTE_UNUSED, if (err >= 0 && sc->rates) { err = tplg_save_printf(dst, pfx, "\trates '"); first = 1;
for (i = 0; err >= 0 && i < SND_PCM_RATE_LAST; i++) {
for (i = 0; err >= 0 && i <= SND_PCM_RATE_LAST; i++) { if (sc->rates & (1ULL << i)) { s = get_rate_name(i); err = tplg_save_printf(dst, NULL, "%s%s",
Not printing this field makes data loss while converting from binary to UCM.
Change-Id: Idba0c8bb537e7fc8af78f697781032005fa78d63 --- src/topology/pcm.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/topology/pcm.c b/src/topology/pcm.c index db40114..49c5eab 100644 --- a/src/topology/pcm.c +++ b/src/topology/pcm.c @@ -604,6 +604,9 @@ int tplg_save_stream_caps(snd_tplg_t *tplg ATTRIBUTE_UNUSED, if (err >= 0 && sc->buffer_size_max) err = tplg_save_printf(dst, pfx, "\tbuffer_size_max %u\n", sc->buffer_size_max); + if (err >= 0 && sc->sig_bits) + err = tplg_save_printf(dst, pfx, "\tsig_bits %u\n", + sc->sig_bits); if (err >= 0) err = tplg_save_printf(dst, pfx, "}\n"); return err;
DAI name is a part of topology binary. Not printing makes data loss while converting from binary to UCM.
Change-Id: I57307108b5e922d18db2f155b9237db3fe175d7f --- src/topology/pcm.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/topology/pcm.c b/src/topology/pcm.c index 49c5eab..5a54e15 100644 --- a/src/topology/pcm.c +++ b/src/topology/pcm.c @@ -781,7 +781,9 @@ int tplg_save_fe_dai(snd_tplg_t *tplg ATTRIBUTE_UNUSED, struct snd_soc_tplg_pcm *pcm = elem->pcm; int err = 0;
- if (pcm->dai_id > 0) + if (strlen(pcm->dai_name)) + err = tplg_save_printf(dst, pfx, "dai.'%s'.id %u\n", pcm->dai_name, pcm->dai_id); + else if (pcm->dai_id > 0) err = tplg_save_printf(dst, pfx, "dai.0.id %u\n", pcm->dai_id); return err; }
Some fields can exceed size limit, e.g. private data has no size restriction. Therefore it needs to be dynamically increased.
Change-Id: Ie274fd304eba5d600e198f4febbc6216f01b57e1 --- src/topology/save.c | 34 +++++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-)
diff --git a/src/topology/save.c b/src/topology/save.c index 4ecf86c..9c74735 100644 --- a/src/topology/save.c +++ b/src/topology/save.c @@ -19,22 +19,43 @@ #include "tplg_local.h"
#define SAVE_ALLOC_SHIFT (13) /* 8192 bytes */ +#define PRINT_BUF_SIZE (1024) +#define PRINT_BUF_SIZE_MAX (1024 * 1024)
int tplg_save_printf(char **dst, const char *pfx, const char *fmt, ...) { va_list va; - char buf[1024], *s; + char *buf, *s; size_t n, l, t, pl; + int ret = 0; + + buf = malloc(PRINT_BUF_SIZE); + if (!buf) + return -ENOMEM;
if (pfx == NULL) pfx = "";
va_start(va, fmt); - n = vsnprintf(buf, sizeof(buf), fmt, va); + n = vsnprintf(buf, PRINT_BUF_SIZE, fmt, va); va_end(va);
- if (n >= sizeof(buf)) - return -EOVERFLOW; + if (n >= PRINT_BUF_SIZE_MAX) { + ret = -EOVERFLOW; + goto end; + } + + if (n >= PRINT_BUF_SIZE) { + char *tmp = realloc(buf, n + 1); + if (!tmp) { + ret = -ENOMEM; + goto end; + } + buf = tmp; + va_start(va, fmt); + n = vsnprintf(buf, n + 1, fmt, va); + va_end(va); + }
pl = strlen(pfx); l = *dst ? strlen(*dst) : 0; @@ -47,7 +68,8 @@ int tplg_save_printf(char **dst, const char *pfx, const char *fmt, ...) if (s == NULL) { free(*dst); *dst = NULL; - return -ENOMEM; + ret = -ENOMEM; + goto end; } } else { s = *dst; @@ -57,6 +79,8 @@ int tplg_save_printf(char **dst, const char *pfx, const char *fmt, ...) strcpy(s + l, pfx); strcpy(s + l + pl, buf); *dst = s; +end: + free(buf); return 0; }
On 7/6/20 4:05 AM, Piotr Maziarz wrote:
This series fixes various problems with topology decoding mechanism. Some of the problems were critical like improper memory management or infinite loops that were causing undefined behaviour or program crashes, while other resulted in losing some data during conversion.
Bugs found while testing with Intel SST topologies.
Changelog: v2: -Divide into more patches, critical fixes are in separate patches now -More specific descriptions -Fix a typo UML to UCM
That fix makes it even more confusing, I get that a UCM file can set values for controls defined in a topology file, but 'decoding to UCM' leaves me wondering what you are referring to.
Also you may want to remove all the Gerrit ChangeId before sending to the mailing list.
-Add error reporting in topology: decode: fix channel map memory allocation -Remove goto again in topology: Make buffer for saving dynamic size for better readability
Piotr Maziarz (10): topology: decode: Fix channel map memory allocation topology: decode: Fix infinite loop in decoding enum control topology: decode: Remove decoding values for enum control topology: decode: Add enum control texts as separate element topology: decode: Fix printing texts section topology: decode: Change declaration of enum decoding function topology: decode: Fix decoding PCM formats and rates topology: decode: Print sig_bits field in PCM capabilities section topology: decode: Add DAI name printing topology: Make buffer for saving dynamic size
src/topology/ctl.c | 51 ++++++++++++++++++++++------------------------- src/topology/dapm.c | 3 +-- src/topology/pcm.c | 11 +++++++--- src/topology/save.c | 34 ++++++++++++++++++++++++++----- src/topology/text.c | 2 +- src/topology/tplg_local.h | 2 +- 6 files changed, 64 insertions(+), 39 deletions(-)
On 2020-07-06 22:01, Pierre-Louis Bossart wrote:
On 7/6/20 4:05 AM, Piotr Maziarz wrote:
This series fixes various problems with topology decoding mechanism. Some of the problems were critical like improper memory management or infinite loops that were causing undefined behaviour or program crashes, while other resulted in losing some data during conversion.
Bugs found while testing with Intel SST topologies.
Changelog: v2: -Divide into more patches, critical fixes are in separate patches now -More specific descriptions -Fix a typo UML to UCM
That fix makes it even more confusing, I get that a UCM file can set values for controls defined in a topology file, but 'decoding to UCM' leaves me wondering what you are referring to.
I meant standard ALSA configuration file format that is used also by UCM files. I'll change it for more clarity.
Also you may want to remove all the Gerrit ChangeId before sending to the mailing list.
-Add error reporting in topology: decode: fix channel map memory allocation -Remove goto again in topology: Make buffer for saving dynamic size for better readability
Piotr Maziarz (10): topology: decode: Fix channel map memory allocation topology: decode: Fix infinite loop in decoding enum control topology: decode: Remove decoding values for enum control topology: decode: Add enum control texts as separate element topology: decode: Fix printing texts section topology: decode: Change declaration of enum decoding function topology: decode: Fix decoding PCM formats and rates topology: decode: Print sig_bits field in PCM capabilities section topology: decode: Add DAI name printing topology: Make buffer for saving dynamic size
src/topology/ctl.c | 51 ++++++++++++++++++++++------------------------- src/topology/dapm.c | 3 +-- src/topology/pcm.c | 11 +++++++--- src/topology/save.c | 34 ++++++++++++++++++++++++++----- src/topology/text.c | 2 +- src/topology/tplg_local.h | 2 +- 6 files changed, 64 insertions(+), 39 deletions(-)
participants (2)
-
Pierre-Louis Bossart -
Piotr Maziarz