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

From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <Andrew.Cooper3@citrix.com>
Cc: Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v2 34/70] x86/emul: CFI hardening
Date: Thu, 17 Feb 2022 12:49:29 +0100	[thread overview]
Message-ID: <0696ce53-b126-edcd-7022-83d1d0688be7@suse.com> (raw)
In-Reply-To: <18ddd7f8-98fd-a1b4-e694-32bc338a33ea@citrix.com>

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