[Sound-open-firmware] suspicious code in hda-ipc.c

[Jie, Yang]

>-----Original Message-----
>From: Sound-open-firmware [mailto:sound-open-firmware-bounces at alsa-
>project.org] On Behalf Of Guennadi Liakhovetski
>Sent: Wednesday, March 20, 2019 9:46 AM
>To: sound-open-firmware at alsa-project.org
>Cc: Bossart, Pierre-louis <pierre-louis.bossart at 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 at alsa-project.org
>https://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware