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