Re: [PATCH] ALSA: control: Validate element info in compat ioctl

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] ALSA: control: uncanonicalized info->id passed to
  snd_ctl_check_elem_info()
- [Low] ALSA: control: get_ctl_type() logs error and returns -EINVAL
  before access check
--

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

ALSA: control: Validate element info in compat ioctl

This commit adds a call to snd_ctl_check_elem_info() in get_ctl_type() to
validate the element type and count returned by a control's info callback
in the compat ioctl path.

> diff --git a/sound/core/control_compat.c b/sound/core/control_compat.c
> index 87f7ad5ab7391..cad00d4aa2048 100644
> --- a/sound/core/control_compat.c
> +++ b/sound/core/control_compat.c
> @@ -183,6 +183,10 @@ static int get_ctl_type(struct snd_card *card, struct snd_ctl_elem_id *id,
>  	info->id = *id;
>  	err = kctl->info(kctl, info);
>  	if (err >= 0) {
> +		err = snd_ctl_check_elem_info(card, info);
> +		if (err < 0)
> +			return err;
> +
>  		err = info->type;
>  		*countp = info->count;
>  	}

[Severity: Medium]
Can snd_ctl_check_elem_info() read past the end of info->id.name when
logging an error?

copy_ctl_value_from_user() copies id directly from userspace, and when
id->numid is non-zero, snd_ctl_find_id() resolves the control by numid
without checking id->name.  Unlike __snd_ctl_elem_info(), which rebuilds
info->id from kctl->id via snd_ctl_build_ioff() before validating,
get_ctl_type() passes the raw user-supplied info->id to
snd_ctl_check_elem_info().  If validation fails and the 44-byte
info->id.name buffer is not NUL-terminated, the dev_err() %s format reads
out of bounds into adjacent fields of struct snd_ctl_elem_info.

Rebuilding info->id with snd_ctl_build_ioff() before calling
snd_ctl_check_elem_info() would match the native path.

[Severity: Low]
Does calling snd_ctl_check_elem_info() here cause compat read and write
ioctls on large TLV bytes_ext controls to log a spurious error and return
-EINVAL instead of -EPERM?

Legitimate TLV-only controls such as ASoC bytes_ext controls can have
info->count > 512 and set SNDRV_CTL_ELEM_ACCESS_SKIP_CHECK.  In the native
path, snd_ctl_elem_read() and snd_ctl_elem_write() check vd->access first
and return -EPERM for TLV-only controls, and __snd_ctl_elem_info() skips
snd_ctl_check_elem_info() when snd_ctl_skip_validation() is true.  Because
copy_ctl_value_from_user() calls get_ctl_type() before the read/write
access check and without checking snd_ctl_skip_validation(), a 32-bit
compat ELEM_READ32 or ELEM_WRITE32 on such a control hits dev_err("invalid
count") and fails with -EINVAL.

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