Re: [PATCH v2] livepatch: do not use .livepatch.funcs section to store internal state

[Ross Lagerwall <ross.lagerwall@xxxxxxxxx>]

On Thu, Nov 23, 2023 at 9:52 AM Roger Pau Monne <roger.pau@xxxxxxxxxx> wrote:
>
> Currently the livepatch logic inside of Xen will use fields of struct
> livepatch_func in order to cache internal state of patched functions.  Note
> this is a field that is part of the payload, and is loaded as an ELF section
> (.livepatch.funcs), taking into account the SHF_* flags in the section
> header.
>
> The flags for the .livepatch.funcs section, as set by livepatch-build-tools,
> are SHF_ALLOC, which leads to its contents (the array of livepatch_func
> structures) being placed in read-only memory:
>
> Section Headers:
>   [Nr] Name              Type             Address           Offset
>        Size              EntSize          Flags  Link  Info  Align
> [...]
>   [ 4] .livepatch.funcs  PROGBITS         0000000000000000  00000080
>        0000000000000068  0000000000000000   A       0     0     8
>
> This previously went unnoticed, as all writes to the fields of livepatch_func
> happen in the critical region that had WP disabled in CR0.  After 8676092a0f16
> however WP is no longer toggled in CR0 for patch application, and only the
> hypervisor .text mappings are made write-accessible.  That leads to the
> following page fault when attempting to apply a livepatch:
>
> ----[ Xen-4.19-unstable  x86_64  debug=y  Tainted:   C    ]----
> CPU:    4
> RIP:    e008:[<ffff82d040221e81>] common/livepatch.c#apply_payload+0x45/0x1e1
> [...]
> Xen call trace:
>    [<ffff82d040221e81>] R common/livepatch.c#apply_payload+0x45/0x1e1
>    [<ffff82d0402235b2>] F check_for_livepatch_work+0x385/0xaa5
>    [<ffff82d04032508f>] F arch/x86/domain.c#idle_loop+0x92/0xee
>
> Pagetable walk from ffff82d040625079:
>  L4[0x105] = 000000008c6c9063 ffffffffffffffff
>  L3[0x141] = 000000008c6c6063 ffffffffffffffff
>  L2[0x003] = 000000086a1e7063 ffffffffffffffff
>  L1[0x025] = 800000086ca5d121 ffffffffffffffff
>
> ****************************************
> Panic on CPU 4:
> FATAL PAGE FAULT
> [error_code=0003]
> Faulting linear address: ffff82d040625079
> ****************************************
>
> Fix this by moving the internal Xen function patching state out of
> livepatch_func into an area not allocated as part of the ELF payload.  While
> there also constify the array of livepatch_func structures in order to prevent
> further surprises.
>
> Note there's still one field (old_addr) that gets set during livepatch load.  
> I
> consider this fine since the field is read-only after load, and at the point
> the field gets set the underlying mapping hasn't been made read-only yet.
>
> Fixes: 8676092a0f16 ('x86/livepatch: Fix livepatch application when CET is 
> active')
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Reviewed-by: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>

Thanks,
Ross