-----Original Message----- From: Sound-open-firmware [mailto:sound-open-firmware-bounces@alsa- project.org] On Behalf Of Guennadi Liakhovetski Sent: Wednesday, March 20, 2019 9:46 AM To: sound-open-firmware@alsa-project.org Cc: Bossart, Pierre-louis pierre-louis.bossart@intel.com Subject: [Sound-open-firmware] suspicious code in hda-ipc.c

Hi,

This code in hda_dsp_ipc_get_reply() in hda-ipc.c looks suspicious to me:

hdr = msg->msg_data; if (hdr->cmd == (SOF_IPC_GLB_PM_MSG | SOF_IPC_PM_CTX_SAVE)) { ... } else { /* get IPC reply from DSP in the mailbox */ sof_mailbox_read(sdev, sdev->host_box.offset, &reply, sizeof(reply)); } if (reply.error < 0) { size = sizeof(reply); ret = reply.error; } else { ... }

/* read the message */ if (msg->msg_data && size > 0) sof_mailbox_read(sdev, sdev->host_box.offset, msg->reply_data, size);

So, as far as I understand, first of all the first part of the "if (msg->msg_data && ...)" test is always true, since we've already dereferenced "hdr" above.

I think just checking "if (size > 0)" here is fine.

Secondly if reply contains a negative error the reply struct will be read from the mailbox again, instead of just using "memcpy(msg->reply_data, &reply, sizeof(reply));" to copy it?

I just checked the latest FW code, looks we are actually not sending reply_data neither it is success or not, so reading reply_data here actually looks odd.

I think we should add a member e.g. reply_data_size into struct sof_ipc_reply, then we only need to read this msg->reply_data when this size is set to non-0 in the replier.

Thanks, ~Keyon

The same pattern is also repeated in other IPC implementations.

Thanks Guennadi _______________________________________________ Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware