Re: [PATCH 4/4] remoteproc: qcom: pas: Add Glymur OOBMSS coredump support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] remoteproc: qcom_pas: Use-After-Unmap race in qcom_pas_da_to_va during concurrent coredump
- [Medium] remoteproc: qcom: coredump failure for relocatable firmware
- [Medium] mm: memory leak of rproc_dump_segment objects on module unload
--

commit 7e66e9e1579da74dff881b896ffcf919220e337d
Author: Anvesh Jain P <anvesh.p@oss.qualcomm.com>

remoteproc: qcom: pas: Add Glymur OOBMSS coredump support

Add coredump support for the secure and non-secure OOBMSS carveouts.
Register fixed dump segments at probe time for attached-state recovery.

> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index 31fdde351beda..0dc9985f01f94 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c

[ ... ]

> @@ -162,6 +163,28 @@ static void qcom_pas_segment_dump(struct rproc *rproc,
>  	memcpy_fromio(dest, pas->mem_region + total_offset, size);
>  }
>  
> +static bool qcom_pas_has_secondary(const struct qcom_pas *pas)
> +{
> +	return pas->secondary_pas_id && pas->secondary_dtb_pas_id;
> +}
> +
> +static int qcom_pas_register_dump_segments(struct rproc *rproc,
> +					   struct qcom_pas *pas)
> +{
> +	int ret;
> +
> +	ret = rproc_coredump_add_segment(rproc, pas->mem_phys, pas->mem_size);

[Severity: Medium]
Will this regression cause a coredump failure for relocatable firmware?

The segments are registered with the device address (da) set to the absolute
physical address (pas->mem_phys and pas->secondary_mem_phys). However, for
relocatable firmware, pas->mem_reloc is updated to a low virtual device
address (e.g., 0) during firmware loading.

This causes the bounds check in qcom_pas_da_to_va() to fail because the
calculation da - pas->mem_reloc will overflow the carveout's size, preventing
the coredump from being generated.

> +	if (ret)
> +		return ret;

[ ... ]

> @@ -572,16 +595,29 @@ static int qcom_pas_stop(struct rproc *rproc)
>  static void *qcom_pas_da_to_va(struct rproc *rproc, u64 da, size_t len, bool *is_iomem)
>  {
>  	struct qcom_pas *pas = rproc->priv;
> -	int offset;
> +	phys_addr_t base;
> +	void __iomem *region;
> +
> +	if (da >= pas->mem_reloc && len <= pas->mem_size &&
> +	    da - pas->mem_reloc <= pas->mem_size - len) {
> +		base = pas->mem_reloc;
> +		region = pas->mem_region;
> +	} else if (da >= pas->secondary_mem_reloc &&
> +		   len <= pas->secondary_mem_size &&
> +		   da - pas->secondary_mem_reloc <= pas->secondary_mem_size - len) {
> +		base = pas->secondary_mem_reloc;
> +		region = pas->secondary_mem_region;
> +	} else {
> +		return NULL;
> +	}
>  
> -	offset = da - pas->mem_reloc;
> -	if (offset < 0 || offset + len > pas->mem_size)
> +	if (!region)
>  		return NULL;
>  
>  	if (is_iomem)
>  		*is_iomem = true;
>  
> -	return (__force void *)pas->mem_region + offset;
> +	return (__force void *)region + (da - base);
>  }

[Severity: High]
Does returning a direct pointer to transiently mapped I/O memory introduce a
use-after-unmap regression during a concurrent coredump?

When a coredump is active, region (pas->mem_region or
pas->secondary_mem_region) is transiently mapped. The debugfs trace reader
rproc_trace_read() calls qcom_pas_da_to_va() and accesses the returned
pointer locklessly via strnlen().

If the concurrent coredump finishes and executes iounmap() in
qcom_pas_coredump() while the trace reader is still accessing the string,
it will cause a system crash when userspace dereferences the unmapped I/O
memory.

[ ... ]

> @@ -1130,6 +1192,12 @@ static int qcom_pas_probe(struct platform_device *pdev)
>  	if (desc->early_boot)
>  		pas->rproc->state = RPROC_DETACHED;
>  
> +	if (qcom_pas_has_secondary(pas)) {
> +		ret = qcom_pas_register_dump_segments(rproc, pas);
> +		if (ret)
> +			goto remove_ssr_sysmon;
> +	}
> +

[Severity: Medium]
Does proactively allocating the dump segments during probe cause a memory leak
regression on module unload if the remoteproc is never started?

If the rproc remains in the RPROC_DETACHED or RPROC_OFFLINE state, module
unload calls rproc_del() -> __rproc_shutdown(). However, __rproc_shutdown()
bails out early for these states and skips the rproc_resource_cleanup() call
that would free the dump segments, causing the kzalloc() allocations to leak
permanently.

>  	ret = rproc_add(rproc);
>  	if (ret)
>  		goto remove_ssr_sysmon;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-oob-support-glymur-v1-0-96cc8ffcced3@oss.qualcomm.com?part=4