ASoC: SOF: fix range checks

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 <guennadi.liakhovetski@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Link: https://lore.kernel.org/r/20200917105633.2579047-8-kai.vehmanen@linux.intel.com
Signed-off-by: Mark Brown <broonie@kernel.org>

1 files changed, 22 insertions, 16 deletions

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