Messages in this thread | ![/](/images/icornerl.gif) | | From | Markus Mayer <> | Date | Thu, 20 Aug 2020 18:04:52 -0700 | Subject | Re: out of bounds access on array error_text[] because of -ETIMEDOUT return from __send_command() |
| |
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
| ![\](/images/icornerr.gif) |