On 6/17/24 7:57 PM, Segher Boessenkool wrote: > On Mon, Jun 17, 2024 at 06:49:18PM -0500, Peter Bergner wrote: >> On 6/17/24 6:11 PM, Segher Boessenkool wrote: >> Yeah, I didn't write that, I only moved it, but I can try to come up with >> an explanation of why we need to disable it now. That said, my hope is to >> not have to disable shrink-wrapping even when we emit the ROP protect hash >> insns in the future, but that will take some extra work. If I can manage >> that, then this should all just go away. :-) Until then, we can stick >> with this patch's micro-optimization. > > If you inline one function into another, there is no ROP protection on > their boundary anymore (since there is no such boundary anymore!) This > is not necessarily a problem, but you do want some noipa or similar > markup where without ROP protection you have no incentive to do that. > > Shrink-wrapping allows more inlining, and more inlining allows more > shrink-wrapping, but there is no direct relation between shrink-wrapping > and our ROP protect stuff? We just need to make sure the hashst and > hashchk things are done at the very start and the very end of the > functions, but we need to make sure of that anyway! > > So yeah, please investigate a bit more :-)
So we should be able to shrink-wrap in the presence of the ROP protection. The ROP attacks work by buffer overrun type issues, clobbering the return address that was saved on the stack causing us to return to somewhere else. If we don't need to save the return address on the stack like for leaf functions, or shrink-wrapped sections that are call free, those codes are not really susceptible to ROP attacks. It's the call paths where we save the return address on the stack that we have to protect. If inlining or shrink wrapping increases the amount of code that is call free (ie, we don't need to save the return address), then that code is not less safe than before but as safe or safer than before. It seems the reason we disabled shrink-wrapping now, was that we were emitting the hashst in the wrong location (PR101324) causing us to store a bad hash value. I think that was just a "bug" that probably should have been fixed rather than worked around by disabling shrink-wrapping. It's on my TODO to take a look at fixing that correctly. >> At the moment, yes, since the rop_ok test not only checks for the -mcpu= >> level, >> it also verifies that the ABI is ok. > > Ah right! Add a short comment? Can do. >> Currently, rop_ok makes sure we have >> Power10 and ELFv2 ABI being used. So currently, if we were to run this test >> on BE, we'd get an UNSUPPORTED using the rop_ok check, but if we removed it, >> we'd see a FAIL. > > Yup. Peter
