Re: [PATCH] ASoC: SOF: ipc-msg-injector: fix copy in sof_msg_inject_ipc4_dfs_write()

["Péter Ujfalusi" <peter.ujfalusi@linux.intel.com>]

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@oracle.com>
> ---
> 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