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

[sashiko-bot@kernel.org]

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