Re: out of bounds access on array error_text[] because of -ETIMEDOUT return from __send_command()

[Markus Mayer]

On Wed, 19 Aug 2020 at 11:34, Florian Fainelli <f.fainelli@xxxxxxxxx> 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@xxxxxxxxxxxx>
> > 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