[7/8] ASoC: SOF: fix range checks

Commit Message

Kai Vehmanen Sept. 17, 2020, 10:56 a.m. UTC

From: Guennadi Liakhovetski <[e-mail]>

On multiple locations checks are performed of untrusted values after adding
a constant to them. This is wrong, because the addition might overflow and
the result can then pass the check, although the original value is invalid.
Fix multiple such issues by checking the actual value and not a sum of it
and a constant.

Signed-off-by: Guennadi Liakhovetski <[e-mail]>
Reviewed-by: Ranjani Sridharan <[e-mail]>
Reviewed-by: Pierre-Louis Bossart <[e-mail]>
Signed-off-by: Kai Vehmanen <[e-mail]>
---
 sound/soc/sof/control.c  | 38 ++++++++++++++++++++++----------------
 sound/soc/sof/topology.c | 20 +++++++++++++-------
 2 files changed, 35 insertions(+), 23 deletions(-)

Patch

diff --git a/sound/soc/sof/control.c b/sound/soc/sof/control.c
index 63ead9ebc4c6..58f8c998e6af 100644
--- a/sound/soc/sof/control.c
+++ b/sound/soc/sof/control.c
@@ -229,14 +229,16 @@  int snd_sof_bytes_get(struct snd_kcontrol *kcontrol,
 		return -EINVAL;
 	}
 
-	size = data->size + sizeof(*data);
-	if (size > be->max) {
+	/* be->max has been verified to be >= sizeof(struct sof_abi_hdr) */
+	if (data->size > be->max - sizeof(*data)) {
 		dev_err_ratelimited(scomp->dev,
-				    "error: DSP sent %zu bytes max is %d\n",
-				    size, be->max);
+				    "error: %u bytes of control data is invalid, max is %zu\n",
+				    data->size, be->max - sizeof(*data));
 		return -EINVAL;
 	}
 
+	size = data->size + sizeof(*data);
+
 	/* copy back to kcontrol */
 	memcpy(ucontrol->value.bytes.data, data, size);
 
@@ -252,7 +254,7 @@  int snd_sof_bytes_put(struct snd_kcontrol *kcontrol,
 	struct snd_soc_component *scomp = scontrol->scomp;
 	struct sof_ipc_ctrl_data *cdata = scontrol->control_data;
 	struct sof_abi_hdr *data = cdata->data;
-	size_t size = data->size + sizeof(*data);
+	size_t size;
 
 	if (be->max > sizeof(ucontrol->value.bytes.data)) {
 		dev_err_ratelimited(scomp->dev,
@@ -261,13 +263,16 @@  int snd_sof_bytes_put(struct snd_kcontrol *kcontrol,
 		return -EINVAL;
 	}
 
-	if (size > be->max) {
+	/* be->max has been verified to be >= sizeof(struct sof_abi_hdr) */
+	if (data->size > be->max - sizeof(*data)) {
 		dev_err_ratelimited(scomp->dev,
-				    "error: size too big %zu bytes max is %d\n",
-				    size, be->max);
+				    "error: data size too big %u bytes max is %zu\n",
+				    data->size, be->max - sizeof(*data));
 		return -EINVAL;
 	}
 
+	size = data->size + sizeof(*data);
+
 	/* copy from kcontrol */
 	memcpy(data, ucontrol->value.bytes.data, size);
 
@@ -334,7 +339,8 @@  int snd_sof_bytes_ext_put(struct snd_kcontrol *kcontrol,
 		return -EINVAL;
 	}
 
-	if (cdata->data->size + sizeof(const struct sof_abi_hdr) > be->max) {
+	/* be->max has been verified to be >= sizeof(struct sof_abi_hdr) */
+	if (cdata->data->size > be->max - sizeof(const struct sof_abi_hdr)) {
 		dev_err_ratelimited(scomp->dev, "error: Mismatch in ABI data size (truncated?).\n");
 		return -EINVAL;
 	}
@@ -420,7 +426,7 @@  int snd_sof_bytes_ext_get(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_tlv header;
 	struct snd_ctl_tlv __user *tlvd =
 		(struct snd_ctl_tlv __user *)binary_data;
-	int data_size;
+	size_t data_size;
 
 	/*
 	 * Decrement the limit by ext bytes header size to
@@ -432,16 +438,16 @@  int snd_sof_bytes_ext_get(struct snd_kcontrol *kcontrol,
 	cdata->data->magic = SOF_ABI_MAGIC;
 	cdata->data->abi = SOF_ABI_VERSION;
 
-	/* Prevent read of other kernel data or possibly corrupt response */
-	data_size = cdata->data->size + sizeof(const struct sof_abi_hdr);
-
 	/* check data size doesn't exceed max coming from topology */
-	if (data_size > be->max) {
-		dev_err_ratelimited(scomp->dev, "error: user data size %d exceeds max size %d.\n",
-				    data_size, be->max);
+	if (cdata->data->size > be->max - sizeof(const struct sof_abi_hdr)) {
+		dev_err_ratelimited(scomp->dev, "error: user data size %d exceeds max size %zu.\n",
+				    cdata->data->size,
+				    be->max - sizeof(const struct sof_abi_hdr));
 		return -EINVAL;
 	}
 
+	data_size = cdata->data->size + sizeof(const struct sof_abi_hdr);
+
 	header.numid = scontrol->cmd;
 	header.length = data_size;
 	if (copy_to_user(tlvd, &header, sizeof(const struct snd_ctl_tlv)))
diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c
index fa85a22b5880..eaa1122d5a68 100644
--- a/sound/soc/sof/topology.c
+++ b/sound/soc/sof/topology.c
@@ -1150,20 +1150,26 @@  static int sof_control_load_bytes(struct snd_soc_component *scomp,
 	struct snd_soc_tplg_bytes_control *control =
 		container_of(hdr, struct snd_soc_tplg_bytes_control, hdr);
 	struct soc_bytes_ext *sbe = (struct soc_bytes_ext *)kc->private_value;
-	int max_size = sbe->max;
+	size_t max_size = sbe->max;
+	size_t priv_size = le32_to_cpu(control->priv.size);
 	int ret;
 
-	/* init the get/put bytes data */
-	scontrol->size = sizeof(struct sof_ipc_ctrl_data) +
-		le32_to_cpu(control->priv.size);
+	if (max_size < sizeof(struct sof_ipc_ctrl_data) ||
+	    max_size < sizeof(struct sof_abi_hdr)) {
+		ret = -EINVAL;
+		goto out;
+	}
 
-	if (scontrol->size > max_size) {
-		dev_err(scomp->dev, "err: bytes data size %d exceeds max %d.\n",
-			scontrol->size, max_size);
+	/* init the get/put bytes data */
+	if (priv_size > max_size - sizeof(struct sof_ipc_ctrl_data)) {
+		dev_err(scomp->dev, "err: bytes data size %zu exceeds max %zu.\n",
+			priv_size, max_size - sizeof(struct sof_ipc_ctrl_data));
 		ret = -EINVAL;
 		goto out;
 	}
 
+	scontrol->size = sizeof(struct sof_ipc_ctrl_data) + priv_size;
+
 	scontrol->control_data = kzalloc(max_size, GFP_KERNEL);
 	cdata = scontrol->control_data;
 	if (!scontrol->control_data) {