[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2] livepatch: account for patch offset when applying NOP patch


  • To: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Wed, 30 Mar 2022 16:55:52 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=jHxDX7F96LVMBN5pAoHzZ+IXXj+P9yooSfJBO7M8C+0=; b=TsNiC6sFJdzN4RVkY2SqyQ0llJ/rojf24CQk+cUF0JzFXhFB0twNqlIGFlh9jLjCM9hBBCQSxqO1opVJH4pgwCuXoy1nhWLexEqJMweRPv7k8xZLmvksWN0+SsckAFISRSB5u/tFPo5C/bwcbjLUt9Hokl22VI6X7OEnzULpRCTA9sMXz3yF4EetDANk2v+fxZ3FF0tsLA+itIcgL5CP4CbK72LKXlnEJRGSd4a/V0ib10Rh2ers5xDUStmAiaDfX30vlA3x3/wPq5MhZRhNyQYIcMyJu++XpvBmIc0J/U2pRlRjv8yuqcakZVgC5NHQ3BHmCdwUHZ2YuREpP1czag==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=m0fwyMj0IBMpRrg/zmvFmHRoNlnDhf94p1qutYlKmAQSCRQbkj1xxqQuLsUm0eT9JRxBC2ranQKwoNNPJn9ny9UpZydwJFsq9JrA9oxJ01l+UaHYzlyXOe2YLNmKQTaOpm9Ebo9lcsj303sdTpQjuyu1c6oP+E9T1Sd76Qx8BCGRMLsXZuN1v8ZWg7o0VwjxlDp5S3yMvCICAiMosCLp+iMNCWtsUq/X/0LZUsRzG64bdXV4W2OeJK1iqE53Txw3/DaWjZPR94FbO7ApZ1fmhS5T2agsb7M0PcOxZ3UPcIrN7vPqky5gJ3gWSak6NhmU+6p4wXfa2t6nqYo4mPwPVw==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>, Konrad Wilk <konrad.wilk@xxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Bjoern Doebel <doebel@xxxxxxxxx>
  • Delivery-date: Wed, 30 Mar 2022 14:56:10 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 30.03.2022 16:19, Roger Pau Monné wrote:
> On Wed, Mar 30, 2022 at 01:05:31PM +0200, Jan Beulich wrote:
>> While not triggered by the trivial xen_nop in-tree patch on
>> staging/master, that patch exposes a problem on the stable trees, where
>> all functions have ENDBR inserted. When NOP-ing out a range, we need to
>> account for this. Handle this right in livepatch_insn_len().
>>
>> This requires livepatch_insn_len() to be called _after_ ->patch_offset
>> was set. Note however that the earlier call cannot be deleted. In fact
>> its result should have been used to guard the is_endbr64() /
>> is_endbr64_poison() invocations - add the missing check now. While
>> making that adjustment, also use the local variable "old_ptr"
>> consistently.
>>
>> Fixes: 6974c75180f1 ("xen/x86: Livepatch: support patching CET-enhanced 
>> functions")
> 
> I have to admit I'm confused as to why that commit carries a Tested-by
> from Arm.  Did Arm test the commit on x86 hardware?  Because that
> commit only touches x86 specific code.

;-)

>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> FWIW, on the original implementation, I think it would have been
> clearer to advance old_ptr and adjust the length?

In my 1st attempt I had confined the change to the x86 file, but it
didn't feel right that I then also had to adjust arch_livepatch_revert().

>> ---
>> v2: Re-issue livepatch_insn_len(). Fix buffer overrun.
>> ---
>> Only build tested, as I don't have a live patching environment available.
>>
>> For Arm this assumes that the patch_offset field starts out as zero; I
>> think we can make such an assumption, yet otoh on x86 explicit
>> initialization was added by the cited commit.

Note how this already deals with ...

>> --- a/xen/include/xen/livepatch.h
>> +++ b/xen/include/xen/livepatch.h
>> @@ -90,7 +90,7 @@ static inline
>>  unsigned int livepatch_insn_len(const struct livepatch_func *func)
>>  {
>>      if ( !func->new_addr )
>> -        return func->new_size;
>> +        return func->new_size - func->patch_offset;
> 
> Seeing as func->patch_offset is explicitly initialized in
> arch_livepatch_apply for x86, do we also need to do the same on Arm
> now that the field will be used by common code?
> 
> Maybe the initialization done in arch_livepatch_apply for x86 is not
> strictly required.

... your remark. I'd prefer if I could get away without touching Arm
code. Hence if such initialization was needed, I think it ought to
live in common code. If this was a requirement here, I would perhaps
add a prereq patch doing the movement. My preference though would be
for that to be a follow-on, unless there's an actual reason why the
initialization has to happen; personally I think it ought to be a
requirement on patch building that this (and perhaps other) fields
start out as zero. I therefore view the initialization on x86 as a
guard against the patch getting applied a 2nd time. Yet of course it
would then also have helped (not anymore after this change) to use
= instead of += when updating ->patch_offset.

Jan




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.