sound/soc/sof/compress.c | 3 +++ 1 file changed, 3 insertions(+)
When reporting the pointer for a compressed stream we report the current
I/O frame position by dividing the position by the number of channels
multiplied by the number of container bytes. These values default to 0 and
are only configured as part of setting the stream parameters so this allows
a divide by zero to be configured. Validate that they are non zero,
returning an error if not
Fixes: c1a731c71359 ("ASoC: SOF: compress: Add support for computing timestamps")
Signed-off-by: Mark Brown <broonie@kernel.org>
Cc: stable@vger.kernel.org
---
sound/soc/sof/compress.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/sound/soc/sof/compress.c b/sound/soc/sof/compress.c
index 96570121aae0..90f056eae1c3 100644
--- a/sound/soc/sof/compress.c
+++ b/sound/soc/sof/compress.c
@@ -379,6 +379,9 @@ static int sof_compr_pointer(struct snd_soc_component *component,
if (!spcm)
return -EINVAL;
+ if (!sstream->channels || !sstream->sample_container_bytes)
+ return -EBUSY;
+
tstamp->sampling_rate = sstream->sampling_rate;
tstamp->copied_total = sstream->copied_total;
tstamp->pcm_io_frames = div_u64(spcm->stream[cstream->direction].posn.dai_posn,
---
base-commit: c369299895a591d96745d6492d4888259b004a9e
change-id: 20260326-asoc-compress-tstamp-params-296f38f15217
Best regards,
--
Mark Brown <broonie@kernel.org>
On 26/03/2026 16:52, Mark Brown wrote:
> When reporting the pointer for a compressed stream we report the current
> I/O frame position by dividing the position by the number of channels
> multiplied by the number of container bytes. These values default to 0 and
> are only configured as part of setting the stream parameters so this allows
> a divide by zero to be configured. Validate that they are non zero,
> returning an error if not
>
> Fixes: c1a731c71359 ("ASoC: SOF: compress: Add support for computing timestamps")
> Signed-off-by: Mark Brown <broonie@kernel.org>
> Cc: stable@vger.kernel.org
> ---
> sound/soc/sof/compress.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/sound/soc/sof/compress.c b/sound/soc/sof/compress.c
> index 96570121aae0..90f056eae1c3 100644
> --- a/sound/soc/sof/compress.c
> +++ b/sound/soc/sof/compress.c
> @@ -379,6 +379,9 @@ static int sof_compr_pointer(struct snd_soc_component *component,
> if (!spcm)
> return -EINVAL;
>
> + if (!sstream->channels || !sstream->sample_container_bytes)
> + return -EBUSY;
> +
Is this a theoretical fix?
I don't think this can happen in real world as set_params would need to
fail and if that failed then applications would not ask for a pointer as
the compress stream cannot be even started.
> tstamp->sampling_rate = sstream->sampling_rate;
> tstamp->copied_total = sstream->copied_total;
> tstamp->pcm_io_frames = div_u64(spcm->stream[cstream->direction].posn.dai_posn,
>
> ---
> base-commit: c369299895a591d96745d6492d4888259b004a9e
> change-id: 20260326-asoc-compress-tstamp-params-296f38f15217
>
> Best regards,
> --
> Mark Brown <broonie@kernel.org>
>
>
--
Péter
On Fri, Mar 27, 2026 at 11:49:41AM +0200, Péter Ujfalusi wrote: > On 26/03/2026 16:52, Mark Brown wrote: > > + if (!sstream->channels || !sstream->sample_container_bytes) > > + return -EBUSY; > > + > Is this a theoretical fix? > I don't think this can happen in real world as set_params would need to > fail and if that failed then applications would not ask for a pointer as > the compress stream cannot be even started. Yes, it's not something that would happen in the real world with a non buggy (or hostile) userspace. Still, we shouldn't leave this stuff open.
On 27/03/2026 18:52, Mark Brown wrote: > On Fri, Mar 27, 2026 at 11:49:41AM +0200, Péter Ujfalusi wrote: >> On 26/03/2026 16:52, Mark Brown wrote: > >>> + if (!sstream->channels || !sstream->sample_container_bytes) >>> + return -EBUSY; >>> + > >> Is this a theoretical fix? >> I don't think this can happen in real world as set_params would need to >> fail and if that failed then applications would not ask for a pointer as >> the compress stream cannot be even started. > > Yes, it's not something that would happen in the real world with a non > buggy (or hostile) userspace. Still, we shouldn't leave this stuff > open. Yes, hostile user space is a valid concern, in theory it can ask for TSTAMP or AVAIL before it would be meaningful (a configuration is set - buffer config is known). For avail the state sanity check is in wrong place in snd_compr_ioctl_avail(), it should be before calling snd_compr_calc_avail(). tstamp does not even have a sanity validity check in sound/cor/comrpess_offload.c, which it should as well - snd_compr_tstamp() Should this be fixed in core level to avoid repeating the same check in every driver? -- Péter
On Mon, Mar 30, 2026 at 10:01:59AM +0300, Péter Ujfalusi wrote: > Should this be fixed in core level to avoid repeating the same check in > every driver? I did wonder about that but wasn't sure if there might be some viable use case, especially for things proxying through to a DSP or something. We don't generally guard calls based on the state the stream is in, and not every implementation is going to try to do the division.
On 30/03/2026 14:05, Mark Brown wrote: > On Mon, Mar 30, 2026 at 10:01:59AM +0300, Péter Ujfalusi wrote: > >> Should this be fixed in core level to avoid repeating the same check in >> every driver? > > I did wonder about that but wasn't sure if there might be some viable > use case, especially for things proxying through to a DSP or something. I see, but proxying or not, if there were no configuration set (the state is OPEN) then asking for avail or tstamp do not have any validity. > We don't generally guard calls based on the state the stream is in, compress does this quite much, just avail and tstamp is exempt for some reason. > and not every implementation is going to try to do the division. yeah, the SOF IPC4 compress which is under review for example is not doing this. -- Péter
On Mon, Mar 30, 2026 at 02:50:15PM +0300, Péter Ujfalusi wrote: > On 30/03/2026 14:05, Mark Brown wrote: > > We don't generally guard calls based on the state the stream is in, > compress does this quite much, just avail and tstamp is exempt for some > reason. Actually already we have a guard preventing userspace from doing an avail() when we're unconfigured but we do it after we've called down into the driver which is less than ideal. I think that's because we also check for XRUN and the availability check might cause us to notice that we're in a bad state for that.
On 30/03/2026 23:11, Mark Brown wrote: > On Mon, Mar 30, 2026 at 02:50:15PM +0300, Péter Ujfalusi wrote: >> On 30/03/2026 14:05, Mark Brown wrote: > >>> We don't generally guard calls based on the state the stream is in, > >> compress does this quite much, just avail and tstamp is exempt for some >> reason. > > Actually already we have a guard preventing userspace from doing an > avail() when we're unconfigured but we do it after we've called down > into the driver which is less than ideal. I think that's because we > also check for XRUN and the availability check might cause us to notice > that we're in a bad state for that. I don't see how the avail path checks for XRUN and if the drivers supposed to do that, I'm not even sure if XRUN is possible with compress.. I did noted that the avail have the state check reversed, making it ineffective. The other point is that any return code from the driver's pointer callback is ignored by the core, the return value of stream->ops->pointer() is not even captured, it could be void. Looks like a design choice, but I cannot say. fwiw, the same check should be added to sound/soc/qcom/qdsp6/q6apm-dai.c as it does div with prtd->pcm_size (q6apm_dai_compr_pointer), which is only initialized in set_params. -- Péter
On Tue, Mar 31, 2026 at 08:12:25AM +0300, Péter Ujfalusi wrote: > On 30/03/2026 23:11, Mark Brown wrote: > > Actually already we have a guard preventing userspace from doing an > > avail() when we're unconfigured but we do it after we've called down > > into the driver which is less than ideal. I think that's because we > > also check for XRUN and the availability check might cause us to notice > > that we're in a bad state for that. > I don't see how the avail path checks for XRUN and if the drivers > supposed to do that, I'm not even sure if XRUN is possible with compress.. Yeah, I was hoping there was something in driver code there but didn't actually check. You should at least be able to get a buffer overrun with compressed streams, and even if I'd expect most things to fill silence you can undderrun which would be bad for applications like music playback. > I did noted that the avail have the state check reversed, making it > ineffective. > The other point is that any return code from the driver's pointer > callback is ignored by the core, the return value of > stream->ops->pointer() is not even captured, it could be void. > Looks like a design choice, but I cannot say. I suppose it's easier to have the error reporting in the drivers in case you want it later. > fwiw, the same check should be added to sound/soc/qcom/qdsp6/q6apm-dai.c > as it does div with prtd->pcm_size (q6apm_dai_compr_pointer), which is > only initialized in set_params. Ack, I hadn't looked at any other drivers. I was actually going to send out the core patch but even so some defence in depth would make me happier.
> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: Thursday, March 26, 2026 10:53 PM
> To: Liam Girdwood <lgirdwood@gmail.com>; Peter Ujfalusi
> <peter.ujfalusi@linux.intel.com>; Bard Liao <yung-
> chuan.liao@linux.intel.com>; Ranjani Sridharan
> <ranjani.sridharan@linux.intel.com>; Daniel Baluta <daniel.baluta@nxp.com>;
> Kai Vehmanen <kai.vehmanen@linux.intel.com>; Pierre-Louis Bossart <pierre-
> louis.bossart@linux.dev>; Jaroslav Kysela <perex@perex.cz>; Takashi Iwai
> <tiwai@suse.com>; Paul Olaru <paul.olaru@oss.nxp.com>; Laurentiu Mihalcea
> <laurentiu.mihalcea@nxp.com>
> Cc: sound-open-firmware@alsa-project.org; linux-sound@vger.kernel.org;
> linux-kernel@vger.kernel.org; Mark Brown <broonie@kernel.org>;
> stable@vger.kernel.org
> Subject: [PATCH] ASoC: SOF: Don't allow pointer operations on unconfigured
> streams
>
> When reporting the pointer for a compressed stream we report the current
> I/O frame position by dividing the position by the number of channels
> multiplied by the number of container bytes. These values default to 0 and
> are only configured as part of setting the stream parameters so this allows
> a divide by zero to be configured. Validate that they are non zero,
> returning an error if not
>
> Fixes: c1a731c71359 ("ASoC: SOF: compress: Add support for computing
> timestamps")
> Signed-off-by: Mark Brown <broonie@kernel.org>
> Cc: stable@vger.kernel.org
> ---
> sound/soc/sof/compress.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/sound/soc/sof/compress.c b/sound/soc/sof/compress.c
> index 96570121aae0..90f056eae1c3 100644
> --- a/sound/soc/sof/compress.c
> +++ b/sound/soc/sof/compress.c
> @@ -379,6 +379,9 @@ static int sof_compr_pointer(struct
> snd_soc_component *component,
> if (!spcm)
> return -EINVAL;
>
> + if (!sstream->channels || !sstream->sample_container_bytes)
> + return -EBUSY;
Sorry, but why it is BUSY in this case?
> +
> tstamp->sampling_rate = sstream->sampling_rate;
> tstamp->copied_total = sstream->copied_total;
> tstamp->pcm_io_frames = div_u64(spcm->stream[cstream-
> >direction].posn.dai_posn,
>
> ---
> base-commit: c369299895a591d96745d6492d4888259b004a9e
> change-id: 20260326-asoc-compress-tstamp-params-296f38f15217
>
> Best regards,
> --
> Mark Brown <broonie@kernel.org>
On Fri, Mar 27, 2026 at 02:09:40AM +0000, Liao, Bard wrote: > > + if (!sstream->channels || !sstream->sample_container_bytes) > > + return -EBUSY; > Sorry, but why it is BUSY in this case? -EBUSY is often "wrong state". Could also be -EINVAL, it doesn't super make a difference I think - nobody should actually be doing this.
> -----Original Message----- > From: Mark Brown <broonie@kernel.org> > Sent: Saturday, March 28, 2026 12:48 AM > To: Liao, Bard <bard.liao@intel.com> > Cc: Liam Girdwood <lgirdwood@gmail.com>; Peter Ujfalusi > <peter.ujfalusi@linux.intel.com>; Bard Liao <yung- > chuan.liao@linux.intel.com>; Ranjani Sridharan > <ranjani.sridharan@linux.intel.com>; Daniel Baluta <daniel.baluta@nxp.com>; > Kai Vehmanen <kai.vehmanen@linux.intel.com>; Pierre-Louis Bossart <pierre- > louis.bossart@linux.dev>; Jaroslav Kysela <perex@perex.cz>; Takashi Iwai > <tiwai@suse.com>; Paul Olaru <paul.olaru@oss.nxp.com>; Laurentiu Mihalcea > <laurentiu.mihalcea@nxp.com>; sound-open-firmware@alsa-project.org; > linux-sound@vger.kernel.org; linux-kernel@vger.kernel.org; > stable@vger.kernel.org > Subject: Re: [PATCH] ASoC: SOF: Don't allow pointer operations on > unconfigured streams > > On Fri, Mar 27, 2026 at 02:09:40AM +0000, Liao, Bard wrote: > > > > + if (!sstream->channels || !sstream->sample_container_bytes) > > > + return -EBUSY; > > > Sorry, but why it is BUSY in this case? > > -EBUSY is often "wrong state". Could also be -EINVAL, it doesn't super > make a difference I think - nobody should actually be doing this. Thanks Mark for the explanation, it makes sense.
© 2016 - 2026 Red Hat, Inc.