[bug report] ASoC: SOF: ipc-msg-injector: Separate the message sending

[Péter Ujfalusi]

On 02/06/2022 12:15, Dan Carpenter wrote:
> Hello Peter Ujfalusi,
> 
> The patch a9aa3381e404: "ASoC: SOF: ipc-msg-injector: Separate the
> message sending" from May 6, 2022, leads to the following Smatch
> static checker warning:
> 
> 	sound/soc/sof/sof-client-ipc-msg-injector.c:162 sof_msg_inject_dfs_write()
> 	warn: kernel error codes cast to unsigned 'size'
> 
> sound/soc/sof/sof-client-ipc-msg-injector.c
>     148 static ssize_t sof_msg_inject_dfs_write(struct file *file, const char __user *buffer,
>     149                                         size_t count, loff_t *ppos)
>     150 {
>     151         struct sof_client_dev *cdev = file->private_data;
>     152         struct sof_msg_inject_priv *priv = cdev->data;
>     153         size_t size;
>     154         int ret;
>     155 
>     156         if (*ppos)
>     157                 return 0;
> 
> I think there needs to be an "if (count != priv->max_msg_size)" check
> or something.  Or another option would be the do a memset()

The interface is used to feed in crafted IPC messages to torture the
firmware (and in some level the kernel as well). How it will handle
deliberately ill crafted messages, what it will do if a valid but
unexpected message is sent, etc.

The only check I could think of is to prevent
less than sizeof(struct sof_ipc_cmd_hdr) count writes, but one could
argue that sending a normal header (u32 size, u32 cmd) followed by only
changing the size is also a valid shortcut.

> 
> 	memset(priv->tx_buffer, 0, priv->max_msg_size);
> 
> before the simple_write_to_buffer().  Otherwise if count == 1 then we
> re-use stale data.

If count is < sizeof(struct sof_ipc_cmd_hdr) to be precise, but even if
that passes there could be stale data in the buffer for message types
where there is extended payload.

Yes, there could be more size check, but the injector must not interpret
the message, it should not block invalid messages.

Let me think a bit on the minimum count check for a moment...

> 
>     158 
>     159         size = simple_write_to_buffer(priv->tx_buffer, priv->max_msg_size,
>     160                                       ppos, buffer, count);
>     161         if (size != count)
> --> 162                 return size > 0 ? -EFAULT : size;
>     163 
>     164         memset(priv->rx_buffer, 0, priv->max_msg_size);
>     165 
>     166         ret = sof_msg_inject_send_message(cdev);
>     167 
>     168         /* return the error code if test failed */
>     169         if (ret < 0)
>     170                 size = ret;
>     171 
>     172         return size;
>     173 };
> 
> regards,
> dan carpenter

-- 
Péter