Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
dpgeorge merged 1 commit into from
Jun 14, 2023
Merged

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

dpgeorge merged 1 commit into from
Jun 14, 2023

Conversation

@dlech
Copy link
Contributor

@dlech dlech commented Jun 11, 2023 •
edited
Loading

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
Copy link

github-actions bot commented Jun 11, 2023

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
Copy link

codecov bot commented Jun 11, 2023 •
edited
Loading

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
Copy link
Contributor Author

dlech commented Jun 11, 2023 •
edited
Loading

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

@dlech
Copy link
Contributor Author

dlech commented Jun 11, 2023

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
Copy link
Member

dpgeorge commented Jun 14, 2023

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 <david@pybricks.com>
@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