Skip to content

Commit

Permalink
create-diff-object/ppc64le: Don't allow sibling calls
Browse files Browse the repository at this point in the history
With the following patch:

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index e008aefc3a9d..7c70e369390d 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -2228,6 +2228,8 @@ static void xs_tcp_shutdown(struct rpc_xprt *xprt)
 	struct socket *sock = transport->sock;
 	int skst = transport->inet ? transport->inet->sk_state : TCP_CLOSE;

+	asm("nop");
+
 	if (sock == NULL)
 		return;
 	switch (skst) {

We saw the following panic on a RHEL7.6 kernel:

  Unable to handle kernel paging request for data at address 0xd00000000577f390
  Faulting instruction address: 0xd000000002e918f4
  Oops: Kernel access of bad area, sig: 11 [#1]
  SMP NR_CPUS=2048 NUMA pSeries
  Modules linked in: kpatch_3_10_0_957_1_3_1_1(OEK) nfsd nfs_acl rpcsec_gss_krb5 auth_rpcgss nfsv4 dns_resolver nfs lockd grace fscache sunrpc virtio_balloon ip_tables xfs libcrc32c virtio_net virtio_console virtio_blk virtio_pci virtio_ring virtio dm_mirror dm_region_hash dm_log dm_mod
  CPU: 9 PID: 5961 Comm: kworker/9:1H Kdump: loaded Tainted: G           OE K------------   3.10.0-957.1.3.el7.ppc64le #1
  Workqueue: xprtiod xprt_autoclose [sunrpc]
  task: c00000000300c3c0 ti: c0000003f1814000 task.ti: c0000003f1814000
  NIP: d000000002e918f4 LR: d000000002e57394 CTR: c00000000089d100
  REGS: c0000003f1817980 TRAP: 0300   Tainted: G           OE K------------    (3.10.0-957.1.3.el7.ppc64le)
  MSR: 8000000100009033 <SF,EE,ME,IR,DR,RI,LE>  CR: 240f2084  XER: 20000000
  CFAR: 000000010bb5270c DAR: d00000000577f390 DSISR: 40000000 SOFTE: 1
  GPR00: c00000000000b054 c0000003f1817c00 d00000000579add8 c000000214f0f4d0
  GPR04: c0000003fd618200 c0000003fd618200 0000000000000001 0000000000000dc2
  GPR08: 0000000000000dc3 0000000000000000 0000000000000000 d00000000577f370
  GPR12: c0000003f1814000 c000000007b85100 c00000000012fd88 c0000003f711bb40
  GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
  GPR20: 0000000000000001 c0000000013510b0 0000000000000001 fffffffffffffef7
  GPR24: 0000000000000000 0000000000000000 0000000000000000 c000000001b60600
  GPR28: c000000214f0f000 c000000214f0f4d0 c000000214f0f408 c000000214f0f448
  NIP [d000000002e918f4] __rpc_create_common.part.6+0x640/0x533c [sunrpc]
  LR [d000000002e57394] xprt_autoclose+0x74/0xe0 [sunrpc]
  Call Trace:
  [c0000003f1817c00] [c00000000000b054] livepatch_handler+0x30/0x80 (unreliable)
  [c0000003f1817c40] [c00000000012333c] process_one_work+0x1dc/0x680
  [c0000003f1817ce0] [c000000000123980] worker_thread+0x1a0/0x520
  [c0000003f1817d80] [c00000000012fe74] kthread+0xf4/0x100
  [c0000003f1817e30] [c00000000000a628] ret_from_kernel_thread+0x5c/0xb4
  Instruction dump:
  396b4570 f8410018 e98b0020 7d8903a6 4e800420 00000000 73747562 000f49c0
  c0000000 3d62fffe 396b4598 f8410018 <e98b0020> 7d8903a6 4e800420 00000000
  ---[ end trace 98e026b8fa880db7 ]---

The original version of xs_tcp_shutdown() has the following sequence:

  0xd000000003cfda44 <xs_tcp_shutdown+148>:       addi    r1,r1,64
  0xd000000003cfda48 <xs_tcp_shutdown+152>:       ld      r0,16(r1)
  0xd000000003cfda4c <xs_tcp_shutdown+156>:       ld      r29,-24(r1)
  0xd000000003cfda50 <xs_tcp_shutdown+160>:       ld      r30,-16(r1)
  0xd000000003cfda54 <xs_tcp_shutdown+164>:       ld      r31,-8(r1)
  0xd000000003cfda58 <xs_tcp_shutdown+168>:       mtlr    r0
  0xd000000003cfda5c <xs_tcp_shutdown+172>:       b       0xd000000003cfd768

That is, it restores the stack to the caller's stack frame and then does
a sibling call to the localentry point of xs_reset_transport()).  So
when xs_reset_transport() returns, it will return straight to
xs_tcp_shutdown()'s caller (xprt_autoclose).

The patched version of the function has this instead (dumped from a
vmcore):

  0xd000000003df0834 <xs_tcp_shutdown+148>:       addi    r1,r1,64
  0xd000000003df0838 <xs_tcp_shutdown+152>:       ld      r0,16(r1)
  0xd000000003df083c <xs_tcp_shutdown+156>:       ld      r29,-24(r1)
  0xd000000003df0840 <xs_tcp_shutdown+160>:       ld      r30,-16(r1)
  0xd000000003df0844 <xs_tcp_shutdown+164>:       ld      r31,-8(r1)
  0xd000000003df0848 <xs_tcp_shutdown+168>:       mtlr    r0
  0xd000000003df084c <xs_tcp_shutdown+172>:       b       0xd000000003df0ad0

After restoring the stack, instead of branching directly to
xs_reset_transport(), it (rightfully) branches to a toc stub.  A stub is
needed because the function it's branching to is in another module
(branching from the patch module to the sunrpc module).

The stub is:

  0xd000000003df0ad0 <xs_tcp_shutdown+816>:       addis   r11,r2,-1
  0xd000000003df0ad4 <xs_tcp_shutdown+820>:       addi    r11,r11,26328
  0xd000000003df0ad8 <xs_tcp_shutdown+824>:       std     r2,24(r1)
  0xd000000003df0adc <xs_tcp_shutdown+828>:       ld      r12,32(r11)
  0xd000000003df0ae0 <xs_tcp_shutdown+832>:       mtctr   r12
  0xd000000003df0ae4 <xs_tcp_shutdown+836>:       bctr

And the "std r2,24(r1)" corrupts the caller's stack.

This stub makes sense for a normal call, because the stack would be
owned by the caller of the stub, so it's ok to write r2 to it.  But
because this is a sibling call, the stack has been restored and r2 gets
incorrectly saved to the original caller's stack (i.e., xprt_autoclose's
stack).

So xprt_autoclose() -- which is in the sunrpc module -- gets the
livepatch module's toc pointer written to its stack.  It panics on when
it tries to use that vlue on its very next call.

Fix it by disallowing sibling calls from patched functions on ppc64le.

In theory we could instead a) generate a custom stub, or b) modify the
kernel livepatch_handler code to save/restore the stack r2 value, but
this is easier for now.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
  • Loading branch information
jpoimboe committed Jul 23, 2019
1 parent 33d1746 commit 8b952bd
Showing 1 changed file with 56 additions and 0 deletions.
56 changes: 56 additions & 0 deletions kpatch-build/create-diff-object.c
Original file line number Diff line number Diff line change
Expand Up @@ -3253,6 +3253,60 @@ static void kpatch_build_strings_section_data(struct kpatch_elf *kelf)
}
}

/*
* Don't allow sibling calls from patched functions on ppc64le. Before doing a
* sibling call, the patched function restores the stack to its caller's stack.
* The kernel-generated stub then writes the patch module's r2 (toc) value to
* the caller's stack, corrupting it, eventually causing a panic after it
* returns to the caller and the caller tries to use the livepatch module's toc
* value.
*
* In theory we could instead a) generate a custom stub, or b) modify the
* kernel livepatch_handler code to save/restore the stack r2 value, but this
* is easier for now.
*/
static void kpatch_no_sibling_calls_ppc64le(struct kpatch_elf *kelf)
{
#ifdef __powerpc64__
struct symbol *sym;
unsigned int insn;
unsigned long offset;

list_for_each_entry(sym, &kelf->symbols, list) {
if (sym->type != STT_FUNC || sym->status != CHANGED)
continue;

for (offset = 0; offset < sym->sec->data->d_size; offset += 4) {

insn = *(unsigned int *)(sym->sec->data->d_buf + offset);

/*
* The instruction 0x48000000 can be assumed to be a
* sibling call:
*
* Bits 0-5 (opcode) == 0x9: unconditional branch
* Bit 30 (absolute) == 0: relative address
* Bit 31 (link) == 0: doesn't set LR (not a call)
*
* Bits 6-29 (branch address) == zero, which means
* it's either a branch to self (infinite loop), or
* there's a REL24 relocation for the address which
* will be written by the linker or the kernel.
*/
if (insn != 0x48000000)
continue;

/* Make sure it's not a branch-to-self: */
if (!find_rela_by_offset(sym->sec->rela, offset))
continue;

ERROR("Found an unsupported sibling call at %s()+0x%lx. Add __attribute__((optimize(\"-fno-optimize-sibling-calls\"))) to %s() definition.",
sym->name, sym->sym.st_value + offset, sym->name);
}
}
#endif
}

struct arguments {
char *args[7];
int debug;
Expand Down Expand Up @@ -3414,6 +3468,8 @@ int main(int argc, char *argv[])
free(base_locals);
free(hint);

kpatch_no_sibling_calls_ppc64le(kelf_out);

/* create strings, patches, and dynrelas sections */
kpatch_create_strings_elements(kelf_out);
kpatch_create_patches_sections(kelf_out, lookup, parent_name);
Expand Down

0 comments on commit 8b952bd

Please sign in to comment.