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

Re: [PATCH v3] x86/mm: Short circuit damage from "fishy" ref/typecount failure


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Tue, 19 Jan 2021 18:09:56 +0000
  • 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=pd+2HUtR3QTjYImg4dZA5qwbtkwydHLIhbIfOJvKD24=; b=VQcDwXJtLdk3IUtCtrd3B5kj09OMJPLZwwnv5r/Xkp6HnOcOeMiNFjthcRX2nihVRnsSelDHXAoGeKiNvi6wx0LHcykBw0ixRaMV4YgFznZn3vLGD3WfX9QDbXjZS0ZZATHO5pT0gahdUrlas5s9yqrzpXpBI3TOEwsKg0Zk4oIfAAX7Ok3e7vQ/9lrU9ZtGFe3oqBVJtWxnsWN6zYjms3CLbNO2LBiZGmdkpR8Incag5Ys1HdxIydnMn7KxOILqt1DBqh5b9jLDfCxqxrA0xJYhmg8yqMViZx3SPzXe4RPu8IQ1Ro+q7TidGXIeP1iS0LgOY9zT1piXmr//K5FZgg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=OPZE9hlz1+2rOMDSdiz5QZHyqmYy+Do4JMGC9HsHtwHaybXgBMIi2yJDmSAhVD7GEwIbCg0DQH1jN1+j/fe7aN4bIl5221RyLYwCSQ4maujIBUKcLpvF6sBXaYUCh5tmUpLh7UdNxXTxNpsbhgMezzD1nPG6gYhbSzwL9faJuMizLOhN1pazEACXtRiQ2qDIO7kcaTSkx8PlgDU4axBDaViT/lHK+mW6rUN1cv03oDSZ4bsjCBBJuRui4fzKMP+SkRAdEzlFO68dngVcOAlvy73JFpU0IZtOHbRbLFHicsjZJ2CgtXjeEXoprtxiclXcmvM9Yp5nZjuZdNnmlGrHSw==
  • Authentication-results: esa4.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Roger Pau Monné <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Paul Durrant <paul@xxxxxxx>, Tamas K Lengyel <tamas@xxxxxxxxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Tue, 19 Jan 2021 18:10:25 +0000
  • Ironport-sdr: mfn7+Mo/t5kQ4YxUuOZoqlP0nQZSjGY5OrwzQ6pcywkmozUuIcqhKicrD5g6GaEgPcX8LKXMX8 0VLkahkRudL5NSzB8pQXEeTZ5H2TEPTO/Ixpqwm8dRxpVj60n8/VN7ayEBJhs2W+l2K8zAEwVu ghO8zEv1AKoSrsz6uGucAVdUf2Cta165St3JiusaL69taMLfKYZR9TpQ4c2UnkluMIfimEKdza fQw2OCAtLSfCfnc/Af+A+v9GrEHqv3IvjHKekpcC5PmEQuEOBpk5ibdgUNoks/y2evZA9SrTiU taE=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 19/01/2021 16:48, Jan Beulich wrote:
> On 19.01.2021 14:02, Andrew Cooper wrote:
>> This code has been copied in 3 places, but it is problematic.
>>
>> All cases will hit a BUG() later in domain teardown, when a the missing
>> type/count reference is underflowed.
> I'm afraid I could use some help with this: Why would there
> be a missing reference, when the getting of one failed?

Look at the cleanup logic for the associated fields.

Either the plain ref fails (impossible without other fatal refcounting
errors AFAICT), or the typeref fails (a concern, but impossible AFAICT).

When the plain ref fails, put_page_alloc_ref() spots the underflow with
a BUG, while if the typeref fails, it is _put_page_type()'s BUG which
spots the underflow.

> IOW
> I'm not (yet) convinced you don't make the impact more
> severe in the (supposedly) impossible case of these paths
> getting hit, by converting a domain crash into a host one.

I have test cases.  I didn't go searching for the BUG()s by code inspection.

> It is in particular relevant that a PV guest may be able to
> cheat and "guess" an MFN to obtain references for before a
> certain hypercall (or other operation) actually completed.

And do what with it?  PV guest's can't force typerefs for
pgtable/segdesc because we prohibited cross-pg_owner scenarios many XSAs
ago.  A PV guest also can't force it to none or shared.

That only leaves PGT_writable_page, which is the ref we're interested in
taking.

>> v2:
>>  * Reword the commit message.
>>  * Switch BUG() to BUG_ON() to further reduce code volume.
> Short of us explicitly agreeing that such is fine to use, I
> think we ought to stick to the previously (long ago) agreed
> rule that BUG_ON() controlling expressions should not have
> side effects, for us not wanting to guarantee it will now
> and forever _not_ behave like ASSERT() wrt to evaluating
> the expression.

So what you want is v1 of this patch.

~Andrew



 


Rackspace

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