Re: [PATCH 1/2] memory: brcmstb_dpfe: Fix out-of-bounds access due to DCPU offset
From: Florian Fainelli
Date: Thu Aug 27 2026 - 13:37:44 EST
On 8/26/26 23:48, Krzysztof Kozlowski wrote:
On 26/08/2026 22:14, Danesh Petigara wrote:
From: Justin Chen <justin.chen@xxxxxxxxxxxx>
On API v1/v2 boards, the DCPU coprocessor can steer kernel readl_relaxed()
and writel_relaxed() to any address within 256 MB of the ioremapped DPFE
dmem or regs base. The DCPU firmware provides a 28-bit offset which the
driver adds to the ioremap base without any bounds checking in
get_msg_ptr().
This allows a compromised DCPU firmware to trick the host kernel into
reading or writing arbitrary memory-mapped I/O registers in vmalloc
space. When combined with a root-writable sysfs file like dpfe_refresh,
it provides an arbitrary MMIO write primitive. Similarly, world-readable
sysfs files can be used to leak other devices' register contents.
If someone can compromise firmware to provide different addresses, then
what stops this person to change DTB with completely different MMIO
ranges for this device?
On our platforms the DTB is part of the boot loader which is signed+encrypted and customers do not build any UART driver or user-interface so changing the DTB is somewhat difficult. Same goes with the Linux kernel, it's also signed+encrypted. This is defense in depth at this point.
Isn't better just to drop root-writeable sysfs interfaces, since they
are the insecure parts?
Yes that would probably be an acceptable move forward. Markus do you remember what was the use case for the dpfe_refresh file to be R/W?
Fix this by recording the resource_size() of the dmem and regs ioremaps
at probe time, and rejecting any offset that, along with the largest
field accessed (DRAM_VENDOR_ERROR + sizeof(u32)), exceeds the recorded
mapping size.
Fixes: fee5f1ef6cf7 ("memory: brcmstb: dpfe: support new way of passing data from the DCPU")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Justin Chen <justin.chen@xxxxxxxxxxxx>
Assisted-by: Gemini:gemini-3.1-pro-preview cursor
Signed-off-by: Danesh Petigara <danesh.petigara@xxxxxxxxxxxx>
---
drivers/memory/brcmstb_dpfe.c | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/drivers/memory/brcmstb_dpfe.c b/drivers/memory/brcmstb_dpfe.c
index 08d9e05b1b33..66343205f585 100644
--- a/drivers/memory/brcmstb_dpfe.c
+++ b/drivers/memory/brcmstb_dpfe.c
@@ -182,6 +182,8 @@ struct brcmstb_dpfe_priv {
void __iomem *regs;
void __iomem *dmem;
void __iomem *imem;
+ resource_size_t regs_size;
+ resource_size_t dmem_size;
struct device *dev;
const struct dpfe_api *dpfe_api;
struct mutex lock;
@@ -401,9 +403,14 @@ static void __iomem *get_msg_ptr(struct brcmstb_dpfe_priv *priv, u32 response,
*/
switch (msg_type) {
case 1:
+ if (DCPU_MSG_RAM_START + offset + DRAM_VENDOR_ERROR +
+ sizeof(u32) > priv->regs_size)
+ goto bad_offset;
ptr = priv->regs + DCPU_MSG_RAM_START + offset;
break;
case 0:
+ if (offset + DRAM_VENDOR_ERROR + sizeof(u32) > priv->dmem_size)
+ goto bad_offset;
ptr = priv->dmem + offset;
break;
default:
@@ -415,6 +422,12 @@ static void __iomem *get_msg_ptr(struct brcmstb_dpfe_priv *priv, u32 response,
}
return ptr;
+
+bad_offset:
+ dev_err(priv->dev, "DCPU returned out-of-range offset %#x\n", offset);
+ if (buf && size)
+ *size = sprintf(buf, "ERROR: DCPU offset out of range\n");
+ return NULL;
}
static void __finalize_command(struct brcmstb_dpfe_priv *priv)
@@ -858,6 +871,7 @@ static int brcmstb_dpfe_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct brcmstb_dpfe_priv *priv;
+ struct resource *res;
int ret;
priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
@@ -869,17 +883,21 @@ static int brcmstb_dpfe_probe(struct platform_device *pdev)
mutex_init(&priv->lock);
platform_set_drvdata(pdev, priv);
- priv->regs = devm_platform_ioremap_resource_byname(pdev, "dpfe-cpu");
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dpfe-cpu");
You cannot use devm_platform_get_and_ioremap_resource()? Are the 'reg'
entries flexible/random?
IIRC the names and indexes are stable so we could do a positional index resource lookup, but maybe what we want is to introduce a devm_platform_get_and_ioremap_resource_by_name()?
--
Florian