linux-kernel - Re: [PATCH] firmware: cs_dsp: test_control

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250211161448-c6560879-7bc9-4fe1-a9cf-713f029c1ee7@linutronix.de>
Date: Tue, 11 Feb 2025 16:21:25 +0100
From: Thomas Weißschuh <thomas.weissschuh@...utronix.de>
To: Richard Fitzgerald <rf@...nsource.cirrus.com>
Cc: Simon Trimmer <simont@...nsource.cirrus.com>, 
	Charles Keepax <ckeepax@...nsource.cirrus.com>, Mark Brown <broonie@...nel.org>, patches@...nsource.cirrus.com, 
	linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH] firmware: cs_dsp: test_control_parse: null-terminate
 test strings

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@...utronix.de>
> > Fixes: 83baecd92e7c ("firmware: cs_dsp: Add KUnit testing of control parsing")
> > Cc: stable@...r.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