Re: [PATCH 1/2] memory: brcmstb_dpfe: Fix out-of-bounds access due to DCPU offset

[Florian Fainelli <florian.fainelli@broadcom.com>]

On 8/26/26 23:48, Krzysztof Kozlowski wrote:
> On 26/08/2026 22:14, Danesh Petigara wrote:
>> From: Justin Chen <justin.chen@broadcom.com>
>>
>> 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@vger.kernel.org
>> Signed-off-by: Justin Chen <justin.chen@broadcom.com>
>> Assisted-by: Gemini:gemini-3.1-pro-preview cursor
>> Signed-off-by: Danesh Petigara <danesh.petigara@broadcom.com>
>> ---
>>   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