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

[Julien Grall <julien@xxxxxxx>]

On 23/09/2024 12:05, Michal Orzel wrote:
> Hi Julien,
Hi Michal,

> 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.
Do you mean in this series or forever? If the former, would you be able 
to outline what you expect after the end of this series? What should 
work? What should not work?
> 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.
I need some clarifications. By "we do not yet support detachment", do 
you mean while a guest is running or do you also include a domain 
shutting down?
Cheers,

--
Julien Grall