> On 4 Nov 2022, at 05:01, Andrew Cooper <Andrew.Cooper3@citrix.com> 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@suse.com>
> 
> 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