diff mbox series

remoteproc: imx_rproc: validate resource table

Message ID 20220111033333.403448-2-peng.fan@oss.nxp.com (mailing list archive)
State Changes Requested
Headers show
Series remoteproc: imx_rproc: validate resource table | expand

Commit Message

Peng Fan (OSS) Jan. 11, 2022, 3:33 a.m. UTC
From: Peng Fan <peng.fan@nxp.com>

Currently NXP use one device tree to support all NXP released Cortex-M
demos. There is one simple demo that not need to communicate with
Linux, thus it will not update the resource table. So there maybe
garbage data in it. In such case, Linux should directly ignore it.

It is hard to decide what data is garbage data, NXP released SDK use
ver(1), reserved(0) in a valid resource table. But in case others
use different value, so here use 0xff as a max value for ver and num.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/imx_rproc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Mathieu Poirier Jan. 17, 2022, 6:25 p.m. UTC | #1
Good morning,

On Tue, Jan 11, 2022 at 11:33:23AM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Currently NXP use one device tree to support all NXP released Cortex-M
> demos. There is one simple demo that not need to communicate with
> Linux, thus it will not update the resource table. So there maybe
> garbage data in it. In such case, Linux should directly ignore it.
> 
> It is hard to decide what data is garbage data, NXP released SDK use
> ver(1), reserved(0) in a valid resource table. But in case others
> use different value, so here use 0xff as a max value for ver and num.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/imx_rproc.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 0bd24c937a73..75fde16f80a4 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -490,11 +490,19 @@ static int imx_rproc_attach(struct rproc *rproc)
>  static struct resource_table *imx_rproc_get_loaded_rsc_table(struct rproc *rproc, size_t *table_sz)
>  {
>  	struct imx_rproc *priv = rproc->priv;
> +	struct resource_table *table;
>  
>  	/* The resource table has already been mapped in imx_rproc_addr_init */
>  	if (!priv->rsc_table)
>  		return NULL;
>  
> +	table = priv->rsc_table;
> +	/* Gabage data check */
> +	if (table->ver >= 0xff || table->num >= 0xff || table->reserved[0] || table->reserved[1]) {
> +		dev_err(priv->dev, "Ignore invalid rsc table\n");
> +		return NULL;
> +	}
> +

This seems like the wrong fix to me.  Either use different DTs or update the
resource table for all demos - efficiency should not be a problem since they are
demos.  With the above it is only a matter of time before the pattern
associated with valid resource tables changes, leading to more hacks that will be
impossible to maintain over time.

Thanks,
Mathieu

>  	*table_sz = SZ_1K;
>  	return (struct resource_table *)priv->rsc_table;
>  }
> -- 
> 2.25.1
>
Peng Fan Jan. 18, 2022, 1:24 a.m. UTC | #2
> Subject: Re: [PATCH] remoteproc: imx_rproc: validate resource table
> 
> Good morning,
> 
> On Tue, Jan 11, 2022 at 11:33:23AM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > Currently NXP use one device tree to support all NXP released Cortex-M
> > demos. There is one simple demo that not need to communicate with
> > Linux, thus it will not update the resource table. So there maybe
> > garbage data in it. In such case, Linux should directly ignore it.
> >
> > It is hard to decide what data is garbage data, NXP released SDK use
> > ver(1), reserved(0) in a valid resource table. But in case others use
> > different value, so here use 0xff as a max value for ver and num.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/remoteproc/imx_rproc.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/remoteproc/imx_rproc.c
> > b/drivers/remoteproc/imx_rproc.c index 0bd24c937a73..75fde16f80a4
> > 100644
> > --- a/drivers/remoteproc/imx_rproc.c
> > +++ b/drivers/remoteproc/imx_rproc.c
> > @@ -490,11 +490,19 @@ static int imx_rproc_attach(struct rproc *rproc)
> > static struct resource_table *imx_rproc_get_loaded_rsc_table(struct
> > rproc *rproc, size_t *table_sz)  {
> >  	struct imx_rproc *priv = rproc->priv;
> > +	struct resource_table *table;
> >
> >  	/* The resource table has already been mapped in imx_rproc_addr_init
> */
> >  	if (!priv->rsc_table)
> >  		return NULL;
> >
> > +	table = priv->rsc_table;
> > +	/* Gabage data check */
> > +	if (table->ver >= 0xff || table->num >= 0xff || table->reserved[0] ||
> table->reserved[1]) {
> > +		dev_err(priv->dev, "Ignore invalid rsc table\n");
> > +		return NULL;
> > +	}
> > +
> 
> This seems like the wrong fix to me.  Either use different DTs or update the
> resource table for all demos - efficiency should not be a problem since they
> are demos.  With the above it is only a matter of time before the pattern
> associated with valid resource tables changes, leading to more hacks that will
> be impossible to maintain over time.

I see, drop this patch :)

Thanks,
Peng.

> 
> Thanks,
> Mathieu
> 
> >  	*table_sz = SZ_1K;
> >  	return (struct resource_table *)priv->rsc_table;  }
> > --
> > 2.25.1
> >
diff mbox series

Patch

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 0bd24c937a73..75fde16f80a4 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -490,11 +490,19 @@  static int imx_rproc_attach(struct rproc *rproc)
 static struct resource_table *imx_rproc_get_loaded_rsc_table(struct rproc *rproc, size_t *table_sz)
 {
 	struct imx_rproc *priv = rproc->priv;
+	struct resource_table *table;
 
 	/* The resource table has already been mapped in imx_rproc_addr_init */
 	if (!priv->rsc_table)
 		return NULL;
 
+	table = priv->rsc_table;
+	/* Gabage data check */
+	if (table->ver >= 0xff || table->num >= 0xff || table->reserved[0] || table->reserved[1]) {
+		dev_err(priv->dev, "Ignore invalid rsc table\n");
+		return NULL;
+	}
+
 	*table_sz = SZ_1K;
 	return (struct resource_table *)priv->rsc_table;
 }