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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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