| Message ID | 1586851826-16596-1-git-send-email-peng.fan@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mailbox: imx-mailbox: fix scu msg header size check | expand |
On Tue, Apr 14, 2020 at 04:10:26PM +0800, peng.fan@nxp.com wrote: > From: Peng Fan <peng.fan@nxp.com> > > The i.MX8 SCU message header size is the number of "u32" elements, > not "u8", so fix the check. > > Reported-by: coverity-bot <keescook+coverity-bot@chromium.org> > Addresses-Coverity-ID: 1461658 ("Memory - corruptions") > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > > 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
> Subject: Re: [PATCH] mailbox: imx-mailbox: fix scu msg header size check > > On Tue, Apr 14, 2020 at 04:10:26PM +0800, peng.fan@nxp.com wrote: > > From: Peng Fan <peng.fan@nxp.com> > > > > The i.MX8 SCU message header size is the number of "u32" elements, not > > "u8", so fix the check. > > > > Reported-by: coverity-bot <keescook+coverity-bot@chromium.org> > > Addresses-Coverity-ID: 1461658 ("Memory - corruptions") > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > --- > > > > 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 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, peng.fan@nxp.com wrote: > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > The i.MX8 SCU message header size is the number of "u32" elements, not > > > "u8", so fix the check. > > > > > > Reported-by: coverity-bot <keescook+coverity-bot@chromium.org> > > > Addresses-Coverity-ID: 1461658 ("Memory - corruptions") > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > > --- > > > > > > 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
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; }