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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 27/08/2026 19:37, Florian Fainelli wrote:
> 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.

Ack

> 
>>
>> 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?

That's in this case separate solution, worth anyway investigating.

> 
>>
>>>
>>> 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()?

If the bindings enforces strict order, that could be a way, but I do not
want to push towards new
devm_platform_get_and_ioremap_resource_by_name() because that could lead
to long patchset with bikeshedding about how many wrappers we need...

Approach is fine then. New wrapper can be brought on top.

Best regards,
Krzysztof