From: Peng Fan <[email protected]>
The i.MX8 SCU message header size is the number of "u32" elements,
not "u8", so fix the check.
Reported-by: coverity-bot <[email protected]>
Addresses-Coverity-ID: 1461658 ("Memory - corruptions")
Signed-off-by: Peng Fan <[email protected]>
---
V2:
I not include the fixes tag, since this patch still in next tree.
drivers/mailbox/imx-mailbox.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
index 7906624a731c..c2398cb63ea0 100644
--- a/drivers/mailbox/imx-mailbox.c
+++ b/drivers/mailbox/imx-mailbox.c
@@ -154,12 +154,12 @@ static int imx_mu_scu_tx(struct imx_mu_priv *priv,
switch (cp->type) {
case IMX_MU_TYPE_TX:
- if (msg->hdr.size > sizeof(*msg)) {
+ if (msg->hdr.size > (sizeof(*msg) / 4)) {
/*
* The real message size can be different to
* struct imx_sc_rpc_msg_max size
*/
- dev_err(priv->dev, "Exceed max msg size (%zu) on TX, got: %i\n", sizeof(*msg), msg->hdr.size);
+ dev_err(priv->dev, "Exceed max msg size (%zu) on TX, got: %i\n", sizeof(*msg) / 4, msg->hdr.size);
return -EINVAL;
}
@@ -198,9 +198,9 @@ static int imx_mu_scu_rx(struct imx_mu_priv *priv,
imx_mu_xcr_rmw(priv, 0, IMX_MU_xCR_RIEn(0));
*data++ = imx_mu_read(priv, priv->dcfg->xRR[0]);
- if (msg.hdr.size > sizeof(msg)) {
+ if (msg.hdr.size > (sizeof(msg) / 4)) {
dev_err(priv->dev, "Exceed max msg size (%zu) on RX, got: %i\n",
- sizeof(msg), msg.hdr.size);
+ sizeof(msg) / 4, msg.hdr.size);
return -EINVAL;
}
--
2.16.4
> Subject: Re: [PATCH] mailbox: imx-mailbox: fix scu msg header size check
>
> On Tue, Apr 14, 2020 at 04:10:26PM +0800, [email protected] wrote:
> > From: Peng Fan <[email protected]>
> >
> > The i.MX8 SCU message header size is the number of "u32" elements, not
> > "u8", so fix the check.
> >
> > Reported-by: coverity-bot <[email protected]>
> > Addresses-Coverity-ID: 1461658 ("Memory - corruptions")
> > Signed-off-by: Peng Fan <[email protected]>
> > ---
> >
> > V2:
> > I not include the fixes tag, since this patch still in next tree.
> >
> > drivers/mailbox/imx-mailbox.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/mailbox/imx-mailbox.c
> > b/drivers/mailbox/imx-mailbox.c index 7906624a731c..c2398cb63ea0
> > 100644
> > --- a/drivers/mailbox/imx-mailbox.c
> > +++ b/drivers/mailbox/imx-mailbox.c
> > @@ -154,12 +154,12 @@ static int imx_mu_scu_tx(struct imx_mu_priv
> > *priv,
> >
> > switch (cp->type) {
> > case IMX_MU_TYPE_TX:
> > - if (msg->hdr.size > sizeof(*msg)) {
> > + if (msg->hdr.size > (sizeof(*msg) / 4)) {
>
> No need for the parenthesis. Maybe a comment would be helpful here,
> something like:
>
> /*
> * msg->hdr.size specifies the number of u32 words while sizeof
> * yields bytes.
> */
V2 will have the update.
>
> > /*
> > * The real message size can be different to
> > * struct imx_sc_rpc_msg_max size
> > */
> > - dev_err(priv->dev, "Exceed max msg size (%zu) on TX,
> got: %i\n", sizeof(*msg), msg->hdr.size);
> > + dev_err(priv->dev, "Exceed max msg size (%zu) on TX,
> got: %i\n",
> > +sizeof(*msg) / 4, msg->hdr.size);
>
> The unit here is also "number of u32 words", maybe bytes is more natural?
ok. Will change to msg->hdr.size << 2 keeping sizeof(*msg).
> And I suggesting specifying the unit in the error message.
Is this ok to you?
dev_err(priv->dev, "Exceed max msg size (%zu) on TX, got: %i,
msg->hdr.size: %i\n", sizeof(*msg), msg->hdr.size << 2, msg->hdr.size);
Thanks,
Peng.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-K?nig
> |
> Industrial Linux Solutions |
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.
> pengutronix.de%2F&data=02%7C01%7Cpeng.fan%40nxp.com%7Ca6a32
> 1daf8f84601a28808d7e04d8def%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> 7C0%7C0%7C637224496010304343&sdata=GebTJ82O2xOf52yISwVZTM
> 6s2q%2Blar533PAGnm%2FAPHI%3D&reserved=0 |
On Tue, Apr 14, 2020 at 04:10:26PM +0800, [email protected] wrote:
> From: Peng Fan <[email protected]>
>
> The i.MX8 SCU message header size is the number of "u32" elements,
> not "u8", so fix the check.
>
> Reported-by: coverity-bot <[email protected]>
> Addresses-Coverity-ID: 1461658 ("Memory - corruptions")
> Signed-off-by: Peng Fan <[email protected]>
> ---
>
> V2:
> I not include the fixes tag, since this patch still in next tree.
>
> drivers/mailbox/imx-mailbox.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> index 7906624a731c..c2398cb63ea0 100644
> --- a/drivers/mailbox/imx-mailbox.c
> +++ b/drivers/mailbox/imx-mailbox.c
> @@ -154,12 +154,12 @@ static int imx_mu_scu_tx(struct imx_mu_priv *priv,
>
> switch (cp->type) {
> case IMX_MU_TYPE_TX:
> - if (msg->hdr.size > sizeof(*msg)) {
> + if (msg->hdr.size > (sizeof(*msg) / 4)) {
No need for the parenthesis. Maybe a comment would be helpful here,
something like:
/*
* msg->hdr.size specifies the number of u32 words while sizeof
* yields bytes.
*/
> /*
> * The real message size can be different to
> * struct imx_sc_rpc_msg_max size
> */
> - dev_err(priv->dev, "Exceed max msg size (%zu) on TX, got: %i\n", sizeof(*msg), msg->hdr.size);
> + dev_err(priv->dev, "Exceed max msg size (%zu) on TX, got: %i\n", sizeof(*msg) / 4, msg->hdr.size);
The unit here is also "number of u32 words", maybe bytes is more
natural? And I suggesting specifying the unit in the error message.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Tue, Apr 14, 2020 at 08:40:19AM +0000, Peng Fan wrote:
> > Subject: Re: [PATCH] mailbox: imx-mailbox: fix scu msg header size check
> >
> > On Tue, Apr 14, 2020 at 04:10:26PM +0800, [email protected] wrote:
> > > From: Peng Fan <[email protected]>
> > >
> > > The i.MX8 SCU message header size is the number of "u32" elements, not
> > > "u8", so fix the check.
> > >
> > > Reported-by: coverity-bot <[email protected]>
> > > Addresses-Coverity-ID: 1461658 ("Memory - corruptions")
> > > Signed-off-by: Peng Fan <[email protected]>
> > > ---
> > >
> > > V2:
> > > I not include the fixes tag, since this patch still in next tree.
> > >
> > > drivers/mailbox/imx-mailbox.c | 8 ++++----
> > > 1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/mailbox/imx-mailbox.c
> > > b/drivers/mailbox/imx-mailbox.c index 7906624a731c..c2398cb63ea0
> > > 100644
> > > --- a/drivers/mailbox/imx-mailbox.c
> > > +++ b/drivers/mailbox/imx-mailbox.c
> > > @@ -154,12 +154,12 @@ static int imx_mu_scu_tx(struct imx_mu_priv
> > > *priv,
> > >
> > > switch (cp->type) {
> > > case IMX_MU_TYPE_TX:
> > > - if (msg->hdr.size > sizeof(*msg)) {
> > > + if (msg->hdr.size > (sizeof(*msg) / 4)) {
> >
> > No need for the parenthesis. Maybe a comment would be helpful here,
> > something like:
> >
> > /*
> > * msg->hdr.size specifies the number of u32 words while sizeof
> > * yields bytes.
> > */
>
> V2 will have the update.
>
> >
> > > /*
> > > * The real message size can be different to
> > > * struct imx_sc_rpc_msg_max size
> > > */
> > > - dev_err(priv->dev, "Exceed max msg size (%zu) on TX,
> > got: %i\n", sizeof(*msg), msg->hdr.size);
> > > + dev_err(priv->dev, "Exceed max msg size (%zu) on TX,
> > got: %i\n",
> > > +sizeof(*msg) / 4, msg->hdr.size);
> >
> > The unit here is also "number of u32 words", maybe bytes is more natural?
>
> ok. Will change to msg->hdr.size << 2 keeping sizeof(*msg).
>
> > And I suggesting specifying the unit in the error message.
>
> Is this ok to you?
> dev_err(priv->dev, "Exceed max msg size (%zu) on TX, got: %i,
> msg->hdr.size: %i\n", sizeof(*msg), msg->hdr.size << 2, msg->hdr.size);
I'd prefer:
dev_err(priv->dev, "Maximal message size (%zu bytes) exceeded on TX; got: %i bytes\n"
. Duplicating the value doesn't add much value.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |