Re: [PATCH 1/4] dt-overlay: Fix NULL pointer dereference

[Julien Grall <julien@xxxxxxx>]

Hi Michal,

On 19/09/2024 12:42, Michal Orzel wrote:
> Attempt to attach an overlay (xl dt-overlay attach) to a domain without
> first adding this overlay to Xen (xl dt-overlay add) results in an
> overlay track entry being NULL in handle_attach_overlay_nodes(). This
> leads to NULL pointer dereference and the following data abort crash:
>
> (XEN) Cannot find any matching tracker with input dtbo. Operation is supported 
> only for prior added dtbo.
> (XEN) Data Abort Trap. Syndrome=0x5
> (XEN) Walking Hypervisor VA 0x40 on CPU0 via TTBR 0x0000000046948000
> (XEN) 0TH[0x000] = 0x46940f7f
> (XEN) 1ST[0x000] = 0x0
> (XEN) CPU0: Unexpected Trap: Data Abort
> (XEN) ----[ Xen-4.20-unstable  arm64  debug=y  Not tainted ]----
> ...
> (XEN) Xen call trace:
> (XEN)    [<00000a0000208b30>] dt_overlay_domctl+0x304/0x370 (PC)
> (XEN)    [<00000a0000208b30>] dt_overlay_domctl+0x304/0x370 (LR)
> (XEN)    [<00000a0000274b7c>] arch_do_domctl+0x48/0x328
>
> Fixes: 4c733873b5c2 ("xen/arm: Add XEN_DOMCTL_dt_overlay and device attachment to 
> domains")
> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
> ---
>   xen/common/dt-overlay.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/xen/common/dt-overlay.c b/xen/common/dt-overlay.c
> index d53b4706cd2f..8606b14d1e8e 100644
> --- a/xen/common/dt-overlay.c
> +++ b/xen/common/dt-overlay.c
> @@ -908,8 +908,11 @@ static long handle_attach_overlay_nodes(struct domain *d,
>    out:
>       spin_unlock(&overlay_lock);
>   
> -    rangeset_destroy(entry->irq_ranges);
> -    rangeset_destroy(entry->iomem_ranges);
> +    if ( entry )
> +    {
> +        rangeset_destroy(entry->irq_ranges);
> +        rangeset_destroy(entry->iomem_ranges);
> +    }
While looking at the error paths in handle_attach_overlay_nodes(), I 
noticed we don't revert any partial changes made by handle_device().
In this case, I am wondering whether it is correct to destroy the 
rangeset. How would you be able to revert the changes?
Cheers,

--
Julien Grall