Re: [PATCH v2 34/70] x86/emul: CFI hardening

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.02.2022 22:34, Andrew Cooper wrote:
> On 15/02/2022 14:13, Jan Beulich wrote:
>> On 15.02.2022 14:43, Andrew Cooper wrote:
>>> On 14/02/2022 13:38, Jan Beulich wrote:
>>>> On 14.02.2022 13:50, Andrew Cooper wrote:
>>>>> Control Flow Integrity schemes use toolchain and optionally hardware 
>>>>> support
>>>>> to help protect against call/jump/return oriented programming attacks.
>>>>>
>>>>> Use cf_check to annotate function pointer targets for the toolchain.
>>>>>
>>>>> pv_emul_is_mem_write() is only used in a single file.  Having it as a 
>>>>> static
>>>>> inline is pointless because it can't be inlined to begin with.
>>>> I'd like you to consider to re-word this:
>>> This is the reworded version.
>>>
>>>> It being static inline was for
>>>> the case of there appearing a 2nd user. I don't view such as pointless.
>>> I find that impossible to reconcile with your normal review feedback.
>> Interesting. I don't think I would have objected to something like
>> this, if it was conceivable that a 2nd user may appear. I don't
>> think this is the only inline function we've got with just a single
>> user. I also don't think this is the only inline function we've got
>> with its address taken, and hence having an out-of-line instantiation.
>>
>>> It is unconditionally forced out of line because of how it's used,
>>> meaning that if it ever got used in a second translation unit we'd end
>>> up with a duplicate function, at which point it would need to be
>>> non-static and exported to pass review.  (And sanity.)
>> I'm afraid you've lost me here. What duplicate function? Before and
>> after the patch the function is static; what changes is merely the
>> "inline". Two CUs can have identically named static functions, can't
>> they? Or if that's not the point you try to make, then I have no idea
>> what it is that you're trying to tell me.
> 
> Yes, the same static inline can be out-of-lined in multiple translation
> units.  This creates two identical copies of the logic, and then falls
> foul of our unique symbols constraint.

How / where / when? If you care about unique symbol names, you've got
a Kconfig setting to enable.

> The absence of complaints in the general case shows that we don't
> currently any cases where a static inline is out-of-lined in multiple
> translation units.

Nope. I see reports of duplicate symbols (in their warning incarnation)
quite frequently. hvm.c#cpu_callback and hvm.c#cpu_nfb, to just name
two. Those aren't inline functions, but the naming constraints apply
equally.

> Under IBT, it means more tagged functions, which I suppose doesn't make
> a useful difference from the attackers point of view, but it's still
> logic duplication in the final build that we'd prefer to avoid.

Right, which is why I didn't object in any way (and you did have my ack
for the patch already anyway), but merely asked that you soften
"pointless" in the description. I really don't like it if, for reasons
I cannot follow, things are criticized more severely than (imo)
warranted. I'd like to point out that effectively you're reverting
08143c5b6c1f ("x86: move pv_emul_is_mem_write to pv/emulate.h") then
(just that the function moves to a different file now, following the
movement of its users), which you did give your R-b. (And yes, I know
views can change over time.)

Jan