Re: [PATCH v3 02/14] ASoC: SOF: Add Sound Open Firmware KControl support

[Takashi Iwai <tiwai@suse.de>]

On Tue, 11 Dec 2018 22:23:06 +0100,
Pierre-Louis Bossart wrote:
> 
> +int snd_sof_enum_get(struct snd_kcontrol *kcontrol,
> +		     struct snd_ctl_elem_value *ucontrol)
> +{
....
> +	/* read back each channel */
> +	for (i = 0; i < channels; i++)
> +		ucontrol->value.integer.value[i] = cdata->chanv[i].value;

enum type needs to access ucontrol->value.enumerated.item[i].
This has a different size, hence using integer.value[] would be broken
on BE archs.

> +int snd_sof_enum_put(struct snd_kcontrol *kcontrol,
> +		     struct snd_ctl_elem_value *ucontrol)
> +{
....
> +	/* update each channel */
> +	for (i = 0; i < channels; i++)
> +		cdata->chanv[i].value = ucontrol->value.integer.value[i];

Ditto.

> +int snd_sof_bytes_get(struct snd_kcontrol *kcontrol,
> +		      struct snd_ctl_elem_value *ucontrol)
> +{
....
> +	size = data->size + sizeof(*data);
> +	if (size > be->max) {
> +		dev_err(sdev->dev, "error: DSP sent %zu bytes max is %d\n",
> +			size, be->max);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	/* copy back to kcontrol */
> +	memcpy(ucontrol->value.bytes.data, data, size);

I *hope* that the data size max was already examined not to exceed
ucontrol data array size beforehand.  But a sanity check to catch a
buffer overflow here won't hurt.
Ditto for *_put().

> +int snd_sof_bytes_ext_put(struct snd_kcontrol *kcontrol,
> +			  const unsigned int __user *binary_data,
> +			  unsigned int size)
> +{
> +	struct soc_bytes_ext *be =
> +		(struct soc_bytes_ext *)kcontrol->private_value;
> +	struct snd_sof_control *scontrol = be->dobj.private;
> +	struct snd_sof_dev *sdev = scontrol->sdev;
> +	struct sof_ipc_ctrl_data *cdata = scontrol->control_data;
> +	struct snd_ctl_tlv header;
> +	struct snd_ctl_tlv __user *tlvd =
> +		(struct snd_ctl_tlv __user *)binary_data;

Don't drop const.

> +	int ret;
> +	int err;
> +	int max_size = SOF_IPC_MSG_MAX_SIZE -
> +		sizeof(const struct sof_ipc_ctrl_data);
> +
> +	ret = pm_runtime_get_sync(sdev->dev);
> +	if (ret < 0) {
> +		dev_err(sdev->dev, "error: bytes_ext put failed to resume %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	/* The beginning of bytes data contains a header from where
> +	 * the length (as bytes) is needed to know the correct copy
> +	 * length of data from tlvd->tlv.
> +	 */
> +	if (copy_from_user(&header, tlvd, sizeof(const struct snd_ctl_tlv))) {
> +		ret = -EFAULT;
> +		goto out;
> +	}
> +	/* The maximum length that can be copied is limited by IPC max
> +	 * length and topology defined length for ext bytes control.
> +	 */
> +	max_size = (be->max < max_size) ? be->max : max_size;
> +	if (header.length > max_size) {
> +		dev_err(sdev->dev, "error: Bytes data size %d exceeds max %d.\n",
> +			header.length, max_size);
> +		ret = -EINVAL;
> +		goto out;

Here user can pass a malicious data, and printing the error at each
time would flood the kernel log.  The error message can be dropped or
make debug, or use ratelimited version.
Ditto for the rest checks.


thanks,

Takashi