Re: [Xen-devel] [PATCH v4 10/11] livepatch/arm[32, 64]: Modify .livepatch.funcs section to be RW when patching

[Julien Grall <julien.grall@xxxxxxx>]

Hi Jan,

On 21/09/17 13:16, Jan Beulich wrote:
> > > > On 21.09.17 at 00:31, <konrad@xxxxxxxxxx> wrote:
> > @@ -43,7 +46,29 @@ int arch_livepatch_quiesce(void)
> >           return -ENOMEM;
> >       }
> >   
> > -    return 0;
> > +    if ( nfuncs )
> > +    {
> > +        unsigned long va = (unsigned long)func;
> > +        unsigned int offs = va & (PAGE_SIZE - 1);
> > +        unsigned int pages = PFN_UP(offs + nfuncs * sizeof(*func));
> > +
> > +        va &= PAGE_MASK;
> > +
> > +        rc = modify_xen_mappings(va, va + (pages * PAGE_SIZE), PTE_NX);
> > +        if ( rc )
> > +        {
> > +            printk(XENLOG_ERR LIVEPATCH "Failed to modify 0x%lx to RW\n", va);
> %#lx ?
>
> > +            vunmap(vmap_of_xen_text);
> > +            vmap_of_xen_text = NULL;
> > +        }
> > +        else
> > +        {
> > +            livepatch_stash.va = va;
> > +            livepatch_stash.pages = pages;
> > +        }
> > +    }
> You're effectively doing all this behind the back of vmalloc() / vmap();
> I'm not sure this is a good idea, but I'm also not a maintainer of this
> code.
We already have place in the code (both x86 and Arm) modifying memory 
attributes on the back of vmalloc/vmap. See arch_livepatch_secure for 
instance.
I suggested this solution because it avoids to create a temporary 
mapping for the .livepatch.funcs section.
Do you foresee potentially issue of temporarily modifying permissions of 
a mapping?
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel