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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

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.

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.

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.

~Andrew