Re: Revert of the 4.17 hypercall handler changes Re: [PATCH-for-4.17] xen: fix generated code for calling hypercall handlers

[George Dunlap <George.Dunlap@xxxxxxxxxx>]

> On 4 Nov 2022, at 05:01, Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx> wrote:
> 
> On 03/11/2022 16:36, Juergen Gross wrote:
>> The code generated for the call_handlers_*() macros needs to avoid
>> undefined behavior when multiple handlers share the same priority.
>> The issue is the hypercall number being unverified fed into the macros
>> and then used to set a mask via "mask = 1ULL << <hypercall-number>".
>> 
>> Avoid a shift amount of more than 63 by setting mask to zero in case
>> the hypercall number is too large.
>> 
>> Fixes: eca1f00d0227 ("xen: generate hypercall interface related code")
>> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> 
> This is not a suitable fix.  There being a security issue is just the
> tip of the iceberg.

At the x86 Maintainer’s meeting on Monday, we (Andrew, Jan, and I) talked about 
this patch.  Here is my summary of the conversation (with the caveat that I may 
get some of the details wrong).

The proposed benefits of the series are:

1. By removing indirect calls, it removes those as a “speculative attack 
surface”.

2. By removing indirect calls, it provides some performance benefit, since 
indirect calls  require an extra memory fetch.

3. It avoids casting function pointers to function pointers of a different 
type.  Our current practice is *technically* UB, and is incompatible with some 
hardware safety mechanisms which we may want to take advantage of at some point 
in the future; the series addresses both.

There were two incidental technical problems pointed out:

1. A potential shift of more than 64 bytes, which is UB; this has been fixed.

2. The prototype for the kexec_op call was changed from unsigned long to 
unsigned int; this is an ABI change which will cause differing behavior.  Jan 
will be looking at how he can fix this, now that it’s been noted.

But the more fundamental costs include:

1. The code is much more difficult now to reason about

2. The code is much larger

3. The long if/else chain could theoretically help hypercalls at the top if the 
chain, but would definitely begin to hurt hypercalls at the bottom of the 
chain; and the more hypercalls we add, the more of a theoretical performance 
penalty this will have

4. By using 64-bit masks, the implementation limits the number of hypercalls to 
64; a number we are likely to exceed if we implement ABIv2 to be compatible 
with AMD SEV.

Additionally, there is a question about whether some of the alleged benefits 
actually help:

1. On AMD processors, we enable IBRS, which completely removes indirect calls 
as a speculative attack surface already.  And on Intel processors, this attack 
surface has already been significantly reduced.  So removing indirect calls is 
not as important an issue.

2. Normal branches are *also* a surface of speculative attacks; so even apart 
from the above, all this series does is change one potential attack surface for 
another one.

3. When we analyze theoretical performance with deep CPU pipelines and 
speculation in mind, the theoretical disadvantage of indirect branches goes 
away; and depending on the hardware, there is a theoretical performance 
degradation.

4. From a practical perspective, the performance tests are very much 
insufficient to show either that this is an improvement, or that does not cause 
a performance regression.  To show that there hasn’t been a performance 
degradation, a battery of tests needs to be done on hardware from a variety of 
different vendors and cpu generations, since each of them will have different 
properties after all speculative mitigations have been applied.

So the argument is as follows:

There is no speculative benefit for the series; there is insufficient 
performance evidence, either to justify a performance benefit or to allay 
doubts about a performance regression; and the benefit that there is 
insufficient to counterbalance the costs, and so the series should be reverted.

At the end of the discussion, Jan and I agreed that Andrew had made a good case 
for the series to be removed at some point.  The discussion needs to be 
concluded on the list, naturally; and if there is a consensus to remove the 
series, the next question would be whether we should revert it now, before 
4.17.0, or wait until after the release and revert it then (perhaps with a 
backport to 4.17.1).

(Jan and Andy, please let me know if I’ve misunderstood anything from that 
meeting.)

I have more details regarding the technical aspects above, but this email is 
already rather long.  Let me know if you need more details and I’ll try to fill 
them in.

Thoughts?
 -George

Attachment: signature.asc
Description: Message signed with OpenPGP