Re: [PATCH v1 1/8] powerpc/lib/code-patching: Enhance code patching

[Balbir Singh <bsingharora@gmail.com>]

On Sun, 2017-05-28 at 17:59 +0200, christophe leroy wrote:
> 
> Le 25/05/2017 à 05:36, Balbir Singh a écrit :
> > Today our patching happens via direct copy and
> > patch_instruction. The patching code is well
> > contained in the sense that copying bits are limited.
> > 
> > While considering implementation of CONFIG_STRICT_RWX,
> > the first requirement is to a create another mapping
> > that will allow for patching. We create the window using
> > text_poke_area, allocated via get_vm_area(), which might
> > be an overkill. We can do per-cpu stuff as well. The
> > downside of these patches that patch_instruction is
> > now synchornized using a lock. Other arches do similar
> > things, but use fixmaps. The reason for not using
> > fixmaps is to make use of any randomization in the
> > future. The code also relies on set_pte_at and pte_clear
> > to do the appropriate tlb flushing.
> 
> Isn't it overkill to remap the text in another area ?
>
> Among the 6 arches implementing CONFIG_STRICT_KERNEL_RWX (arm, arm64, 
> parisc, s390, x86/32, x86/64):
> - arm, x86/32 and x86/64 set text RW during the modification

x86 uses set_fixmap() in text_poke(), am I missing something?

> - s390 seems to uses a special instruction which bypassed write protection
> - parisc doesn't seem to implement any function which modifies kernel text.
> 
> Therefore it seems only arm64 does it via another mapping.
> Wouldn't it be lighter to just unprotect memory during the modification 
> as done on arm and x86 ?
> 

I am not sure if the trade-off is quite that simple, for security I thought

1. It would be better to randomize text_poke_area(), which is why I dynamically
allocated it. If we start randomizing get_vm_area(), we get the benefit
2. text_poke_aread() is RW and the normal text is RX, for any attack
to succeed, it would need to find text_poke_area() at the time of patching,
patch the kernel in that small window and use the normal mapping for
execution

Generally patch_instruction() is not fast path except for ftrace, tracing.
In my tests I did not find the slow down noticable

> Or another alternative could be to disable DMMU and do the write at 
> physical address ?
>

This would be worse off, I think, but we were discussing doing something
like that xmon. But for other cases, I think  it opens up a bigger window.

> Christophe

Balbir Singh