[alsa-devel] [PATCH 1/2] ASoC: topology: protect against accessing beyond loaded topology data

[Pierre-Louis Bossart]

[Adding Mark and Takashi in Cc: ]

On 10/7/19 3:41 AM, Guennadi Liakhovetski wrote:
> Add checks for sufficient topology data length before accessing data
> according to its internal structure. Without these checks malformed
> or corrupted topology images can lead to accessing invalid addresses
> in the kernel.
> 
> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski at linux.intel.com>
> ---
>   sound/soc/soc-topology.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
> diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
> index 0fd0329..d1d3c6f 100644
> --- a/sound/soc/soc-topology.c
> +++ b/sound/soc/soc-topology.c
> @@ -2501,9 +2501,18 @@ static int soc_tplg_manifest_load(struct soc_tplg *tplg,
>   static int soc_valid_header(struct soc_tplg *tplg,
>   	struct snd_soc_tplg_hdr *hdr)
>   {
> +	size_t remainder = tplg->fw->size - soc_tplg_get_hdr_offset(tplg);
> +
>   	if (soc_tplg_get_hdr_offset(tplg) >= tplg->fw->size)
>   		return 0;
>   
> +	/* Check that we have enough data before accessing the header */
> +	if (remainder < sizeof(*hdr)) {
> +		dev_err(tplg->dev, "ASoC: insufficient %zd bytes.\n",
> +			remainder);
> +		return -EINVAL;
> +	}

do we still need the first test above? This only tests that remainder is 
<= 0, which is covered in the second added case (with the wrap-around).

> +
>   	if (le32_to_cpu(hdr->size) != sizeof(*hdr)) {
>   		dev_err(tplg->dev,
>   			"ASoC: invalid header size for type %d at offset 0x%lx size 0x%zx.\n",
> @@ -2546,6 +2555,14 @@ static int soc_valid_header(struct soc_tplg *tplg,
>   		return -EINVAL;
>   	}
>   
> +	if (le32_to_cpu(hdr->payload_size) + sizeof(*hdr) > remainder) {
> +		dev_err(tplg->dev,
> +			"ASoC: payload size %zu too large at offset 0x%lx.\n",
> +			le32_to_cpu(hdr->payload_size),
> +			soc_tplg_get_hdr_offset(tplg));
> +		return -EINVAL;
> +	}
> +
>   	if (tplg->pass == le32_to_cpu(hdr->type))
>   		dev_dbg(tplg->dev,
>   			"ASoC: Got 0x%x bytes of type %d version %d vendor %d at pass %d\n",
>