[PATCH] ALSA: control: Verify put() result when in debug mode

[PATCH] ALSA: control: Verify put() result when in debug mode

[Cezary Rojewski]

The put() operation is expected to return:
1) 0 on success if no changes were made
2) 1 on success if changes were made
3) error code otherwise

Currently 2) is usually ignored when writing control-operations. While
forcing compliance is not an option right now, make it easier for
developers to adhere to the expectations and notice problems by logging
them when CONFIG_SND_CTL_DEBUG is enabled.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/core/control.c | 35 +++++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index 9c3fd5113a61..257976efece1 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1264,6 +1264,32 @@ static int snd_ctl_elem_read_user(struct snd_card *card,
 	return result;
 }
 
+__maybe_unused
+static int snd_ctl_write_verify(struct snd_kcontrol *kctl, struct snd_ctl_elem_value *control)
+{
+	struct snd_ctl_elem_value original;
+	int ret, retcmp;
+
+	ret = kctl->get(kctl, &original);
+	if (ret)
+		return ret;
+
+	ret = kctl->put(kctl, control);
+	if (ret < 0)
+		return ret;
+
+	retcmp = memcmp(&original.value.bytes.data[0], &control->value.bytes.data[0],
+			sizeof(original.value.bytes.data[0]));
+	if (retcmp)
+		retcmp = 1;
+
+	if (retcmp == ret)
+		pr_info("kctl->put() returned the expected value of '%d'\n", ret);
+	else
+		pr_warn("expected kctl->put() to return '%d' but got '%d'\n", ret, retcmp);
+	return ret;
+}
+
 static int snd_ctl_elem_write(struct snd_card *card, struct snd_ctl_file *file,
 			      struct snd_ctl_elem_value *control)
 {
@@ -1299,8 +1325,13 @@ static int snd_ctl_elem_write(struct snd_card *card, struct snd_ctl_file *file,
 			result = sanity_check_input_values(card, control, &info,
 							   false);
 	}
-	if (!result)
-		result = kctl->put(kctl, control);
+
+	if (!result) {
+		if (IS_ENABLED(CONFIG_SND_CTL_DEBUG))
+			result = snd_ctl_write_verify(kctl, control);
+		else
+			result = kctl->put(kctl, control);
+	}
 	if (result < 0) {
 		up_write(&card->controls_rwsem);
 		return result;
-- 
2.34.1

Re: [PATCH] ALSA: control: Verify put() result when in debug mode

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1377 bytes --]

On Fri, Jan 30, 2026 at 02:55:41PM +0100, Cezary Rojewski wrote:
> The put() operation is expected to return:
> 1) 0 on success if no changes were made
> 2) 1 on success if changes were made
> 3) error code otherwise
> 
> Currently 2) is usually ignored when writing control-operations. While
> forcing compliance is not an option right now, make it easier for
> developers to adhere to the expectations and notice problems by logging
> them when CONFIG_SND_CTL_DEBUG is enabled.

Makes sense to me, although I fear it's about as likely that people
enable this config as that they run mixer-test it's reasonable to be
helpful.

> +	struct snd_ctl_elem_value original;

> +	ret = kctl->get(kctl, &original);
> +	if (ret)
> +		return ret;

> +	retcmp = memcmp(&original.value.bytes.data[0], &control->value.bytes.data[0],
> +			sizeof(original.value.bytes.data[0]));
> +	if (retcmp)
> +		retcmp = 1;

original was just allocated from the stack so who knows what values it
had originally, and the get() is only going to write the part of the
value that has data since the normal get() path has a memset() in the
core.  Similarly with the new value coming in from userspace there's no
requirement for userspace to set anything that isn't part of the value
being written to any particular value.  This means we're liable to get
spurious mismatches.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ALSA: control: Verify put() result when in debug mode

[Takashi Iwai]

On Fri, 30 Jan 2026 15:12:58 +0100,
Mark Brown wrote:
> > +	struct snd_ctl_elem_value original;
> 
> > +	ret = kctl->get(kctl, &original);
> > +	if (ret)
> > +		return ret;
> 
> > +	retcmp = memcmp(&original.value.bytes.data[0], &control->value.bytes.data[0],
> > +			sizeof(original.value.bytes.data[0]));
> > +	if (retcmp)
> > +		retcmp = 1;
> 
> original was just allocated from the stack so who knows what values it
> had originally, and the get() is only going to write the part of the
> value that has data since the normal get() path has a memset() in the
> core.  Similarly with the new value coming in from userspace there's no
> requirement for userspace to set anything that isn't part of the value
> being written to any particular value.  This means we're liable to get
> spurious mismatches.

Yes, and if I understand correctly, the above memcmpy() just compare
the single byte from original and the result?  Then it'll lead to
false-positive outputs.

We'll need to query the control info and check the relevant values
for each info->type and count.

Also, better to name it snd_ctl_put_verify() instead of
snd_ctl_write_verify(); it's an equivalent with snd_ctl_put() having
an additional verification.


thanks,

Takashi

Re: [PATCH] ALSA: control: Verify put() result when in debug mode

[Cezary Rojewski]

On 2026-01-30 3:12 PM, Mark Brown wrote:
> On Fri, Jan 30, 2026 at 02:55:41PM +0100, Cezary Rojewski wrote:
>> The put() operation is expected to return:
>> 1) 0 on success if no changes were made
>> 2) 1 on success if changes were made
>> 3) error code otherwise
>>
>> Currently 2) is usually ignored when writing control-operations. While
>> forcing compliance is not an option right now, make it easier for
>> developers to adhere to the expectations and notice problems by logging
>> them when CONFIG_SND_CTL_DEBUG is enabled.
> 
> Makes sense to me, although I fear it's about as likely that people
> enable this config as that they run mixer-test it's reasonable to be
> helpful.

Fair point. I admit I've noticed the problem in the existing 
sound/soc/intel/* drivers long time ago and.. forgot to address them due 
to lack of any "pings" in the logs.

alsa-utils' tools and alsa-lib do not allow to differentiate between 
success-no-changes and success-changes. Ideally userspace-level tests 
(functional tests) would be allowed to verify that. While in the long 
run updating/extending alsa-lib is probably the way to go, I believe 
having kernel-level check _now_ is not bad.

>> +	struct snd_ctl_elem_value original;
> 
>> +	ret = kctl->get(kctl, &original);
>> +	if (ret)
>> +		return ret;
> 
>> +	retcmp = memcmp(&original.value.bytes.data[0], &control->value.bytes.data[0],
>> +			sizeof(original.value.bytes.data[0]));
>> +	if (retcmp)
>> +		retcmp = 1;
> 
> original was just allocated from the stack so who knows what values it
> had originally, and the get() is only going to write the part of the
> value that has data since the normal get() path has a memset() in the
> core.  Similarly with the new value coming in from userspace there's no
> requirement for userspace to set anything that isn't part of the value
> being written to any particular value.  This means we're liable to get
> spurious mismatches.

You're right, I've based my patch on results from drivers that do init 
the space entirely. That may not be true for every driver in sound/.

Re: [PATCH] ALSA: control: Verify put() result when in debug mode

[Cezary Rojewski]

On 2026-01-30 3:29 PM, Takashi Iwai wrote:
> On Fri, 30 Jan 2026 15:12:58 +0100,
> Mark Brown wrote:
>>> +	struct snd_ctl_elem_value original;
>>
>>> +	ret = kctl->get(kctl, &original);
>>> +	if (ret)
>>> +		return ret;
>>
>>> +	retcmp = memcmp(&original.value.bytes.data[0], &control->value.bytes.data[0],
>>> +			sizeof(original.value.bytes.data[0]));
>>> +	if (retcmp)
>>> +		retcmp = 1;
>>
>> original was just allocated from the stack so who knows what values it
>> had originally, and the get() is only going to write the part of the
>> value that has data since the normal get() path has a memset() in the
>> core.  Similarly with the new value coming in from userspace there's no
>> requirement for userspace to set anything that isn't part of the value
>> being written to any particular value.  This means we're liable to get
>> spurious mismatches.
> 
> Yes, and if I understand correctly, the above memcmpy() just compare
> the single byte from original and the result?  Then it'll lead to
> false-positive outputs.
> 
> We'll need to query the control info and check the relevant values
> for each info->type and count.

Wouldn't memset(0) as a preparation-step solve the issue? That is, each 
element - instance of struct snd_ctl_elem_value - has up to 512 bytes of 
data, regardless of type. Not seeing usefulness of ->info() here. 
Perhaps I'm missing something.

> 
> Also, better to name it snd_ctl_put_verify() instead of
> snd_ctl_write_verify(); it's an equivalent with snd_ctl_put() having
> an additional verification.

Guess the idea for the name is that the operation focuses solely on 
kctl->put(), not the "write" as a whole? Sounds good, will do in v2.

Re: [PATCH] ALSA: control: Verify put() result when in debug mode

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1475 bytes --]

On Fri, Jan 30, 2026 at 04:00:10PM +0100, Cezary Rojewski wrote:
> On 2026-01-30 3:29 PM, Takashi Iwai wrote:

> > > > +	retcmp = memcmp(&original.value.bytes.data[0], &control->value.bytes.data[0],
> > > > +			sizeof(original.value.bytes.data[0]));

> > > original was just allocated from the stack so who knows what values it
> > > had originally, and the get() is only going to write the part of the
> > > value that has data since the normal get() path has a memset() in the
> > > core.  Similarly with the new value coming in from userspace there's no
> > > requirement for userspace to set anything that isn't part of the value
> > > being written to any particular value.  This means we're liable to get
> > > spurious mismatches.

> > Yes, and if I understand correctly, the above memcmpy() just compare
> > the single byte from original and the result?  Then it'll lead to
> > false-positive outputs.

> > We'll need to query the control info and check the relevant values
> > for each info->type and count.

> Wouldn't memset(0) as a preparation-step solve the issue? That is, each
> element - instance of struct snd_ctl_elem_value - has up to 512 bytes of
> data, regardless of type. Not seeing usefulness of ->info() here. Perhaps
> I'm missing something.

A memset() on the original value before reading would get it into a
known state but wouldn't solve the problem that userpace can pass any
old junk in the unused parts of the new value it passes into put().

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ALSA: control: Verify put() result when in debug mode

[Takashi Iwai]

On Fri, 30 Jan 2026 16:00:10 +0100,
Cezary Rojewski wrote:
> 
> On 2026-01-30 3:29 PM, Takashi Iwai wrote:
> > On Fri, 30 Jan 2026 15:12:58 +0100,
> > Mark Brown wrote:
> >>> +	struct snd_ctl_elem_value original;
> >> 
> >>> +	ret = kctl->get(kctl, &original);
> >>> +	if (ret)
> >>> +		return ret;
> >> 
> >>> +	retcmp = memcmp(&original.value.bytes.data[0], &control->value.bytes.data[0],
> >>> +			sizeof(original.value.bytes.data[0]));
> >>> +	if (retcmp)
> >>> +		retcmp = 1;
> >> 
> >> original was just allocated from the stack so who knows what values it
> >> had originally, and the get() is only going to write the part of the
> >> value that has data since the normal get() path has a memset() in the
> >> core.  Similarly with the new value coming in from userspace there's no
> >> requirement for userspace to set anything that isn't part of the value
> >> being written to any particular value.  This means we're liable to get
> >> spurious mismatches.
> > 
> > Yes, and if I understand correctly, the above memcmpy() just compare
> > the single byte from original and the result?  Then it'll lead to
> > false-positive outputs.
> > 
> > We'll need to query the control info and check the relevant values
> > for each info->type and count.
> 
> Wouldn't memset(0) as a preparation-step solve the issue? That is,
> each element - instance of struct snd_ctl_elem_value - has up to 512
> bytes of data, regardless of type. Not seeing usefulness of ->info()
> here. Perhaps I'm missing something.

Well, you're calling memcmp with sizeof(original.value.bytes.data[0]),
and this size is just one byte, no?

And, if you need to compare the full data, the remaining space has to
be initialized, yes.  For the variable original, you can do memset(),
but for control->value, there is no guarantee of initialization of
remaining space because it's a copy from user-space.


Takashi

Re: [PATCH] ALSA: control: Verify put() result when in debug mode

[Kuninori Morimoto]

Hi Cezary

> The put() operation is expected to return:
> 1) 0 on success if no changes were made
> 2) 1 on success if changes were made
> 3) error code otherwise
> 
> Currently 2) is usually ignored when writing control-operations. While
> forcing compliance is not an option right now, make it easier for
> developers to adhere to the expectations and notice problems by logging
> them when CONFIG_SND_CTL_DEBUG is enabled.
> 
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
(snip)
> +__maybe_unused
> +static int snd_ctl_write_verify(struct snd_kcontrol *kctl, struct snd_ctl_elem_value *control)
> +{
> +	struct snd_ctl_elem_value original;
> +	int ret, retcmp;
> +
> +	ret = kctl->get(kctl, &original);
> +	if (ret)
> +		return ret;
> +
> +	ret = kctl->put(kctl, control);
> +	if (ret < 0)
> +		return ret;
> +
> +	retcmp = memcmp(&original.value.bytes.data[0], &control->value.bytes.data[0],
> +			sizeof(original.value.bytes.data[0]));
> +	if (retcmp)
> +		retcmp = 1;
> +
> +	if (retcmp == ret)
> +		pr_info("kctl->put() returned the expected value of '%d'\n", ret);
> +	else
> +		pr_warn("expected kctl->put() to return '%d' but got '%d'\n", ret, retcmp);
> +	return ret;
> +}

I think this is easy to read ?

	static int snd_kctl_put(...)
	{
		if (IS_ENABLED(CONFIG_SND_CTL_DEBUG)) {
			/* Code with verification */
		} else {
			/* Code without verification */
		}
		return ret;
	}

	...

	if (!result)
-		result = kctl->put(...);
+		result = snd_kctl_put(...);


Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH] ALSA: control: Verify put() result when in debug mode

[Cezary Rojewski]

On 2026-02-02 1:20 AM, Kuninori Morimoto wrote:

>> +static int snd_ctl_write_verify(struct snd_kcontrol *kctl, struct snd_ctl_elem_value *control)
>> +{
>> +	struct snd_ctl_elem_value original;
>> +	int ret, retcmp;
>> +
>> +	ret = kctl->get(kctl, &original);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = kctl->put(kctl, control);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	retcmp = memcmp(&original.value.bytes.data[0], &control->value.bytes.data[0],
>> +			sizeof(original.value.bytes.data[0]));
>> +	if (retcmp)
>> +		retcmp = 1;
>> +
>> +	if (retcmp == ret)
>> +		pr_info("kctl->put() returned the expected value of '%d'\n", ret);
>> +	else
>> +		pr_warn("expected kctl->put() to return '%d' but got '%d'\n", ret, retcmp);
>> +	return ret;
>> +}
> 
> I think this is easy to read ?
> 
> 	static int snd_kctl_put(...)
> 	{
> 		if (IS_ENABLED(CONFIG_SND_CTL_DEBUG)) {
> 			/* Code with verification */
> 		} else {
> 			/* Code without verification */
> 		}
> 		return ret;
> 	}
> 
> 	...
> 
> 	if (!result)
> -		result = kctl->put(...);
> +		result = snd_kctl_put(...);

Hello,

I see the idea - reduce the number of if-statements in the actual code. 
What I decided to do in v2 is the above with a little twist - enlist 
macros. Not a fun for declaring yet another function with a single 
if-statement. Also, opted for "snd_ctl_put_xxx" instead of 
"snd_kctl_put_xxx" to match naming pattern most widely used in 
sound/core/control.c file.

Kind regards,
Czarek

Re: [PATCH] ALSA: control: Verify put() result when in debug mode

[Cezary Rojewski]

On 2026-01-30 4:14 PM, Takashi Iwai wrote:
> On Fri, 30 Jan 2026 16:00:10 +0100,
> Cezary Rojewski wrote:
>>
>> On 2026-01-30 3:29 PM, Takashi Iwai wrote:
>>> On Fri, 30 Jan 2026 15:12:58 +0100,
>>> Mark Brown wrote:
>>>>> +	struct snd_ctl_elem_value original;
>>>>
>>>>> +	ret = kctl->get(kctl, &original);
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>
>>>>> +	retcmp = memcmp(&original.value.bytes.data[0], &control->value.bytes.data[0],
>>>>> +			sizeof(original.value.bytes.data[0]));
>>>>> +	if (retcmp)
>>>>> +		retcmp = 1;
>>>>
>>>> original was just allocated from the stack so who knows what values it
>>>> had originally, and the get() is only going to write the part of the
>>>> value that has data since the normal get() path has a memset() in the
>>>> core.  Similarly with the new value coming in from userspace there's no
>>>> requirement for userspace to set anything that isn't part of the value
>>>> being written to any particular value.  This means we're liable to get
>>>> spurious mismatches.
>>>
>>> Yes, and if I understand correctly, the above memcmpy() just compare
>>> the single byte from original and the result?  Then it'll lead to
>>> false-positive outputs.
>>>
>>> We'll need to query the control info and check the relevant values
>>> for each info->type and count.
>>
>> Wouldn't memset(0) as a preparation-step solve the issue? That is,
>> each element - instance of struct snd_ctl_elem_value - has up to 512
>> bytes of data, regardless of type. Not seeing usefulness of ->info()
>> here. Perhaps I'm missing something.
> 
> Well, you're calling memcmp with sizeof(original.value.bytes.data[0]),
> and this size is just one byte, no?
> 
> And, if you need to compare the full data, the remaining space has to
> be initialized, yes.  For the variable original, you can do memset(),
> but for control->value, there is no guarantee of initialization of
> remaining space because it's a copy from user-space.

I admit, I'm a bit embarrassed by the 
"sizeof(original.value.bytes.data[0])" mistake of mine, fixed in v2.

The ->info() operation brings questions to my mind but I believe in v2 
I've utilized it as suggested: use info->type and info->count to 
sanitize the 'new' value. If I'm wrong about that, let me know in the v2 
review.

Kind regards,
Czarek

Re: [PATCH] ALSA: control: Verify put() result when in debug mode

[Cezary Rojewski]

On 2026-01-30 4:13 PM, Mark Brown wrote:
> On Fri, Jan 30, 2026 at 04:00:10PM +0100, Cezary Rojewski wrote:

>> Wouldn't memset(0) as a preparation-step solve the issue? That is, each
>> element - instance of struct snd_ctl_elem_value - has up to 512 bytes of
>> data, regardless of type. Not seeing usefulness of ->info() here. Perhaps
>> I'm missing something.
> 
> A memset() on the original value before reading would get it into a
> known state but wouldn't solve the problem that userpace can pass any
> old junk in the unused parts of the new value it passes into put().

Thank you for pointing this out. Indeed, "proper userspace" is not 
always the case, which my v1 clearly forgot about.

I did few tests with ->info, namely info->type and info->count. Reused 
the existing fill_remaining_elem_value() to be exact, with '0' as a 
'pattern'. Hopefully that method is enough to deem whatever is provided 
to ->put() worthy for the verification.

Kind regards,
Czarek