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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 20/09/2024 10:29, Julien Grall wrote:
> 
> 
> 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?
I guess the same story applies as for the partial add/remove which was stated 
by Vikram
in the commit msg of 7e5c4a8b86f12942de0566b1d61f71d15774364b meaning partial 
success withe some
failures may lead to other failures and might need a system reboot. I did not 
carefully look into
this series, my plan was to fix the issues without changing the functional 
behavior. FWICS, we do not
yet support detachment (only add/remove and attach) and removal of nodes and 
ranges is only
possible if the nodes are assigned to hwdom.

~Michal