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

[Guennadi Liakhovetski]

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

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",
> >