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

[Liam Girdwood]

On Wed, 2019-03-20 at 09:45 +0100, Guennadi Liakhovetski wrote:
> 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. 

> 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?
> The same pattern is also repeated in other IPC implementations.
> 

Best to send a fix.

Thanks

Liam

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