Re: [PATCH] ALSA: hdsp: Fix divide-by-zero in DDS offset control

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ALSA: hdsp: incomplete offset validation in hdsp_set_dds_offset()
--

commit a91b0358087c1f7f09a07e8d353cc380f3a7e223
Author: Bob Song <songxiebing@kylinos.cn>

ALSA: hdsp: Fix divide-by-zero in DDS offset control

This commit updates hdsp_set_dds_offset() to reject non-positive target
sample rates with -EINVAL before calling hdsp_set_dds_value() to prevent a
divide-by-zero in div_u64().

> diff --git a/sound/pci/rme9652/hdsp.c b/sound/pci/rme9652/hdsp.c
> index 7dce548b9f363..694816f966d29 100644
> --- a/sound/pci/rme9652/hdsp.c
> +++ b/sound/pci/rme9652/hdsp.c
> @@ -2802,6 +2802,9 @@ static int hdsp_dds_offset(struct hdsp *hdsp)
>  static int hdsp_set_dds_offset(struct hdsp *hdsp, int offset_hz)
>  {
>  	int rate = hdsp->system_sample_rate + offset_hz;
> +
> +	if (rate <= 0)
> +		return -EINVAL;

[Severity: Medium]
Is checking rate <= 0 sufficient to validate offset_hz here?

When snd_hdsp_put_dds_offset() calls hdsp_set_dds_offset(), offset_hz comes
from ucontrol->value.integer.value[0] without enforcing the [-5000, 5000]
range declared in snd_hdsp_info_dds_offset():

sound/pci/rme9652/hdsp.c:snd_hdsp_info_dds_offset() {
	...
	uinfo->value.integer.min = -5000;
	uinfo->value.integer.max = 5000;
	return 0;
}

First, adding an unvalidated positive offset_hz to hdsp->system_sample_rate
can overflow a signed 32-bit int.

Second, a positive rate in the range 1..24414 (for example, passing
offset_hz = 1 - hdsp->system_sample_rate) still passes the rate <= 0 check
and reaches hdsp_set_dds_value():

sound/pci/rme9652/hdsp.c:hdsp_set_dds_value() {
	...
	n = DDS_NUMERATOR;
	n = div_u64(n, rate);
	/* n should be less than 2^32 for being written to FREQ register */
	snd_BUG_ON(n >> 32);
	/* HDSP_freqReg and HDSP_resetPointer are the same, so keep the DDS
	   value to write it after a reset */
	hdsp->dds_value = n;
	hdsp_write(hdsp, HDSP_freqReg, hdsp->dds_value);
}

Because DDS_NUMERATOR is 104857600000000ULL, any rate in 1..24414 produces
n >= 2^32, which triggers snd_BUG_ON(n >> 32) and truncates n when writing
hdsp->dds_value and HDSP_freqReg. Offsets outside [-5000, 5000] can also
cross the 56000 or 112000 thresholds in hdsp_set_dds_value(), whereas
hdsp_dds_offset() chooses the 2x or 4x multiplier based on
hdsp->system_sample_rate.

In addition, when hdsp_set_dds_offset() returns -EINVAL, its caller
snd_hdsp_put_dds_offset() drops the error code:

sound/pci/rme9652/hdsp.c:snd_hdsp_put_dds_offset() {
	...
	if (val != hdsp_dds_offset(hdsp))
		change = (hdsp_set_dds_offset(hdsp, val) == 0) ? 1 : 0;
	...
	return change;
}

Should hdsp_set_dds_offset() validate that offset_hz is within
[-5000, 5000] before computing rate, and should snd_hdsp_put_dds_offset()
propagate negative error codes from hdsp_set_dds_offset()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104083045.1939479-1-songxiebing@kylinos.cn?part=1