[PATCH] D102842: [Verifier] Fail on invalid indices for {insert,extract} vector intrinsics

Fri May 21 08:10:59 PDT 2021

joechrisellis added a subscriber: paulwalker-arm.
joechrisellis added inline comments.

================
Comment at: llvm/test/CodeGen/AArch64/sve-insert-vector.ll:112
 ; CHECK-NEXT:    st1h { z0.h }, p0, [sp]
-; CHECK-NEXT:    stur q1, [sp, #2]
+; CHECK-NEXT:    str q1, [x9, x8]
 ; CHECK-NEXT:    ld1h { z0.h }, p0/z, [sp]
----------------
david-arm wrote:
> This is just FYI and not caused by your patch, but suppose vscale = 1 and we clamp the index of 8 to 7. The `str q1, [x9, x8]` instruction actually ends up storing out 128 bits starting from index 7, which is beyond the end of temporary stack space. It looks like we could be corrupting the stack!
Hi Dave, thanks for pointing this out -- this is a great spot. Had a chat with @paulwalker-arm about this and will be looking into what the possibilities are here. A key takeaway from our conversation was that if vscale = 1 for this particular insertion then we're not strictly performing an 'insert subvector', so in some sense we could consider this abuse of the intrinsic. Most of our use-cases for the intrinsic are with an idx of 0 as well, so I don't think this will cause problems if the compilation pipeline is run end-to-end on a C program or something.

Of course, we can't really defend against something like this statically, so the possibilities are limited. We discussed:

- more stringent checks at runtime -- but this could easily result in poor code quality. Perhaps there is a different/better way of checking the bounds/clamping.
- allocating more stack space than strictly necessary to give us some breathing room (? -- might have misunderstood this).

(@paulwalker-arm, if I have misunderstood anything let me know 😄)

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102842/new/

https://reviews.llvm.org/D102842