[PATCH] firmware: cs_dsp: test_control_parse: null-terminate test strings

[PATCH] firmware: cs_dsp: test_control_parse: null-terminate test strings

[Thomas Weißschuh]

The char pointers in 'struct cs_dsp_mock_coeff_def' are expected to
point to C strings. They need to be terminated by a null byte.
However the code does not allocate that trailing null byte and only
works if by chance the allocation is followed by such a null byte.

Refactor the repeated string allocation logic into a new helper which
makes sure the terminating null is always present.
It also makes the code more readable.

Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
Fixes: 83baecd92e7c ("firmware: cs_dsp: Add KUnit testing of control parsing")
Cc: stable@vger.kernel.org
---
 .../cirrus/test/cs_dsp_test_control_parse.c        | 51 ++++++++--------------
 1 file changed, 19 insertions(+), 32 deletions(-)

diff --git a/drivers/firmware/cirrus/test/cs_dsp_test_control_parse.c b/drivers/firmware/cirrus/test/cs_dsp_test_control_parse.c
index cb90964740ea351113dac274f0366de7cedfd3d1..942ba1af5e7c1e47e8a2fbe548a7993b94f96515 100644
--- a/drivers/firmware/cirrus/test/cs_dsp_test_control_parse.c
+++ b/drivers/firmware/cirrus/test/cs_dsp_test_control_parse.c
@@ -73,6 +73,18 @@ static const struct cs_dsp_mock_coeff_def mock_coeff_template = {
 	.length_bytes = 4,
 };
 
+static char *cs_dsp_ctl_alloc_test_string(struct kunit *test, char c, size_t len)
+{
+	char *str;
+
+	str = kunit_kmalloc(test, len + 1, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, str);
+	memset(str, c, len);
+	str[len] = '\0';
+
+	return str;
+}
+
 /* Algorithm info block without controls should load */
 static void cs_dsp_ctl_parse_no_coeffs(struct kunit *test)
 {
@@ -160,12 +172,8 @@ static void cs_dsp_ctl_parse_max_v1_name(struct kunit *test)
 	struct cs_dsp_mock_coeff_def def = mock_coeff_template;
 	struct cs_dsp_coeff_ctl *ctl;
 	struct firmware *wmfw;
-	char *name;
 
-	name = kunit_kzalloc(test, 256, GFP_KERNEL);
-	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, name);
-	memset(name, 'A', 255);
-	def.fullname = name;
+	def.fullname = cs_dsp_ctl_alloc_test_string(test, 'A', 255);
 
 	cs_dsp_mock_wmfw_start_alg_info_block(local->wmfw_builder,
 					      cs_dsp_ctl_parse_test_algs[0].id,
@@ -252,14 +260,9 @@ static void cs_dsp_ctl_parse_max_short_name(struct kunit *test)
 	struct cs_dsp_test_local *local = priv->local;
 	struct cs_dsp_mock_coeff_def def = mock_coeff_template;
 	struct cs_dsp_coeff_ctl *ctl;
-	char *name;
 	struct firmware *wmfw;
 
-	name = kunit_kmalloc(test, 255, GFP_KERNEL);
-	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, name);
-	memset(name, 'A', 255);
-
-	def.shortname = name;
+	def.shortname = cs_dsp_ctl_alloc_test_string(test, 'A', 255);
 
 	cs_dsp_mock_wmfw_start_alg_info_block(local->wmfw_builder,
 					      cs_dsp_ctl_parse_test_algs[0].id,
@@ -273,7 +276,7 @@ static void cs_dsp_ctl_parse_max_short_name(struct kunit *test)
 	ctl = list_first_entry_or_null(&priv->dsp->ctl_list, struct cs_dsp_coeff_ctl, list);
 	KUNIT_ASSERT_NOT_NULL(test, ctl);
 	KUNIT_EXPECT_EQ(test, ctl->subname_len, 255);
-	KUNIT_EXPECT_MEMEQ(test, ctl->subname, name, ctl->subname_len);
+	KUNIT_EXPECT_MEMEQ(test, ctl->subname, def.shortname, ctl->subname_len);
 	KUNIT_EXPECT_EQ(test, ctl->flags, def.flags);
 	KUNIT_EXPECT_EQ(test, ctl->type, def.type);
 	KUNIT_EXPECT_EQ(test, ctl->len, def.length_bytes);
@@ -323,12 +326,8 @@ static void cs_dsp_ctl_parse_with_max_fullname(struct kunit *test)
 	struct cs_dsp_mock_coeff_def def = mock_coeff_template;
 	struct cs_dsp_coeff_ctl *ctl;
 	struct firmware *wmfw;
-	char *fullname;
 
-	fullname = kunit_kmalloc(test, 255, GFP_KERNEL);
-	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, fullname);
-	memset(fullname, 'A', 255);
-	def.fullname = fullname;
+	def.fullname = cs_dsp_ctl_alloc_test_string(test, 'A', 255);
 
 	cs_dsp_mock_wmfw_start_alg_info_block(local->wmfw_builder,
 					      cs_dsp_ctl_parse_test_algs[0].id,
@@ -392,12 +391,8 @@ static void cs_dsp_ctl_parse_with_max_description(struct kunit *test)
 	struct cs_dsp_mock_coeff_def def = mock_coeff_template;
 	struct cs_dsp_coeff_ctl *ctl;
 	struct firmware *wmfw;
-	char *description;
 
-	description = kunit_kmalloc(test, 65535, GFP_KERNEL);
-	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, description);
-	memset(description, 'A', 65535);
-	def.description = description;
+	def.description = cs_dsp_ctl_alloc_test_string(test, 'A', 65535);
 
 	cs_dsp_mock_wmfw_start_alg_info_block(local->wmfw_builder,
 					      cs_dsp_ctl_parse_test_algs[0].id,
@@ -429,17 +424,9 @@ static void cs_dsp_ctl_parse_with_max_fullname_and_description(struct kunit *tes
 	struct cs_dsp_mock_coeff_def def = mock_coeff_template;
 	struct cs_dsp_coeff_ctl *ctl;
 	struct firmware *wmfw;
-	char *fullname, *description;
-
-	fullname = kunit_kmalloc(test, 255, GFP_KERNEL);
-	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, fullname);
-	memset(fullname, 'A', 255);
-	def.fullname = fullname;
 
-	description = kunit_kmalloc(test, 65535, GFP_KERNEL);
-	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, description);
-	memset(description, 'A', 65535);
-	def.description = description;
+	def.fullname = cs_dsp_ctl_alloc_test_string(test, 'A', 255);
+	def.description = cs_dsp_ctl_alloc_test_string(test, 'A', 65535);
 
 	cs_dsp_mock_wmfw_start_alg_info_block(local->wmfw_builder,
 					      cs_dsp_ctl_parse_test_algs[0].id,

---
base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
change-id: 20250211-cs_dsp-kunit-strings-f43e27078ed2

Best regards,
-- 
Thomas Weißschuh <thomas.weissschuh@linutronix.de>

Re: [PATCH] firmware: cs_dsp: test_control_parse: null-terminate test strings

[Richard Fitzgerald]

On 11/02/2025 3:00 pm, Thomas Weißschuh wrote:
> The char pointers in 'struct cs_dsp_mock_coeff_def' are expected to
> point to C strings. They need to be terminated by a null byte.
> However the code does not allocate that trailing null byte and only
> works if by chance the allocation is followed by such a null byte.
> 
> Refactor the repeated string allocation logic into a new helper which
> makes sure the terminating null is always present.
> It also makes the code more readable.
> 
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> Fixes: 83baecd92e7c ("firmware: cs_dsp: Add KUnit testing of control parsing")
> Cc: stable@vger.kernel.org
> ---
>   .../cirrus/test/cs_dsp_test_control_parse.c        | 51 ++++++++--------------
>   1 file changed, 19 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/firmware/cirrus/test/cs_dsp_test_control_parse.c b/drivers/firmware/cirrus/test/cs_dsp_test_control_parse.c
> index cb90964740ea351113dac274f0366de7cedfd3d1..942ba1af5e7c1e47e8a2fbe548a7993b94f96515 100644
> --- a/drivers/firmware/cirrus/test/cs_dsp_test_control_parse.c
> +++ b/drivers/firmware/cirrus/test/cs_dsp_test_control_parse.c
> @@ -73,6 +73,18 @@ static const struct cs_dsp_mock_coeff_def mock_coeff_template = {
>   	.length_bytes = 4,
>   };
>   
> +static char *cs_dsp_ctl_alloc_test_string(struct kunit *test, char c, size_t len)
> +{
> +	char *str;
> +
> +	str = kunit_kmalloc(test, len + 1, GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, str);
> +	memset(str, c, len);
> +	str[len] = '\0';
> +
> +	return str;
> +}
> +
>   /* Algorithm info block without controls should load */
>   static void cs_dsp_ctl_parse_no_coeffs(struct kunit *test)
>   {
> @@ -160,12 +172,8 @@ static void cs_dsp_ctl_parse_max_v1_name(struct kunit *test)
>   	struct cs_dsp_mock_coeff_def def = mock_coeff_template;
>   	struct cs_dsp_coeff_ctl *ctl;
>   	struct firmware *wmfw;
> -	char *name;
>   
> -	name = kunit_kzalloc(test, 256, GFP_KERNEL);

This allocates 256 bytes of zero-filled memory...

> -	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, name);
> -	memset(name, 'A', 255);

... and this fills the first 255 bytes, leaving the last byte still as
zero. So the string is zero-terminated. I don't see a problem here.

Just fix the other allocs to be kzalloc with the correct length?

Re: [PATCH] firmware: cs_dsp: test_control_parse: null-terminate test strings

[Thomas Weißschuh]

On Tue, Feb 11, 2025 at 03:10:38PM +0000, Richard Fitzgerald wrote:
> On 11/02/2025 3:00 pm, Thomas Weißschuh wrote:
> > The char pointers in 'struct cs_dsp_mock_coeff_def' are expected to
> > point to C strings. They need to be terminated by a null byte.
> > However the code does not allocate that trailing null byte and only
> > works if by chance the allocation is followed by such a null byte.
> > 
> > Refactor the repeated string allocation logic into a new helper which
> > makes sure the terminating null is always present.
> > It also makes the code more readable.
> > 
> > Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> > Fixes: 83baecd92e7c ("firmware: cs_dsp: Add KUnit testing of control parsing")
> > Cc: stable@vger.kernel.org
> > ---
> >   .../cirrus/test/cs_dsp_test_control_parse.c        | 51 ++++++++--------------
> >   1 file changed, 19 insertions(+), 32 deletions(-)
> > 
> > diff --git a/drivers/firmware/cirrus/test/cs_dsp_test_control_parse.c b/drivers/firmware/cirrus/test/cs_dsp_test_control_parse.c
> > index cb90964740ea351113dac274f0366de7cedfd3d1..942ba1af5e7c1e47e8a2fbe548a7993b94f96515 100644
> > --- a/drivers/firmware/cirrus/test/cs_dsp_test_control_parse.c
> > +++ b/drivers/firmware/cirrus/test/cs_dsp_test_control_parse.c
> > @@ -73,6 +73,18 @@ static const struct cs_dsp_mock_coeff_def mock_coeff_template = {
> >   	.length_bytes = 4,
> >   };
> > +static char *cs_dsp_ctl_alloc_test_string(struct kunit *test, char c, size_t len)
> > +{
> > +	char *str;
> > +
> > +	str = kunit_kmalloc(test, len + 1, GFP_KERNEL);
> > +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, str);
> > +	memset(str, c, len);
> > +	str[len] = '\0';
> > +
> > +	return str;
> > +}
> > +
> >   /* Algorithm info block without controls should load */
> >   static void cs_dsp_ctl_parse_no_coeffs(struct kunit *test)
> >   {
> > @@ -160,12 +172,8 @@ static void cs_dsp_ctl_parse_max_v1_name(struct kunit *test)
> >   	struct cs_dsp_mock_coeff_def def = mock_coeff_template;
> >   	struct cs_dsp_coeff_ctl *ctl;
> >   	struct firmware *wmfw;
> > -	char *name;
> > -	name = kunit_kzalloc(test, 256, GFP_KERNEL);
> 
> This allocates 256 bytes of zero-filled memory...
> 
> > -	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, name);
> > -	memset(name, 'A', 255);
> 
> ... and this fills the first 255 bytes, leaving the last byte still as
> zero. So the string is zero-terminated. I don't see a problem here.

This single instance it is indeed correct.
In all other five it's broken.

> Just fix the other allocs to be kzalloc with the correct length?

If you prefer that, sure I can change it.

Personally I like the helper much better. One does not have to look at a
dense block of code to see what the actual intention is.
Assuming the location in cs_dsp_ctl_parse_max_v1_name() was fixed when
some breakage was observed, with a helper it would have been fixed for
all locations and not crept into upstream code.


Thomas

Re: [PATCH] firmware: cs_dsp: test_control_parse: null-terminate test strings

[Richard Fitzgerald]

On 11/02/2025 3:21 pm, Thomas Weißschuh wrote:
> On Tue, Feb 11, 2025 at 03:10:38PM +0000, Richard Fitzgerald wrote:
>> On 11/02/2025 3:00 pm, Thomas Weißschuh wrote:
>>> The char pointers in 'struct cs_dsp_mock_coeff_def' are expected to
>>> point to C strings. They need to be terminated by a null byte.
>>> However the code does not allocate that trailing null byte and only
>>> works if by chance the allocation is followed by such a null byte.
>>>
>>> Refactor the repeated string allocation logic into a new helper which
>>> makes sure the terminating null is always present.
>>> It also makes the code more readable.
>>>
>>> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
>>> Fixes: 83baecd92e7c ("firmware: cs_dsp: Add KUnit testing of control parsing")
>>> Cc: stable@vger.kernel.org
>>> ---
>>>    .../cirrus/test/cs_dsp_test_control_parse.c        | 51 ++++++++--------------
>>>    1 file changed, 19 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/drivers/firmware/cirrus/test/cs_dsp_test_control_parse.c b/drivers/firmware/cirrus/test/cs_dsp_test_control_parse.c
>>> index cb90964740ea351113dac274f0366de7cedfd3d1..942ba1af5e7c1e47e8a2fbe548a7993b94f96515 100644
>>> --- a/drivers/firmware/cirrus/test/cs_dsp_test_control_parse.c
>>> +++ b/drivers/firmware/cirrus/test/cs_dsp_test_control_parse.c
>>> @@ -73,6 +73,18 @@ static const struct cs_dsp_mock_coeff_def mock_coeff_template = {
>>>    	.length_bytes = 4,
>>>    };
>>> +static char *cs_dsp_ctl_alloc_test_string(struct kunit *test, char c, size_t len)
>>> +{
>>> +	char *str;
>>> +
>>> +	str = kunit_kmalloc(test, len + 1, GFP_KERNEL);
>>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, str);
>>> +	memset(str, c, len);
>>> +	str[len] = '\0';
>>> +
>>> +	return str;
>>> +}
>>> +
>>>    /* Algorithm info block without controls should load */
>>>    static void cs_dsp_ctl_parse_no_coeffs(struct kunit *test)
>>>    {
>>> @@ -160,12 +172,8 @@ static void cs_dsp_ctl_parse_max_v1_name(struct kunit *test)
>>>    	struct cs_dsp_mock_coeff_def def = mock_coeff_template;
>>>    	struct cs_dsp_coeff_ctl *ctl;
>>>    	struct firmware *wmfw;
>>> -	char *name;
>>> -	name = kunit_kzalloc(test, 256, GFP_KERNEL);
>>
>> This allocates 256 bytes of zero-filled memory...
>>
>>> -	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, name);
>>> -	memset(name, 'A', 255);
>>
>> ... and this fills the first 255 bytes, leaving the last byte still as
>> zero. So the string is zero-terminated. I don't see a problem here.
> 
> This single instance it is indeed correct.
> In all other five it's broken.
> 
>> Just fix the other allocs to be kzalloc with the correct length?
> 
> If you prefer that, sure I can change it.
> 
> Personally I like the helper much better. One does not have to look at a
> dense block of code to see what the actual intention is.
> Assuming the location in cs_dsp_ctl_parse_max_v1_name() was fixed when
> some breakage was observed, with a helper it would have been fixed for
> all locations and not crept into upstream code.
> 
> 
> Thomas

Actually I try to avoid helpers in tests. The trouble is that the
function name _sounds_ like they do what you want, but you have to go
and look at the helper to see what it _really_ does. More helpers means
it's more difficult to review whether the test case does what it should
do. I went through this early on where I had a lot more helpers for all
that similar code in the test cases, and I found that I had test cases
that were wrong but that was difficult to see because the test procedure
was hidden across a lot of helper functions.

For these strings there are some cases where it's important for the
string to NOT have a NULL terminator in the firmware file so I'm a bit
concerned that it was intended to create a string without a terminator
and the bug is failure to handle this. But I'm really busy right now
with other things so haven't got much time to look at this, If you
and Mark want to take this patch to prevent the string overruns I'll
come back when I have time and do a follow-up patch if I think the
test framework should handle the non-terminated strings.

Re: [PATCH] firmware: cs_dsp: test_control_parse: null-terminate test strings

[Richard Fitzgerald]

On 11/02/2025 3:00 pm, Thomas Weißschuh wrote:
> The char pointers in 'struct cs_dsp_mock_coeff_def' are expected to
> point to C strings. They need to be terminated by a null byte.
> However the code does not allocate that trailing null byte and only
> works if by chance the allocation is followed by such a null byte.
> 
> Refactor the repeated string allocation logic into a new helper which
> makes sure the terminating null is always present.
> It also makes the code more readable.
> 
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> Fixes: 83baecd92e7c ("firmware: cs_dsp: Add KUnit testing of control parsing")
> Cc: stable@vger.kernel.org
> ---
>   .../cirrus/test/cs_dsp_test_control_parse.c        | 51 ++++++++--------------
>   1 file changed, 19 insertions(+), 32 deletions(-)
> 

Reviewed-by: Richard Fitzgerald <rf@opensource.cirrus.com>
Tested-by: Richard Fitzgerald <rf@opensource.cirrus.com>

Re: [PATCH] firmware: cs_dsp: test_control_parse: null-terminate test strings

[Mark Brown]

On Tue, 11 Feb 2025 16:00:02 +0100, Thomas Weißschuh wrote:
> The char pointers in 'struct cs_dsp_mock_coeff_def' are expected to
> point to C strings. They need to be terminated by a null byte.
> However the code does not allocate that trailing null byte and only
> works if by chance the allocation is followed by such a null byte.
> 
> Refactor the repeated string allocation logic into a new helper which
> makes sure the terminating null is always present.
> It also makes the code more readable.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] firmware: cs_dsp: test_control_parse: null-terminate test strings
      commit: 42ae6e2559e63c2d4096b698cd47aaeb974436df

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark