linux-kernel - Re: out of bounds access on array error_text[] because of -ETIMEDOUT return from __send

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4acc34c4-9c17-41f7-2e39-7eda16792572@gmail.com>
Date:   Wed, 19 Aug 2020 11:34:12 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Colin Ian King <colin.king@...onical.com>,
        Markus Mayer <mmayer@...adcom.com>,
        bcm-kernel-feedback-list@...adcom.com,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        linux-arm-kernel@...ts.infradead.org
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: out of bounds access on array error_text[] because of -ETIMEDOUT
 return from __send_command()

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@...adcom.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