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

On Wed, Jul 06, 2022 at 11:55:33AM +0300, Péter Ujfalusi wrote:

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.

Oh yeah. Thanks. Will do.

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);

That doesn't work. Simple_write_to_buffer takes a pointer and not a value. So it's pretty common to do:

llof_t dummy = 0;

...

size = simple_write_to_buffer(ipc4_msg->data_ptr, &dummy, buffer + sizeof(ipc4_msg->header_u64), data_size), count);

But the simple_write_to_buffer() function is kind of bad. The only thing which saves us from a security nightmare is that it's mostly used in debugfs so it's root only.

The problem with simple_write_to_buffer is that it leads to uninitialized variable bugs if we don't zero out the buffer at the start of the function. It returns a positive success value if it is able to write even one byte somewhere in the buffer.

The start of the buffer can be uninitialized if *ppos is non-zero.

I think I have looked through every caller and I have not seen one where we care about *ppos. A bunch do the dummy = 0 thing. For the rest we could just add an "if (*ppos) return 0;" at the start of the function. These functions are writing a line. Just do it in one go. It's not like the rest of the write functions are set up to handle partial writes.

So in other words, you can find a code which will accept a non-zero *ppos and it's not going to crash, but the data from the user is invariably going to nonsense.

The end part of the buffer can be uninitialized. A lot of callers address this by adding a check:

size = simple_write_to_buffer(); if (size != buf_size) return size < 0 ? size : -EIO;

That ensures that *ppos is zero and that it was able to initialize the whole buffer. But if you want to fill a whole buffer then just use copy_from_user().

The only advantage of simple_write_to_buffer() is that it takes both the size of the buffer and the size of write into account. However it's probably better to just do it manually/explicitly:

if (*ppos) return 0;

...

if (count > size_of_buf) count = size_of_buf; if (copy_from_user(buf, user, count)) return -EFAULT;

Looking at the sof_msg_inject_ipc4_dfs_write() function again, we check that:

size = simple_write_to_buffer( ... if (size != count) return -EFAULT;

This ensures that we wrote as much as expected, but it doesn't ensure that we filled the buffer. But since this function is just use to inject garbage data as part of fuzzing then re-using old data is fine.

regards, dan carpenter