ASoC: Intel: avs: Fix potential buffer overflow by snprintf()

[PATCH v2] ASoC: Intel: avs: Fix potential buffer overflow by snprintf()

Posted by hariconscious@gmail.com 2 months, 3 weeks ago

From: HariKrishna Sagala <hariconscious@gmail.com>

snprintf() returns the would-be-filled size when the string overflows
the given buffer size, hence using this value may result in a buffer
overflow (although it's unrealistic).

This patch replaces it with a safer version, scnprintf() for papering
over such a potential issue.
Link: https://github.com/KSPP/linux/issues/105
'Fixes: 5a565ba23abe ("ASoC: Intel: avs: Probing and firmware tracing
over debugfs")'

Signed-off-by: HariKrishna Sagala <hariconscious@gmail.com>
---
Thank you for the feedback and the suggestions.
Corrected the indentation & commit message.
V1:
https://lore.kernel.org/all/20251112120235.54328-2-hariconscious@gmail.com/
 sound/soc/intel/avs/debugfs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sound/soc/intel/avs/debugfs.c b/sound/soc/intel/avs/debugfs.c
index 3534de46f9e4..cdb82392b9ee 100644
--- a/sound/soc/intel/avs/debugfs.c
+++ b/sound/soc/intel/avs/debugfs.c
@@ -119,9 +119,9 @@ static ssize_t probe_points_read(struct file *file, char __user *to, size_t coun
 	}
 
 	for (i = 0; i < num_desc; i++) {
-		ret = snprintf(buf + len, PAGE_SIZE - len,
-			       "Id: %#010x  Purpose: %d  Node id: %#x\n",
-			       desc[i].id.value, desc[i].purpose, desc[i].node_id.val);
+		ret = scnprintf(buf + len, PAGE_SIZE - len,
+					"Id: %#010x  Purpose: %d  Node id: %#x\n",
+					desc[i].id.value, desc[i].purpose, desc[i].node_id.val);
 		if (ret < 0)
 			goto free_desc;
 		len += ret;

base-commit: 24172e0d79900908cf5ebf366600616d29c9b417
-- 
2.43.0

Re: [PATCH v2] ASoC: Intel: avs: Fix potential buffer overflow by snprintf()

Posted by Greg KH 2 months, 3 weeks ago

On Wed, Nov 12, 2025 at 11:48:51PM +0530, hariconscious@gmail.com wrote:
> From: HariKrishna Sagala <hariconscious@gmail.com>
> 
> snprintf() returns the would-be-filled size when the string overflows
> the given buffer size, hence using this value may result in a buffer
> overflow (although it's unrealistic).

unrealistic == impossible

So why make this change at all?

> This patch replaces it with a safer version, scnprintf() for papering
> over such a potential issue.

Don't "paper over", actually fix real things.


> Link: https://github.com/KSPP/linux/issues/105
> 'Fixes: 5a565ba23abe ("ASoC: Intel: avs: Probing and firmware tracing
> over debugfs")'

No, this is not a "fix".

Also please do not wrap lines of fixes tags.

thanks,

greg k-h

Re: [PATCH v2] ASoC: Intel: avs: Fix potential buffer overflow by snprintf()

Posted by Cezary Rojewski 2 months, 3 weeks ago

On 2025-11-12 8:20 PM, Greg KH wrote:
> On Wed, Nov 12, 2025 at 11:48:51PM +0530, hariconscious@gmail.com wrote:
>> From: HariKrishna Sagala <hariconscious@gmail.com>
>>
>> snprintf() returns the would-be-filled size when the string overflows
>> the given buffer size, hence using this value may result in a buffer
>> overflow (although it's unrealistic).
> 
> unrealistic == impossible
> 
> So why make this change at all?

The problem will never occur in production-scenario given the AudioDSP 
firmware limitation - max ~10 probe-point entries so, the built string 
will be far away from 4K_SZ bytes.

If the verdict is: ignore the recommendation as the problem is 
unrealistic, I'm OK with that. Typically though I'd prefer to stick to 
the recommendations.

> 
>> This patch replaces it with a safer version, scnprintf() for papering
>> over such a potential issue.
> 
> Don't "paper over", actually fix real things.
> 
> 
>> Link: https://github.com/KSPP/linux/issues/105
>> 'Fixes: 5a565ba23abe ("ASoC: Intel: avs: Probing and firmware tracing
>> over debugfs")'
> 
> No, this is not a "fix".

The patch isn't worded well, that's clear.
While the patch is an outcome of static-analysis, isn't it good to have 
'Fixes:' to point out the offending commit regardless?

Re: [PATCH v2] ASoC: Intel: avs: Fix potential buffer overflow by snprintf()

Posted by Greg KH 2 months, 3 weeks ago

On Thu, Nov 13, 2025 at 09:46:12AM +0100, Cezary Rojewski wrote:
> On 2025-11-12 8:20 PM, Greg KH wrote:
> > On Wed, Nov 12, 2025 at 11:48:51PM +0530, hariconscious@gmail.com wrote:
> > > From: HariKrishna Sagala <hariconscious@gmail.com>
> > > 
> > > snprintf() returns the would-be-filled size when the string overflows
> > > the given buffer size, hence using this value may result in a buffer
> > > overflow (although it's unrealistic).
> > 
> > unrealistic == impossible
> > 
> > So why make this change at all?
> 
> The problem will never occur in production-scenario given the AudioDSP
> firmware limitation - max ~10 probe-point entries so, the built string will
> be far away from 4K_SZ bytes.
> 
> If the verdict is: ignore the recommendation as the problem is unrealistic,
> I'm OK with that. Typically though I'd prefer to stick to the
> recommendations.

That's fine, but don't claim that it fixes a buffer overflow when that
is NOT what this is doing at all.

> > > This patch replaces it with a safer version, scnprintf() for papering
> > > over such a potential issue.
> > 
> > Don't "paper over", actually fix real things.
> > 
> > 
> > > Link: https://github.com/KSPP/linux/issues/105
> > > 'Fixes: 5a565ba23abe ("ASoC: Intel: avs: Probing and firmware tracing
> > > over debugfs")'
> > 
> > No, this is not a "fix".
> 
> The patch isn't worded well, that's clear.
> While the patch is an outcome of static-analysis, isn't it good to have
> 'Fixes:' to point out the offending commit regardless?

No, it is not "fixing" anything.  Please don't claim that it does.  It
is "just" a code transformation to get rid of an api that some people do
not like.

thanks,

greg k-h

Re: [PATCH v2] ASoC: Intel: avs: Fix potential buffer overflow by snprintf()

Posted by Mark Brown 2 months, 3 weeks ago

On Wed, Nov 12, 2025 at 02:20:19PM -0500, Greg KH wrote:

> Also please do not wrap lines of fixes tags.

Someone probably ought to teach checkpatch about that one, it moans
about long lines without checking for Fixes: IIRC.

Re: [PATCH v2] ASoC: Intel: avs: Fix potential buffer overflow by snprintf()

Posted by Sakari Ailus 2 months, 3 weeks ago

On Wed, Nov 12, 2025 at 07:33:51PM +0000, Mark Brown wrote:
> On Wed, Nov 12, 2025 at 02:20:19PM -0500, Greg KH wrote:
> 
> > Also please do not wrap lines of fixes tags.
> 
> Someone probably ought to teach checkpatch about that one, it moans
> about long lines without checking for Fixes: IIRC.

I can recall this issue, too... I checked how to reproduce and fix this but
it seems it's already done: I couldn't reproduce it. I'm getting this for
breaking a Fixes: line:

WARNING: Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("<title line>")' - ie: 'Fixes: ...

It basically now checks the subject matches with the quoted string.

So all is well!

-- 
Sakari Ailus