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

Re: [PATCH] x86/cet: Fix shskt manipulation error with BUGFRAME_{warn,run_fn}

  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Mon, 16 Aug 2021 11:31:51 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.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-SenderADCheck; bh=u94Ph1b3saNQ/nnS2pLXJOXiK8NtOEiZm3XUeuodqZA=; b=i/GiPBdlAN5GDrValfokV3okx6lC+HfQRoEpXygxfSbg1xb5FiyXl7Iv8adROm4LpQx2DE/HXnwR6n/jW8Wz+dKUXu+NZkUUpYOfFLHk+JJx1OYO9/cA8018ssgt8eN4xwR/d0cQFAdVyBNcUDBc35oKd99mSf8MKWXXgsFuo1bMy0CWViKc41rP89poIjsVYic2VezGl1kHeSmJ+GlPaSCnhVB/T7nfCbEnDMukCboAIrzCMobkZK9rVGYtfYleNVPfbG7lHCthu0dTU9oMQPFGUH4/ZuDwQU4xxZf7SgVRaIFI0Ox9eDR9CD8aXPaxxi85r3cUa2DzfCiNymNxvw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=eVwzTJnW4E6upXmNl79lb8qsWH/Zpv9DnaKFqdahbfapnHwsKtsUiAVVeJcASbJEzYxsG4Quj6wnvWOzHqtvvCFCSj7aYuxW/IrdesN7ndTRp42tqV08ZtZ+YmFVdQDI9NgBU50aYlj/NUcfUuMgLJ5TJDadj/8JGwdNP3YSRTLhSD9x9khCddoI5WNefxiun4wPx4NtXFCfFvrrOzebow9BbCNX7D2q7gSopbxND397T0G0SxrUW7egh8cLGap8g0RcRSbmEKJN0ReST4LxXj0aUFX2FDgIWEd0+o25hXNaGq/OZsEebBDmMleeBq6jI2IwBYNJAhyLBVSJxRENCQ==
  • Authentication-results: esa3.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Roger Pau Monné <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Mon, 16 Aug 2021 10:32:12 +0000
  • Ironport-hdrordr: A9a23:SQzKgqxaUpUYos1gFZuIKrPwG71zdoMgy1knxilNoHtuHvBw9v rAoB1/73TJYVkqNk3I9ergBEDjewK4yXcF2+ks1N6ZNWGN1VdASrsSjrcK7AeQfREWndQtsZ uIHZIOauHYPBxXitv7/Rn9M/tI+qj+zElwv5am85/mJzsaDJ1d0w==
  • Ironport-sdr: wOnH8AmcZEloZjRx8aOQw4/v4dIXWNv5KJLd/W4Oyw8xMdLp9xnZsquuRNATv27iDGFgEKC1sI mNAyZyg4PQ0g9Z4zIUKV8GxO0KWHuuQpf+Ph+oJ2d26TzXyaJ+QdYf9gOSkGeNizO0bIX/DowV 3us81Rau/Yw+VQGA1H5zfTJ74t37xfYp6SJbTixzC+bGBhjlrpSDUTqC6cYrUV7anK6pDAQWEr 54gMIwsM4fAo1ZapqUstEpIANe27AvhywqAWsQMk1Ov7e4BXsWoNY/6ApvsogY/97P/s8W9+s8 ASog3i5E3ZWeb09cOp7PBoym
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 13/08/2021 15:00, Jan Beulich wrote:
> On 12.08.2021 19:03, Andrew Cooper wrote:
>> This was a clear oversight in the original CET work.  The BUGFRAME_run_fn and
>> BUGFRAME_warn paths update regs->rip without an equivlenet adjustment to the
>> shadow stack, causes IRET to suffer #CP due to the mismatch.
>> One subtle, and therefore fragile, aspect of extable_shstk_fixup() was that 
>> it
>> required regs->rip to have its old value as a cross-check that the correct
>> word in the shadow stack was being adjusted.
>> Rework extable_shstk_fixup() into fixup_exception_return() which takes
>> ownership of the update to both the regular and shadow stacks, ensuring that
>> the regs->rip update is ordered suitably.
>> Use the new fixup_exception_return() for BUGFRAME_run_fn and BUGFRAME_warn to
>> ensure that the shadow stack is updated too.
>> Fixes: 209fb9919b50 ("x86/extable: Adjust extable handling to be shadow 
>> stack compatible")
>> Reported-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>


>> I'm not a massive fan of the large ifdef area.  The logic could be rearranged
>> to use IS_ENABLED(CONFIG_XEN_SHSTK) by indenting most of the function, but I
>> can't see any way to drop the goto's, and this is certainly the 
>> least-invasive
>> diff.
> It's not really neat, but we've got worse code elsewhere.
> I wonder whether gdb_arch_resume() and gdb_arch_write_reg() also
> need some sort of similar adjustment.


So there's nothing we can do right now, because GDB has yet to gain an
understanding of shadow stacks (or rather, the prototype so far is still
under discussion on LKML, given that CET support has yet to be included
into Linux).

Beyond that, I haven't seen gdbstub modified in a decade, nor have I
found anyone who's used it.  I'd be astounded if it actually works.

I think its fine for people using GDB to know that they need to turn off
CET first, but I don't expect anyone to have a pleasant time trying to
use gdbstub to begin with in Xen.




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