* out of bounds access on array error_text[] because of -ETIMEDOUT return from __send_command()
@ 2020-08-18 12:21 Colin Ian King
2020-08-19 18:34 ` Florian Fainelli
0 siblings, 1 reply; 4+ messages in thread
From: Colin Ian King @ 2020-08-18 12:21 UTC (permalink / raw)
To: Markus Mayer, bcm-kernel-feedback-list, Krzysztof Kozlowski,
Florian Fainelli, linux-arm-kernel
Cc: linux-kernel
Hi,
static analysis with coverity has found a buffer overflow issue with the
brcmstb driver, I believe it may have been introduced with the following
commit:
commit a7c25759d8d84b64c437a78f05df7314b02934e5
Author: Markus Mayer <mmayer@broadcom.com>
Date: Tue Apr 2 16:01:00 2019 -0700
memory: brcmstb: dpfe: wait for DCPU to be ready
The static analysis is as follows for the source file
/drivers/memory/brcmstb_dpfe.c :
684 static ssize_t generic_show(unsigned int command, u32 response[],
685 struct brcmstb_dpfe_priv *priv, char *buf)
686 {
687 int ret;
688
1. Condition !priv, taking false branch.
689 if (!priv)
690 return sprintf(buf, "ERROR: driver private data not
set\n");
691
2. return_constant: Function call __send_command(priv, command,
response) may return -110.
3. assignment: Assigning: ret = __send_command(priv, command,
response). The value of ret is now -110.
692 ret = __send_command(priv, command, response);
4. Condition ret < 0, taking true branch.
693 if (ret < 0)
Out-of-bounds read (OVERRUN)
5. overrun-local: Overrunning array error_text of 6 8-byte elements
at element index 110 (byte offset 887) using index -ret (which evaluates
to 110).
694 return sprintf(buf, "ERROR: %s\n", error_text[-ret]);
695
696 return 0;
697 }
Function __send_command() can return -ETIMEDOUT and this causes an
out-of-bounds access on error_text[].
Colin
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: out of bounds access on array error_text[] because of -ETIMEDOUT return from __send_command()
2020-08-18 12:21 out of bounds access on array error_text[] because of -ETIMEDOUT return from __send_command() Colin Ian King
@ 2020-08-19 18:34 ` Florian Fainelli
2020-08-20 17:23 ` Markus Mayer
0 siblings, 1 reply; 4+ messages in thread
From: Florian Fainelli @ 2020-08-19 18:34 UTC (permalink / raw)
To: Colin Ian King, Markus Mayer, bcm-kernel-feedback-list,
Krzysztof Kozlowski, Florian Fainelli, linux-arm-kernel
Cc: linux-kernel
On 8/18/20 5:21 AM, Colin Ian King wrote:
> Hi,
>
> static analysis with coverity has found a buffer overflow issue with the
> brcmstb driver, I believe it may have been introduced with the following
> commit:
>
> commit a7c25759d8d84b64c437a78f05df7314b02934e5
> Author: Markus Mayer <mmayer@broadcom.com>
> Date: Tue Apr 2 16:01:00 2019 -0700
>
> memory: brcmstb: dpfe: wait for DCPU to be ready
>
> The static analysis is as follows for the source file
> /drivers/memory/brcmstb_dpfe.c :
>
> 684 static ssize_t generic_show(unsigned int command, u32 response[],
> 685 struct brcmstb_dpfe_priv *priv, char *buf)
> 686 {
> 687 int ret;
> 688
> 1. Condition !priv, taking false branch.
>
> 689 if (!priv)
> 690 return sprintf(buf, "ERROR: driver private data not
> set\n");
> 691
> 2. return_constant: Function call __send_command(priv, command,
> response) may return -110.
> 3. assignment: Assigning: ret = __send_command(priv, command,
> response). The value of ret is now -110.
>
> 692 ret = __send_command(priv, command, response);
> 4. Condition ret < 0, taking true branch.
>
> 693 if (ret < 0)
>
> Out-of-bounds read (OVERRUN)
> 5. overrun-local: Overrunning array error_text of 6 8-byte elements
> at element index 110 (byte offset 887) using index -ret (which evaluates
> to 110).
> 694 return sprintf(buf, "ERROR: %s\n", error_text[-ret]);
> 695
> 696 return 0;
> 697 }
>
>
> Function __send_command() can return -ETIMEDOUT and this causes an
> out-of-bounds access on error_text[].
Markus, what do you think of this:
diff --git a/drivers/memory/brcmstb_dpfe.c b/drivers/memory/brcmstb_dpfe.c
index 60e8633b1175..b41c6251ddc3 100644
--- a/drivers/memory/brcmstb_dpfe.c
+++ b/drivers/memory/brcmstb_dpfe.c
@@ -445,7 +445,7 @@ static int __send_command(struct brcmstb_dpfe_priv
*priv, unsigned int cmd,
}
if (resp != 0) {
mutex_unlock(&priv->lock);
- return -ETIMEDOUT;
+ return -ffs(DCPU_RET_ERR_TIMEDOUT);
}
/* Compute checksum over the message */
That way we only return DCPU-style error code from __send_command and we
de-reference error_text accordingly? Or we could just introduce a proper
lookup with a function instead of a direct array de-reference.
--
Florian
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: out of bounds access on array error_text[] because of -ETIMEDOUT return from __send_command()
2020-08-19 18:34 ` Florian Fainelli
@ 2020-08-20 17:23 ` Markus Mayer
2020-08-21 1:04 ` Markus Mayer
0 siblings, 1 reply; 4+ messages in thread
From: Markus Mayer @ 2020-08-20 17:23 UTC (permalink / raw)
To: Florian Fainelli
Cc: Colin Ian King, BCM Kernel Feedback, Krzysztof Kozlowski,
Linux ARM Kernel, linux-kernel
On Wed, 19 Aug 2020 at 11:34, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> On 8/18/20 5:21 AM, Colin Ian King wrote:
> > Hi,
> >
> > static analysis with coverity has found a buffer overflow issue with the
> > brcmstb driver, I believe it may have been introduced with the following
> > commit:
> >
> > commit a7c25759d8d84b64c437a78f05df7314b02934e5
> > Author: Markus Mayer <mmayer@broadcom.com>
> > Date: Tue Apr 2 16:01:00 2019 -0700
> >
> > memory: brcmstb: dpfe: wait for DCPU to be ready
> >
> > The static analysis is as follows for the source file
> > /drivers/memory/brcmstb_dpfe.c :
> >
> > 684 static ssize_t generic_show(unsigned int command, u32 response[],
> > 685 struct brcmstb_dpfe_priv *priv, char *buf)
> > 686 {
> > 687 int ret;
> > 688
> > 1. Condition !priv, taking false branch.
> >
> > 689 if (!priv)
> > 690 return sprintf(buf, "ERROR: driver private data not
> > set\n");
> > 691
> > 2. return_constant: Function call __send_command(priv, command,
> > response) may return -110.
> > 3. assignment: Assigning: ret = __send_command(priv, command,
> > response). The value of ret is now -110.
> >
> > 692 ret = __send_command(priv, command, response);
> > 4. Condition ret < 0, taking true branch.
> >
> > 693 if (ret < 0)
> >
> > Out-of-bounds read (OVERRUN)
> > 5. overrun-local: Overrunning array error_text of 6 8-byte elements
> > at element index 110 (byte offset 887) using index -ret (which evaluates
> > to 110).
> > 694 return sprintf(buf, "ERROR: %s\n", error_text[-ret]);
> > 695
> > 696 return 0;
> > 697 }
> >
> >
> > Function __send_command() can return -ETIMEDOUT and this causes an
> > out-of-bounds access on error_text[].
>
> Markus, what do you think of this:
>
> diff --git a/drivers/memory/brcmstb_dpfe.c b/drivers/memory/brcmstb_dpfe.c
> index 60e8633b1175..b41c6251ddc3 100644
> --- a/drivers/memory/brcmstb_dpfe.c
> +++ b/drivers/memory/brcmstb_dpfe.c
> @@ -445,7 +445,7 @@ static int __send_command(struct brcmstb_dpfe_priv
> *priv, unsigned int cmd,
> }
> if (resp != 0) {
> mutex_unlock(&priv->lock);
> - return -ETIMEDOUT;
> + return -ffs(DCPU_RET_ERR_TIMEDOUT);
> }
>
> /* Compute checksum over the message */
>
> That way we only return DCPU-style error code from __send_command and we
> de-reference error_text accordingly? Or we could just introduce a proper
> lookup with a function instead of a direct array de-reference.
Let me do some experiments. What you are proposing should work and is
in line with the current code. A lookup function might be cleaner,
though.
Thanks,
-Markus
> --
> Florian
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: out of bounds access on array error_text[] because of -ETIMEDOUT return from __send_command()
2020-08-20 17:23 ` Markus Mayer
@ 2020-08-21 1:04 ` Markus Mayer
0 siblings, 0 replies; 4+ messages in thread
From: Markus Mayer @ 2020-08-21 1:04 UTC (permalink / raw)
To: Florian Fainelli
Cc: Colin Ian King, BCM Kernel Feedback, Krzysztof Kozlowski,
Linux ARM Kernel, linux-kernel
On Thu, 20 Aug 2020 at 10:23, Markus Mayer <mmayer@broadcom.com> wrote:
>
> On Wed, 19 Aug 2020 at 11:34, Florian Fainelli <f.fainelli@gmail.com> wrote:
> >
> > On 8/18/20 5:21 AM, Colin Ian King wrote:
> > > Hi,
> > >
> > > static analysis with coverity has found a buffer overflow issue with the
> > > brcmstb driver, I believe it may have been introduced with the following
> > > commit:
> > >
> > > commit a7c25759d8d84b64c437a78f05df7314b02934e5
> > > Author: Markus Mayer <mmayer@broadcom.com>
> > > Date: Tue Apr 2 16:01:00 2019 -0700
> > >
> > > memory: brcmstb: dpfe: wait for DCPU to be ready
> > >
> > > The static analysis is as follows for the source file
> > > /drivers/memory/brcmstb_dpfe.c :
> > >
> > > 684 static ssize_t generic_show(unsigned int command, u32 response[],
> > > 685 struct brcmstb_dpfe_priv *priv, char *buf)
> > > 686 {
> > > 687 int ret;
> > > 688
> > > 1. Condition !priv, taking false branch.
> > >
> > > 689 if (!priv)
> > > 690 return sprintf(buf, "ERROR: driver private data not
> > > set\n");
> > > 691
> > > 2. return_constant: Function call __send_command(priv, command,
> > > response) may return -110.
> > > 3. assignment: Assigning: ret = __send_command(priv, command,
> > > response). The value of ret is now -110.
> > >
> > > 692 ret = __send_command(priv, command, response);
> > > 4. Condition ret < 0, taking true branch.
> > >
> > > 693 if (ret < 0)
> > >
> > > Out-of-bounds read (OVERRUN)
> > > 5. overrun-local: Overrunning array error_text of 6 8-byte elements
> > > at element index 110 (byte offset 887) using index -ret (which evaluates
> > > to 110).
> > > 694 return sprintf(buf, "ERROR: %s\n", error_text[-ret]);
> > > 695
> > > 696 return 0;
> > > 697 }
> > >
> > >
> > > Function __send_command() can return -ETIMEDOUT and this causes an
> > > out-of-bounds access on error_text[].
> >
> > Markus, what do you think of this:
> >
> > diff --git a/drivers/memory/brcmstb_dpfe.c b/drivers/memory/brcmstb_dpfe.c
> > index 60e8633b1175..b41c6251ddc3 100644
> > --- a/drivers/memory/brcmstb_dpfe.c
> > +++ b/drivers/memory/brcmstb_dpfe.c
> > @@ -445,7 +445,7 @@ static int __send_command(struct brcmstb_dpfe_priv
> > *priv, unsigned int cmd,
> > }
> > if (resp != 0) {
> > mutex_unlock(&priv->lock);
> > - return -ETIMEDOUT;
> > + return -ffs(DCPU_RET_ERR_TIMEDOUT);
> > }
> >
> > /* Compute checksum over the message */
> >
> > That way we only return DCPU-style error code from __send_command and we
> > de-reference error_text accordingly? Or we could just introduce a proper
> > lookup with a function instead of a direct array de-reference.
>
> Let me do some experiments. What you are proposing should work and is
> in line with the current code. A lookup function might be cleaner,
> though.
I submitted a patch for review. See https://lkml.org/lkml/2020/8/20/2291.
Regards,
-Markus
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-08-21 1:05 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-18 12:21 out of bounds access on array error_text[] because of -ETIMEDOUT return from __send_command() Colin Ian King
2020-08-19 18:34 ` Florian Fainelli
2020-08-20 17:23 ` Markus Mayer
2020-08-21 1:04 ` Markus Mayer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®