[PATCH] ALSA: core: Fix signedness issue in info read callback

[PATCH] ALSA: core: Fix signedness issue in info read callback

[Sreeraj S Kurup]

The read callback returns ssize_t, which allows it to return
either the number of bytes read or a negative error code.

However, snd_info_entry_read() stores the callback result in a
size_t variable. This converts negative error codes into large
unsigned values.

Use a separate ssize_t variable to store the callback result.
This preserves negative error codes and ensures that the file
position is updated only when the callback returns a positive
number of bytes.

This prevents callback errors from being incorrectly reported
as successful reads.

Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
---
 sound/core/info.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/sound/core/info.c b/sound/core/info.c
index c4d57d0809ad..35aaa8bd6d56 100644
--- a/sound/core/info.c
+++ b/sound/core/info.c
@@ -144,6 +144,7 @@ static ssize_t snd_info_entry_read(struct file *file, char __user *buffer,
 	struct snd_info_private_data *data = file->private_data;
 	struct snd_info_entry *entry = data->entry;
 	size_t size;
+	ssize_t ret;
 	loff_t pos;
 
 	pos = *offset;
@@ -153,11 +154,11 @@ static ssize_t snd_info_entry_read(struct file *file, char __user *buffer,
 		return 0;
 	size = entry->size - pos;
 	size = min(count, size);
-	size = entry->c.ops->read(entry, data->file_private_data,
+	ret = entry->c.ops->read(entry, data->file_private_data,
 				  file, buffer, size, pos);
-	if ((ssize_t) size > 0)
-		*offset = pos + size;
-	return size;
+	if (ret > 0)
+		*offset = pos + ret;
+	return ret;
 }
 
 static ssize_t snd_info_entry_write(struct file *file, const char __user *buffer,
-- 
2.55.0

Re: [PATCH] ALSA: core: Fix signedness issue in info read callback

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] ALSA: core: commit message falsely claims runtime bug in
  snd_info_entry_read()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Medium] ALSA: info: unguarded zero-length read in
  snd_info_entry_read() [1]
--

commit b4351fe9c4662bfa6e141f7e6b6bfcf0446f56c1
Author: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

> ALSA: core: Fix signedness issue in info read callback
>
> The read callback returns ssize_t, which allows it to return
> either the number of bytes read or a negative error code.
>
> However, snd_info_entry_read() stores the callback result in a
> size_t variable. This converts negative error codes into large
> unsigned values.
>
> Use a separate ssize_t variable to store the callback result.
> This preserves negative error codes and ensures that the file
> position is updated only when the callback returns a positive
> number of bytes.
>
> This prevents callback errors from being incorrectly reported
> as successful reads.
>
> Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

[Severity: Low]
Is the commit message accurate about the runtime effect of the previous
code?

Before this patch, snd_info_entry_read() checked if ((ssize_t)size > 0)
before advancing *offset, and its ssize_t return type converted size back
to a negative value on return.  Storing the return value in ret is a clean
refactoring that drops the cast, rather than a fix for misreported errors
or bogus offset updates.

> diff --git a/sound/core/info.c b/sound/core/info.c
> index 2c2a80a7d0d2b..c16643c17ca18 100644
> --- a/sound/core/info.c
> +++ b/sound/core/info.c
> @@ -144,6 +144,7 @@ static ssize_t snd_info_entry_read(struct file *file, char __user *buffer,
>  	struct snd_info_private_data *data = file->private_data;
>  	struct snd_info_entry *entry = data->entry;
>  	size_t size;
> +	ssize_t ret;
>  	loff_t pos;
>  
>  	pos = *offset;
> @@ -153,11 +154,11 @@ static ssize_t snd_info_entry_read(struct file *file, char __user *buffer,
>  		return 0;
>  	size = entry->size - pos;
>  	size = min(count, size);
> -	size = entry->c.ops->read(entry, data->file_private_data,
> +	ret = entry->c.ops->read(entry, data->file_private_data,
>  				  file, buffer, size, pos);

[Severity: Medium]
This problem wasn't introduced by this patch, but can snd_info_entry_read()
invoke entry->c.ops->read() with size == 0 [1]?

Commit d97e1b78239c ("ALSA: info - Check file position validity in common
layer") removed the size > 0 checks from driver proc callbacks, but only
guarded the callback with count > 0 in snd_info_entry_write().  When a
caller issues a zero-length read at pos < entry->size, size is clamped to 0
and passed to entry->c.ops->read().

In callbacks such as snd_opl4_mem_proc_read(), passing count == 0 to
vmalloc() triggers WARN_ON_ONCE(!size) in __vmalloc_node_range_noprof() and
fails the read with -ENOMEM instead of returning 0:

sound/drivers/opl4/opl4_proc.c:snd_opl4_mem_proc_read() {
	...
	buf = vmalloc(count);
	if (!buf)
		return -ENOMEM;
	...
}

[1] https://sashiko.dev/#/bug/linux-23f2fe78-69db-4727-af8a-a7d3df991e93

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109080850.2726-1-sreekuttan2156239@gmail.com?part=1

[PATCH v2] ALSA: core: Fix info read handling

[Sreeraj S Kurup]

snd_info_entry_read() may invoke driver read callbacks with a
zero-length read when the requested count is zero.

Some callbacks perform zero-sized allocations, such as
vmalloc(0), which can trigger warnings and cause the read to
fail with -ENOMEM.

Return immediately when count is zero to avoid invoking driver
callbacks unnecessarily.

Also, store the callback result in a separate ssize_t variable
instead of reusing the size_t variable. This makes the signedness
explicit and avoids the unnecessary cast when checking whether
to update the file position.

Update the file position only when the callback returns a
positive number of bytes.

Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
---
v2:
 - Handle zero-length reads in snd_info_entry_read() by returning
   early without invoking the driver callback.
 - Clarify the commit message to describe the ssize_t change as a
   signedness cleanup rather than claiming it fixes incorrectly
   reported callback errors.

 sound/core/info.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/sound/core/info.c b/sound/core/info.c
index c4d57d0809ad..42312136f76c 100644
--- a/sound/core/info.c
+++ b/sound/core/info.c
@@ -144,6 +144,7 @@ static ssize_t snd_info_entry_read(struct file *file, char __user *buffer,
 	struct snd_info_private_data *data = file->private_data;
 	struct snd_info_entry *entry = data->entry;
 	size_t size;
+	ssize_t ret;
 	loff_t pos;
 
 	pos = *offset;
@@ -151,13 +152,15 @@ static ssize_t snd_info_entry_read(struct file *file, char __user *buffer,
 		return -EIO;
 	if (pos >= entry->size)
 		return 0;
+	if (!count)
+		return 0;
 	size = entry->size - pos;
 	size = min(count, size);
-	size = entry->c.ops->read(entry, data->file_private_data,
+	ret = entry->c.ops->read(entry, data->file_private_data,
 				  file, buffer, size, pos);
-	if ((ssize_t) size > 0)
-		*offset = pos + size;
-	return size;
+	if (ret > 0)
+		*offset = pos + ret;
+	return ret;
 }
 
 static ssize_t snd_info_entry_write(struct file *file, const char __user *buffer,
-- 
2.55.0

Re: [PATCH v2] ALSA: core: Fix info read handling

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] ALSA: core: missing Fixes trailer for commit d97e1b78239c
--

commit a395693d2a5039fea3a9bd10c727bfff30a92e11
Author: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

> ALSA: core: Fix info read handling
>
> snd_info_entry_read() may invoke driver read callbacks with a
> zero-length read when the requested count is zero.
>
> Some callbacks perform zero-sized allocations, such as
> vmalloc(0), which can trigger warnings and cause the read to
> fail with -ENOMEM.
>
> Return immediately when count is zero to avoid invoking driver
> callbacks unnecessarily.
>
> Also, store the callback result in a separate ssize_t variable
> instead of reusing the size_t variable. This makes the signedness
> explicit and avoids the unnecessary cast when checking whether
> to update the file position.
>
> Update the file position only when the callback returns a
> positive number of bytes.
>
> Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

[Severity: Low]
Driver read callbacks previously checked size > 0 themselves until those
checks were centralized in snd_info_entry_read() and snd_info_entry_write(),
where only snd_info_entry_write() guarded against count == 0.  Should this
include:

  Fixes: d97e1b78239c ("ALSA: info - Check file position validity in common layer")

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109085922.3104-1-sreekuttan2156239@gmail.com?part=1

[PATCH v3] ALSA: core: Fix info read handling

[Sreeraj S Kurup]

snd_info_entry_read() may invoke driver read callbacks with a
zero-length read when the requested count is zero.

Some callbacks perform zero-sized allocations, such as
vmalloc(0), which can trigger warnings and cause the read to
fail with -ENOMEM.

Return immediately when count is zero to avoid invoking driver
callbacks unnecessarily.

Also, store the callback result in a separate ssize_t variable
instead of reusing the size_t variable. This makes the signedness
explicit and avoids the unnecessary cast when checking whether
to update the file position.

Update the file position only when the callback returns a
positive number of bytes.

Fixes: d97e1b78239c ("ALSA: info - Check file position validity in common layer")
Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
---
v3:
 - Add Fixes trailer for the commit that introduced the zero-length
   read issue.
v2:
 - Handle zero-length reads in snd_info_entry_read() by returning
   early without invoking the driver callback.
 - Clarify the commit message to describe the ssize_t change as a
   signedness cleanup rather than claiming it fixes incorrectly
   reported callback errors.

 sound/core/info.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/sound/core/info.c b/sound/core/info.c
index c4d57d0809ad..42312136f76c 100644
--- a/sound/core/info.c
+++ b/sound/core/info.c
@@ -144,6 +144,7 @@ static ssize_t snd_info_entry_read(struct file *file, char __user *buffer,
 	struct snd_info_private_data *data = file->private_data;
 	struct snd_info_entry *entry = data->entry;
 	size_t size;
+	ssize_t ret;
 	loff_t pos;
 
 	pos = *offset;
@@ -151,13 +152,15 @@ static ssize_t snd_info_entry_read(struct file *file, char __user *buffer,
 		return -EIO;
 	if (pos >= entry->size)
 		return 0;
+	if (!count)
+		return 0;
 	size = entry->size - pos;
 	size = min(count, size);
-	size = entry->c.ops->read(entry, data->file_private_data,
+	ret = entry->c.ops->read(entry, data->file_private_data,
 				  file, buffer, size, pos);
-	if ((ssize_t) size > 0)
-		*offset = pos + size;
-	return size;
+	if (ret > 0)
+		*offset = pos + ret;
+	return ret;
 }
 
 static ssize_t snd_info_entry_write(struct file *file, const char __user *buffer,
-- 
2.55.0