Hi,
On Mon, Sep 26, 2022 at 03:55:47PM +0200, Takashi Iwai wrote:
> Hi,
>
> this is a patch set for simplifying the reference to the current PCM
> state by having the local copy in runtime instead of relying on
> runtime->status indirection. This also hardens against the attack by
> modifying the mmapped status record.
The overall patches looks good to me and I have no objections, while I
have some slight opinions to them in a place of sound driver developer.
> The first patch does the basic job in the core PCM side,
The main concern is indirect accessing to state field via some pointer
hops. I think addition of helper macro at first step eases centre of your
work, like:
```
diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 8c48a5bce88c..f6a160cb8135 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -669,6 +669,20 @@ void snd_pcm_stream_unlock_irqrestore(struct snd_pcm_substream *substream,
stream <= SNDRV_PCM_STREAM_LAST; \
stream++)
+/**
+ * snd_pcm_stream_state - Return state in runtime of the PCM substream.
+ * @substream: substream to check. runtime should be attached.
+ *
+ * Return state in runtime of the PCM substream. The substream should exists and
+ * runtime should be attached to it.
+ */
+static inline snd_pcm_state_t snd_pcm_stream_state(const snd_pcm_substream *substream)
+{
+ snd_BUG_ON(!(sub) || !(sub)->runtime);
+
+ return substream->runtime->status->state;
+}
```
As we can see, sound driver programmer sometimes checks state of runtime
in their code, thus the macro could helps them as well as centre of your
change.
> and the
> second patch flips the PCM status mmap to read-only for hardening,
The patch looks good to me as well as the patch for asihpi driver, and
should be in your tree independent from the others.
> while the remaining patches are for drivers to follow the core
> change.
>
> The conversions are straightforward. In most places, it's just
> replacing runtime->status->state with runtime->state.
The usage of mentioned macro can control the concerned access. Now
addition of new field, `state` to runtime structure can be done easily.
> Takashi
>
> ===
>
> Takashi Iwai (11):
> ALSA: pcm: Avoid reference to status->state
> ALSA: pcm: Make mmap status read-only
> ALSA: aloop: Replace runtime->status->state reference to
> runtime->state
> ALSA: firewire: Replace runtime->status->state reference to
> runtime->state
> ALSA: hda: Replace runtime->status->state reference to runtime->state
> ALSA: asihpi: Replace runtime->status->state reference to
> runtime->state
> ALSA: usb-audio: Replace runtime->status->state reference to
> runtime->state
> ALSA: usx2y: Replace runtime->status->state reference to
> runtime->state
> ASoC: intel: Replace runtime->status->state reference to
> runtime->state
> ASoC: sh: Replace runtime->status->state reference to runtime->state
> usb: gadget: Replace runtime->status->state reference to
> runtime->state
>
> drivers/usb/gadget/function/u_uac1_legacy.c | 4 +-
> include/sound/pcm.h | 20 ++-
> sound/core/oss/pcm_oss.c | 42 +++----
> sound/core/pcm.c | 9 +-
> sound/core/pcm_compat.c | 4 +-
> sound/core/pcm_lib.c | 16 +--
> sound/core/pcm_native.c | 128 ++++++++++----------
> sound/drivers/aloop.c | 4 +-
> sound/firewire/bebob/bebob_pcm.c | 4 +-
> sound/firewire/dice/dice-pcm.c | 4 +-
> sound/firewire/digi00x/digi00x-pcm.c | 4 +-
> sound/firewire/fireface/ff-pcm.c | 4 +-
> sound/firewire/fireworks/fireworks_pcm.c | 4 +-
> sound/firewire/motu/motu-pcm.c | 4 +-
> sound/firewire/oxfw/oxfw-pcm.c | 8 +-
> sound/firewire/tascam/tascam-pcm.c | 4 +-
> sound/hda/hdmi_chmap.c | 2 +-
> sound/pci/asihpi/asihpi.c | 2 +-
> sound/soc/intel/skylake/skl-pcm.c | 4 +-
> sound/soc/sh/rz-ssi.c | 2 +-
> sound/usb/pcm.c | 4 +-
> sound/usb/usx2y/usbusx2yaudio.c | 3 +-
> sound/usb/usx2y/usx2yhwdeppcm.c | 3 +-
> 23 files changed, 150 insertions(+), 133 deletions(-)
>
> ===
>
> Cc: Bard Liao <yung-chuan.liao@xxxxxxxxxxxxxxx>
> Cc: Cezary Rojewski <cezary.rojewski@xxxxxxxxx>
> Cc: Felipe Balbi <balbi@xxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: Kai Vehmanen <kai.vehmanen@xxxxxxxxxxxxxxx>
> Cc: Liam Girdwood <liam.r.girdwood@xxxxxxxxxxxxxxx>
> Cc: Mark Brown <broonie@xxxxxxxxxx>
> Cc: Peter Ujfalusi <peter.ujfalusi@xxxxxxxxxxxxxxx>
> Cc: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> Cc: Ranjani Sridharan <ranjani.sridharan@xxxxxxxxxxxxxxx>
> Cc: Takashi Sakamoto <o-takashi@xxxxxxxxxxxxx>
>
> --
> 2.35.3
Regards
Takashi Sakamoto
