Skip to content

py/nlraarch64: Fix dangerous use of input register. - #11762

Merged
dpgeorge merged 1 commit into
micropython:masterfrom
dlech:fix-nlraarch64
Jun 14, 2023
Merged

py/nlraarch64: Fix dangerous use of input register.#11762
dpgeorge merged 1 commit into
micropython:masterfrom
dlech:fix-nlraarch64

Conversation

@dlech

@dlech dlech commented Jun 11, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Starting with 2757acf, the top variable in nlr_jump() in nlraarch64.c was assigned to register x19 by the compiler. However, the assembly code writes over that register with

ldp x19, x20, [%0,  #32]

since %0 is now x19. This causes the next line

ldp lr,  x9,  [%0,  #16]

to load the wrong values. This lead to a crash when returning from the function since lr now contains a bad address.

To fix the issue, we move the value of the top variable from an unknown register to a known register at the beginning of the asm code then only use known/hard-coded registers after that.

Fixes: #11754

@github-actions

github-actions Bot commented Jun 11, 2023

Copy link
Copy Markdown

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% PICO
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

@codecov

codecov Bot commented Jun 11, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #11762 (b02a5fa) into master (8cf9898) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #11762   +/-   ##
=======================================
  Coverage   98.40%   98.40%           
=======================================
  Files         155      155           
  Lines       20539    20539           
=======================================
  Hits        20211    20211           
  Misses        328      328

@dlech

dlech commented Jun 11, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I didn't see #11716 before doing this, but it fixes the issue as well.

@dlech

dlech commented Jun 11, 2023

Copy link
Copy Markdown
Contributor Author

Surprisingly, #11716 actually generates larger code due to saving the clobbered registers at the beginning of the nlr_jump function.

Disassembly of this PR:

(lldb) disassemble -n nlr_jump
micropython`nlr_jump:
micropython[0x100003d24] <+0>:   stp    x20, x19, [sp, #-0x20]!
micropython[0x100003d28] <+4>:   stp    x29, x30, [sp, #0x10]
micropython[0x100003d2c] <+8>:   add    x29, sp, #0x10
micropython[0x100003d30] <+12>:  mov    x19, x0
micropython[0x100003d34] <+16>:  bl     0x10007a2d8               ; mp_thread_get_state at mpthreadport.c:187:53
micropython[0x100003d38] <+20>:  ldr    x8, [x0, #0x28]
micropython[0x100003d3c] <+24>:  cbnz   x8, 0x100003d48           ; <+36> at nlraarch64.c:60:5
micropython[0x100003d40] <+28>:  mov    x0, x19
micropython[0x100003d44] <+32>:  bl     0x100079af0               ; nlr_jump_fail at main.c:788
micropython[0x100003d48] <+36>:  str    x19, [x8, #0x8]
micropython[0x100003d4c] <+40>:  mov    x19, x8
micropython[0x100003d50] <+44>:  mov    x20, x0
micropython[0x100003d54] <+48>:  mov    x0, x8
micropython[0x100003d58] <+52>:  bl     0x100003ca0               ; nlr_call_jump_callbacks at nlr.c:76
micropython[0x100003d5c] <+56>:  ldr    x8, [x19]
micropython[0x100003d60] <+60>:  str    x8, [x20, #0x28]
micropython[0x100003d64] <+64>:  mov    x0, x19
micropython[0x100003d68] <+68>:  ldr    x29, [x0, #0x70]
micropython[0x100003d6c] <+72>:  ldp    x27, x28, [x0, #0x60]
micropython[0x100003d70] <+76>:  ldp    x25, x26, [x0, #0x50]
micropython[0x100003d74] <+80>:  ldp    x23, x24, [x0, #0x40]
micropython[0x100003d78] <+84>:  ldp    x21, x22, [x0, #0x30]
micropython[0x100003d7c] <+88>:  ldp    x19, x20, [x0, #0x20]
micropython[0x100003d80] <+92>:  ldp    x30, x9, [x0, #0x10]
micropython[0x100003d84] <+96>:  mov    sp, x9
micropython[0x100003d88] <+100>: mov    x0, #0x1
micropython[0x100003d8c] <+104>: ret    
micropython[0x100003d90] <+108>: brk    #0x1

Disassembly of #11716

(lldb) disassemble -n nlr_jump
micropython`nlr_jump:
micropython[0x100003d14] <+0>:   stp    x28, x27, [sp, #-0x60]!
micropython[0x100003d18] <+4>:   stp    x26, x25, [sp, #0x10]
micropython[0x100003d1c] <+8>:   stp    x24, x23, [sp, #0x20]
micropython[0x100003d20] <+12>:  stp    x22, x21, [sp, #0x30]
micropython[0x100003d24] <+16>:  stp    x20, x19, [sp, #0x40]
micropython[0x100003d28] <+20>:  stp    x29, x30, [sp, #0x50]
micropython[0x100003d2c] <+24>:  add    x29, sp, #0x50
micropython[0x100003d30] <+28>:  mov    x19, x0
micropython[0x100003d34] <+32>:  bl     0x10007a2d8               ; mp_thread_get_state at mpthreadport.c:187:53
micropython[0x100003d38] <+36>:  ldr    x8, [x0, #0x28]
micropython[0x100003d3c] <+40>:  cbnz   x8, 0x100003d48           ; <+52> at nlraarch64.c:60:5
micropython[0x100003d40] <+44>:  mov    x0, x19
micropython[0x100003d44] <+48>:  bl     0x100079af0               ; nlr_jump_fail at main.c:788
micropython[0x100003d48] <+52>:  str    x19, [x8, #0x8]
micropython[0x100003d4c] <+56>:  mov    x19, x0
micropython[0x100003d50] <+60>:  mov    x0, x8
micropython[0x100003d54] <+64>:  mov    x20, x8
micropython[0x100003d58] <+68>:  bl     0x100003c90               ; nlr_call_jump_callbacks at nlr.c:76
micropython[0x100003d5c] <+72>:  mov    x10, x20
micropython[0x100003d60] <+76>:  ldr    x8, [x20]
micropython[0x100003d64] <+80>:  str    x8, [x19, #0x28]
micropython[0x100003d68] <+84>:  ldr    x29, [x10, #0x70]
micropython[0x100003d6c] <+88>:  ldp    x27, x28, [x10, #0x60]
micropython[0x100003d70] <+92>:  ldp    x25, x26, [x10, #0x50]
micropython[0x100003d74] <+96>:  ldp    x23, x24, [x10, #0x40]
micropython[0x100003d78] <+100>: ldp    x21, x22, [x10, #0x30]
micropython[0x100003d7c] <+104>: ldp    x19, x20, [x10, #0x20]
micropython[0x100003d80] <+108>: ldp    x30, x9, [x10, #0x10]
micropython[0x100003d84] <+112>: mov    sp, x9
micropython[0x100003d88] <+116>: mov    x0, #0x1
micropython[0x100003d8c] <+120>: ret    
micropython[0x100003d90] <+124>: brk    #0x1

@dpgeorge dpgeorge added the py-core Relates to py/ directory in source label Jun 14, 2023
@dpgeorge

dpgeorge commented Jun 14, 2023

Copy link
Copy Markdown
Member

Thank you, this fix is definitely needed. And it's how other native NLR implementations work.

@dpgeorge dpgeorge mentioned this pull request Jun 14, 2023
Closed
@dlech @dpgeorge
py/nlraarch64: Fix dangerous use of input register.
b02a5fa 
Starting with 2757acf, the `top` variable in `nlr_jump()` in
`nlraarch64.c` was assigned to register `x19` by the compiler.  However,
the assembly code writes over that register with

    ldp x19, x20, [%0,  micropython#32]

since `%0` is now `x19`. This causes the next line

    ldp lr,  x9,  [%0,  micropython#16]

to load the wrong values.

To fix the issue, we move the value of the `top` variable from an unknown
register to a known register at the beginning of the asm code then only use
known/hard-coded registers after that.

Fixes issue micropython#11754.

Signed-off-by: David Lechner <[email protected]>
@dpgeorge
dpgeorge merged commit b02a5fa into micropython:master Jun 14, 2023
@dlech
dlech deleted the fix-nlraarch64 branch June 14, 2023 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

py-core Relates to py/ directory in source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ports/unix: standard build crashes with segfaults often on Apple M1 (macOS Ventura 13.4)

2 participants

@dlech @dpgeorge