alsa-devel.alsa-project.org archive mirror
 help / color / mirror / Atom feed
* [alsa-devel] [PATCH 0/2] [RFC] ASoC: topology: fix access beyond data
@ 2019-10-07  8:41 Guennadi Liakhovetski
  2019-10-07  8:41 ` [alsa-devel] [PATCH 1/2] ASoC: topology: protect against accessing beyond loaded topology data Guennadi Liakhovetski
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Guennadi Liakhovetski @ 2019-10-07  8:41 UTC (permalink / raw)
  To: alsa-devel

This is likely incomplete, I haven't reviewed the whole of the
topology parsing code, and I haven't tested this yet - neither for
actually being a problem without these patches nor for fixing them.
This is a preview to give everybody a chance to showt out if I
misunderstood something and there isn't actually a problem to be
fixed.

Thanks
Guennadi

Guennadi Liakhovetski (2):
  ASoC: topology: protect against accessing beyond loaded topology data
  ASoC: topology: don't access beyond topology data

 sound/soc/soc-topology.c | 37 +++++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)

-- 
1.9.3

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [alsa-devel] [PATCH 1/2] ASoC: topology: protect against accessing beyond loaded topology data
  2019-10-07  8:41 [alsa-devel] [PATCH 0/2] [RFC] ASoC: topology: fix access beyond data Guennadi Liakhovetski
@ 2019-10-07  8:41 ` Guennadi Liakhovetski
  2019-10-07 14:17   ` Pierre-Louis Bossart
  2019-10-07  8:41 ` [alsa-devel] [PATCH 2/2] ASoC: topology: don't access beyond " Guennadi Liakhovetski
  2019-10-07  9:13 ` [alsa-devel] [PATCH 0/2] [RFC] ASoC: topology: fix access beyond data Guennadi Liakhovetski
  2 siblings, 1 reply; 6+ messages in thread
From: Guennadi Liakhovetski @ 2019-10-07  8:41 UTC (permalink / raw)
  To: alsa-devel

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@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;
+	}
+
 	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",
-- 
1.9.3

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [alsa-devel] [PATCH 2/2] ASoC: topology: don't access beyond topology data
  2019-10-07  8:41 [alsa-devel] [PATCH 0/2] [RFC] ASoC: topology: fix access beyond data Guennadi Liakhovetski
  2019-10-07  8:41 ` [alsa-devel] [PATCH 1/2] ASoC: topology: protect against accessing beyond loaded topology data Guennadi Liakhovetski
@ 2019-10-07  8:41 ` Guennadi Liakhovetski
  2019-10-07  9:13 ` [alsa-devel] [PATCH 0/2] [RFC] ASoC: topology: fix access beyond data Guennadi Liakhovetski
  2 siblings, 0 replies; 6+ messages in thread
From: Guennadi Liakhovetski @ 2019-10-07  8:41 UTC (permalink / raw)
  To: alsa-devel

When loading kcontrol elements make sure to first check the size of
available data before accessing it.

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
---
 sound/soc/soc-topology.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index d1d3c6f..f933ad4 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -1115,11 +1115,11 @@ static int soc_tplg_kcontrol_elems_load(struct soc_tplg *tplg,
 	struct snd_soc_tplg_hdr *hdr)
 {
 	struct snd_soc_tplg_ctl_hdr *control_hdr;
+	ssize_t remainder = le32_to_cpu(hdr->payload_size);
 	int i;
 
 	if (tplg->pass != SOC_TPLG_PASS_MIXER) {
-		tplg->pos += le32_to_cpu(hdr->size) +
-			le32_to_cpu(hdr->payload_size);
+		tplg->pos += le32_to_cpu(hdr->size) + remainder;
 		return 0;
 	}
 
@@ -1130,6 +1130,11 @@ static int soc_tplg_kcontrol_elems_load(struct soc_tplg *tplg,
 
 		control_hdr = (struct snd_soc_tplg_ctl_hdr *)tplg->pos;
 
+		if (remainder < sizeof(*control_hdr)) {
+			dev_err(tplg->dev, "ASoC: invalid payload size\n");
+			return -EINVAL;
+		}
+
 		if (le32_to_cpu(control_hdr->size) != sizeof(*control_hdr)) {
 			dev_err(tplg->dev, "ASoC: invalid control size\n");
 			return -EINVAL;
@@ -1143,25 +1148,24 @@ static int soc_tplg_kcontrol_elems_load(struct soc_tplg *tplg,
 		case SND_SOC_TPLG_CTL_RANGE:
 		case SND_SOC_TPLG_DAPM_CTL_VOLSW:
 		case SND_SOC_TPLG_DAPM_CTL_PIN:
-			soc_tplg_dmixer_create(tplg, 1,
-					       le32_to_cpu(hdr->payload_size));
+			soc_tplg_dmixer_create(tplg, 1, remainder);
 			break;
 		case SND_SOC_TPLG_CTL_ENUM:
 		case SND_SOC_TPLG_CTL_ENUM_VALUE:
 		case SND_SOC_TPLG_DAPM_CTL_ENUM_DOUBLE:
 		case SND_SOC_TPLG_DAPM_CTL_ENUM_VIRT:
 		case SND_SOC_TPLG_DAPM_CTL_ENUM_VALUE:
-			soc_tplg_denum_create(tplg, 1,
-					      le32_to_cpu(hdr->payload_size));
+			soc_tplg_denum_create(tplg, 1, remainder);
 			break;
 		case SND_SOC_TPLG_CTL_BYTES:
-			soc_tplg_dbytes_create(tplg, 1,
-					       le32_to_cpu(hdr->payload_size));
+			soc_tplg_dbytes_create(tplg, 1, remainder);
 			break;
 		default:
 			soc_bind_err(tplg, control_hdr, i);
 			return -EINVAL;
 		}
+
+		remainder -= tplg->pos - (u8 *)control_hdr;
 	}
 
 	return 0;
-- 
1.9.3

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [alsa-devel] [PATCH 0/2] [RFC] ASoC: topology: fix access beyond data
  2019-10-07  8:41 [alsa-devel] [PATCH 0/2] [RFC] ASoC: topology: fix access beyond data Guennadi Liakhovetski
  2019-10-07  8:41 ` [alsa-devel] [PATCH 1/2] ASoC: topology: protect against accessing beyond loaded topology data Guennadi Liakhovetski
  2019-10-07  8:41 ` [alsa-devel] [PATCH 2/2] ASoC: topology: don't access beyond " Guennadi Liakhovetski
@ 2019-10-07  9:13 ` Guennadi Liakhovetski
  2 siblings, 0 replies; 6+ messages in thread
From: Guennadi Liakhovetski @ 2019-10-07  9:13 UTC (permalink / raw)
  To: alsa-devel; +Cc: Liam Girdwood

Sorry, I missed a couple of Cc, it was the first time I used
git send-email from this PC.

Thanks
Guennadi

On Mon, Oct 07, 2019 at 10:41:31AM +0200, Guennadi Liakhovetski wrote:
> This is likely incomplete, I haven't reviewed the whole of the
> topology parsing code, and I haven't tested this yet - neither for
> actually being a problem without these patches nor for fixing them.
> This is a preview to give everybody a chance to showt out if I
> misunderstood something and there isn't actually a problem to be
> fixed.
> 
> Thanks
> Guennadi
> 
> Guennadi Liakhovetski (2):
>   ASoC: topology: protect against accessing beyond loaded topology data
>   ASoC: topology: don't access beyond topology data
> 
>  sound/soc/soc-topology.c | 37 +++++++++++++++++++++++++++++--------
>  1 file changed, 29 insertions(+), 8 deletions(-)
> 
> -- 
> 1.9.3
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [alsa-devel] [PATCH 1/2] ASoC: topology: protect against accessing beyond loaded topology data
  2019-10-07  8:41 ` [alsa-devel] [PATCH 1/2] ASoC: topology: protect against accessing beyond loaded topology data Guennadi Liakhovetski
@ 2019-10-07 14:17   ` Pierre-Louis Bossart
  2019-10-07 14:42     ` Guennadi Liakhovetski
  0 siblings, 1 reply; 6+ messages in thread
From: Pierre-Louis Bossart @ 2019-10-07 14:17 UTC (permalink / raw)
  To: Guennadi Liakhovetski, alsa-devel; +Cc: Takashi Iwai, Mark Brown

[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@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",
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [alsa-devel] [PATCH 1/2] ASoC: topology: protect against accessing beyond loaded topology data
  2019-10-07 14:17   ` Pierre-Louis Bossart
@ 2019-10-07 14:42     ` Guennadi Liakhovetski
  0 siblings, 0 replies; 6+ messages in thread
From: Guennadi Liakhovetski @ 2019-10-07 14:42 UTC (permalink / raw)
  To: Pierre-Louis Bossart; +Cc: Takashi Iwai, alsa-devel, Mark Brown

Hi Pierre,

On Mon, Oct 07, 2019 at 09:17:42AM -0500, Pierre-Louis Bossart wrote:
> [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@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).

I think we do. The second comparison is unsigned, so, it won't wrap around to
also cover the first case. The first comparison is true if "remainder" is
"small negative" as you correctly point out, i.e. large positive in unsigned
arithmetics. So, the second test wouldn't catch it. And the return value is
different anyway, so, we need two tests.

Thanks
Guennadi

> > +
> >   	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",
> > 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-10-07 14:43 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-07  8:41 [alsa-devel] [PATCH 0/2] [RFC] ASoC: topology: fix access beyond data Guennadi Liakhovetski
2019-10-07  8:41 ` [alsa-devel] [PATCH 1/2] ASoC: topology: protect against accessing beyond loaded topology data Guennadi Liakhovetski
2019-10-07 14:17   ` Pierre-Louis Bossart
2019-10-07 14:42     ` Guennadi Liakhovetski
2019-10-07  8:41 ` [alsa-devel] [PATCH 2/2] ASoC: topology: don't access beyond " Guennadi Liakhovetski
2019-10-07  9:13 ` [alsa-devel] [PATCH 0/2] [RFC] ASoC: topology: fix access beyond data Guennadi Liakhovetski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).