On 06/07/2022 10:01, Dan Carpenter wrote:
> The bug here is that when we copy the payload the value of *ppos should
> be zero but it is sizeof(ipc4_msg->header_u64) instead. That means that
> the buffer will be copied to the wrong location within the
> ipc4_msg->data_ptr buffer.
>
> Really, in this context, it is simpler and more appropriate to use
> copy_from_user() instead of simple_write_to_buffer() so I have
> re-written the function.
>
> Fixes: 066c67624d8c ("ASoC: SOF: ipc-msg-injector: Add support for IPC4 messages")
> Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> ---
> From static analysis. Not tested. I believe this function is mostly
> used to write random junk to the device and see what breaks. So
> probably it works fine as-is.
>
> sound/soc/sof/sof-client-ipc-msg-injector.c | 27 ++++++++-------------
> 1 file changed, 10 insertions(+), 17 deletions(-)
>
> diff --git a/sound/soc/sof/sof-client-ipc-msg-injector.c b/sound/soc/sof/sof-client-ipc-msg-injector.c
> index 6bdfa527b7f7..e8ab173e95b5 100644
> --- a/sound/soc/sof/sof-client-ipc-msg-injector.c
> +++ b/sound/soc/sof/sof-client-ipc-msg-injector.c
> @@ -181,7 +181,7 @@ static ssize_t sof_msg_inject_ipc4_dfs_write(struct file *file,
> struct sof_client_dev *cdev = file->private_data;
> struct sof_msg_inject_priv *priv = cdev->data;
> struct sof_ipc4_msg *ipc4_msg = priv->tx_buffer;
> - ssize_t size;
> + size_t data_size;
> int ret;
>
> if (*ppos)
> @@ -191,25 +191,18 @@ static ssize_t sof_msg_inject_ipc4_dfs_write(struct file *file,
> return -EINVAL;
>
> /* copy the header first */
> - size = simple_write_to_buffer(&ipc4_msg->header_u64,
> - sizeof(ipc4_msg->header_u64),
> - ppos, buffer, count);
> - if (size < 0)
> - return size;
> - if (size != sizeof(ipc4_msg->header_u64))
> + if (copy_from_user(&ipc4_msg->header_u64, buffer,
> + sizeof(ipc4_msg->header_u64)))
> return -EFAULT;
>
> - count -= size;
> + data_size = count - sizeof(ipc4_msg->header_u64);
> + if (data_size > priv->max_msg_size)
> + return -EINVAL;
> /* Copy the payload */
> - size = simple_write_to_buffer(ipc4_msg->data_ptr,
> - priv->max_msg_size, ppos, buffer,
> - count);
> - if (size < 0)
> - return size;
> - if (size != count)
> + if (copy_from_user(ipc4_msg->data_ptr, buffer, data_size))
I think this is still needs an update:
if (copy_from_user(ipc4_msg->data_ptr,
buffer + sizeof(ipc4_msg->header_u64), data_size))
To skip the already copied header.
Or without a rewrite the fix would be simple as:
/* Copy the payload */
size = simple_write_to_buffer(ipc4_msg->data_ptr, 0,
buffer + sizeof(ipc4_msg->header_u64), data_size),
count);
> return -EFAULT;
>
> - ipc4_msg->data_size = count;
> + ipc4_msg->data_size = data_size;
>
> /* Initialize the reply storage */
> ipc4_msg = priv->rx_buffer;
> @@ -221,9 +214,9 @@ static ssize_t sof_msg_inject_ipc4_dfs_write(struct file *file,
>
> /* return the error code if test failed */
> if (ret < 0)
> - size = ret;
> + count = ret;
>
> - return size;
> + return count;
> };
>
> static int sof_msg_inject_dfs_release(struct inode *inode, struct file *file)
--
Péter
