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

[Dan Carpenter <dan.carpenter@oracle.com>]

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