| 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 |
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 >
> 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 --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; }