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

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)