[PATCH 0/3] ASoC: SOF: Intel: Do not process IPC reply before firmware boot
Hi,
By mistake a developer managed to create a 'corrupted' IPC4 firmware image which loaded fine to the DSP and after boot it sent an IPC reply before we would have received the FW_READY message. It turned out that the image was an IPC3 firmware and the IPC reply was the IPC3 FW_READY notification message which got understood as an IPC4 reply message due to the difference between the two IPC mechanism.
This caused a NULL pointer dereference since the reply memory will be allocated after the FW_READY message.
To make sure this will not bite again, skip any spurious reply messages before the FW_READY.
Regards, Peter --- Peter Ujfalusi (3): ASoC: SOF: Intel: cnl: Do not process IPC reply before firmware boot ASoC: SOF: Intel: hda-ipc: Do not process IPC reply before firmware boot ASoC: SOF: Intel: mtl: Do not process IPC reply before firmware boot
sound/soc/sof/intel/cnl.c | 37 +++++++++++++++++++++------------ sound/soc/sof/intel/hda-ipc.c | 39 ++++++++++++++++++++++------------- sound/soc/sof/intel/mtl.c | 20 +++++++++++------- 3 files changed, 62 insertions(+), 34 deletions(-)
It is not yet clear, but it is possible to create a firmware so broken that it will send a reply message before a FW_READY message (it is not yet clear if FW_READY will arrive later). Since the reply_data is allocated only after the FW_READY message, this will lead to a NULL pointer dereference if not filtered out.
The issue was reported with IPC4 firmware but the same condition is present for IPC3.
Reported-by: Kai Vehmanen kai.vehmanen@linux.intel.com Signed-off-by: Peter Ujfalusi peter.ujfalusi@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/sof/intel/cnl.c | 37 ++++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 13 deletions(-)
diff --git a/sound/soc/sof/intel/cnl.c b/sound/soc/sof/intel/cnl.c index ccf46fcd6c9a..a064453f0bc3 100644 --- a/sound/soc/sof/intel/cnl.c +++ b/sound/soc/sof/intel/cnl.c @@ -60,17 +60,23 @@ irqreturn_t cnl_ipc4_irq_thread(int irq, void *context)
if (primary & SOF_IPC4_MSG_DIR_MASK) { /* Reply received */ - struct sof_ipc4_msg *data = sdev->ipc->msg.reply_data; + if (likely(sdev->fw_state == SOF_FW_BOOT_COMPLETE)) { + struct sof_ipc4_msg *data = sdev->ipc->msg.reply_data;
- data->primary = primary; - data->extension = extension; + data->primary = primary; + data->extension = extension;
- spin_lock_irq(&sdev->ipc_lock); + spin_lock_irq(&sdev->ipc_lock);
- snd_sof_ipc_get_reply(sdev); - snd_sof_ipc_reply(sdev, data->primary); + snd_sof_ipc_get_reply(sdev); + snd_sof_ipc_reply(sdev, data->primary);
- spin_unlock_irq(&sdev->ipc_lock); + spin_unlock_irq(&sdev->ipc_lock); + } else { + dev_dbg_ratelimited(sdev->dev, + "IPC reply before FW_READY: %#x|%#x\n", + primary, extension); + } } else { /* Notification received */ notification_data.primary = primary; @@ -124,15 +130,20 @@ irqreturn_t cnl_ipc_irq_thread(int irq, void *context) CNL_DSP_REG_HIPCCTL, CNL_DSP_REG_HIPCCTL_DONE, 0);
- spin_lock_irq(&sdev->ipc_lock); + if (likely(sdev->fw_state == SOF_FW_BOOT_COMPLETE)) { + spin_lock_irq(&sdev->ipc_lock);
- /* handle immediate reply from DSP core */ - hda_dsp_ipc_get_reply(sdev); - snd_sof_ipc_reply(sdev, msg); + /* handle immediate reply from DSP core */ + hda_dsp_ipc_get_reply(sdev); + snd_sof_ipc_reply(sdev, msg);
- cnl_ipc_dsp_done(sdev); + cnl_ipc_dsp_done(sdev);
- spin_unlock_irq(&sdev->ipc_lock); + spin_unlock_irq(&sdev->ipc_lock); + } else { + dev_dbg_ratelimited(sdev->dev, "IPC reply before FW_READY: %#x\n", + msg); + }
ipc_irq = true; }
It is not yet clear, but it is possible to create a firmware so broken that it will send a reply message before a FW_READY message (it is not yet clear if FW_READY will arrive later). Since the reply_data is allocated only after the FW_READY message, this will lead to a NULL pointer dereference if not filtered out.
The issue was reported with IPC4 firmware but the same condition is present for IPC3.
Reported-by: Kai Vehmanen kai.vehmanen@linux.intel.com Signed-off-by: Peter Ujfalusi peter.ujfalusi@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/sof/intel/hda-ipc.c | 39 ++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 14 deletions(-)
diff --git a/sound/soc/sof/intel/hda-ipc.c b/sound/soc/sof/intel/hda-ipc.c index f08011249955..65e688f749ea 100644 --- a/sound/soc/sof/intel/hda-ipc.c +++ b/sound/soc/sof/intel/hda-ipc.c @@ -148,17 +148,23 @@ irqreturn_t hda_dsp_ipc4_irq_thread(int irq, void *context)
if (primary & SOF_IPC4_MSG_DIR_MASK) { /* Reply received */ - struct sof_ipc4_msg *data = sdev->ipc->msg.reply_data; + if (likely(sdev->fw_state == SOF_FW_BOOT_COMPLETE)) { + struct sof_ipc4_msg *data = sdev->ipc->msg.reply_data;
- data->primary = primary; - data->extension = extension; + data->primary = primary; + data->extension = extension;
- spin_lock_irq(&sdev->ipc_lock); + spin_lock_irq(&sdev->ipc_lock);
- snd_sof_ipc_get_reply(sdev); - snd_sof_ipc_reply(sdev, data->primary); + snd_sof_ipc_get_reply(sdev); + snd_sof_ipc_reply(sdev, data->primary);
- spin_unlock_irq(&sdev->ipc_lock); + spin_unlock_irq(&sdev->ipc_lock); + } else { + dev_dbg_ratelimited(sdev->dev, + "IPC reply before FW_READY: %#x|%#x\n", + primary, extension); + } } else { /* Notification received */
@@ -225,16 +231,21 @@ irqreturn_t hda_dsp_ipc_irq_thread(int irq, void *context) * place, the message might not yet be marked as expecting a * reply. */ - spin_lock_irq(&sdev->ipc_lock); + if (likely(sdev->fw_state == SOF_FW_BOOT_COMPLETE)) { + spin_lock_irq(&sdev->ipc_lock);
- /* handle immediate reply from DSP core */ - hda_dsp_ipc_get_reply(sdev); - snd_sof_ipc_reply(sdev, msg); + /* handle immediate reply from DSP core */ + hda_dsp_ipc_get_reply(sdev); + snd_sof_ipc_reply(sdev, msg);
- /* set the done bit */ - hda_dsp_ipc_dsp_done(sdev); + /* set the done bit */ + hda_dsp_ipc_dsp_done(sdev);
- spin_unlock_irq(&sdev->ipc_lock); + spin_unlock_irq(&sdev->ipc_lock); + } else { + dev_dbg_ratelimited(sdev->dev, "IPC reply before FW_READY: %#x\n", + msg); + }
ipc_irq = true; }
It is not yet clear, but it is possible to create a firmware so broken that it will send a reply message before a FW_READY message (it is not yet clear if FW_READY will arrive later). Since the reply_data is allocated only after the FW_READY message, this will lead to a NULL pointer dereference if not filtered out.
Reported-by: Kai Vehmanen kai.vehmanen@linux.intel.com Signed-off-by: Peter Ujfalusi peter.ujfalusi@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/sof/intel/mtl.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/sound/soc/sof/intel/mtl.c b/sound/soc/sof/intel/mtl.c index 3a043589c12b..a5e244ea0688 100644 --- a/sound/soc/sof/intel/mtl.c +++ b/sound/soc/sof/intel/mtl.c @@ -512,17 +512,23 @@ static irqreturn_t mtl_ipc_irq_thread(int irq, void *context) */ if (primary & SOF_IPC4_MSG_DIR_MASK) { /* Reply received */ - struct sof_ipc4_msg *data = sdev->ipc->msg.reply_data; + if (likely(sdev->fw_state == SOF_FW_BOOT_COMPLETE)) { + struct sof_ipc4_msg *data = sdev->ipc->msg.reply_data;
- data->primary = primary; - data->extension = extension; + data->primary = primary; + data->extension = extension;
- spin_lock_irq(&sdev->ipc_lock); + spin_lock_irq(&sdev->ipc_lock);
- snd_sof_ipc_get_reply(sdev); - snd_sof_ipc_reply(sdev, data->primary); + snd_sof_ipc_get_reply(sdev); + snd_sof_ipc_reply(sdev, data->primary);
- spin_unlock_irq(&sdev->ipc_lock); + spin_unlock_irq(&sdev->ipc_lock); + } else { + dev_dbg_ratelimited(sdev->dev, + "IPC reply before FW_READY: %#x|%#x\n", + primary, extension); + } } else { /* Notification received */ notification_data.primary = primary;
On Tue, 12 Jul 2022 15:23:54 +0300, Peter Ujfalusi wrote:
By mistake a developer managed to create a 'corrupted' IPC4 firmware image which loaded fine to the DSP and after boot it sent an IPC reply before we would have received the FW_READY message. It turned out that the image was an IPC3 firmware and the IPC reply was the IPC3 FW_READY notification message which got understood as an IPC4 reply message due to the difference between the two IPC mechanism.
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/3] ASoC: SOF: Intel: cnl: Do not process IPC reply before firmware boot commit: acacd9eefd0def5a83244d88e5483b5f38ee7287 [2/3] ASoC: SOF: Intel: hda-ipc: Do not process IPC reply before firmware boot commit: 499cc881b09c8283ab5e75b0d6d21cb427722161 [3/3] ASoC: SOF: Intel: mtl: Do not process IPC reply before firmware boot commit: 1549a69b89b7e5b1b830da897529344766728a4b
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (2)
-
Mark Brown -
Peter Ujfalusi