linux-kernel - Re: [PATCH v1 1/1] fs/splice: add missing callback for inaccessible pages

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <172c51f7-7dd6-7dd0-153f-aedd4b10a9f3@intel.com>
Date:   Wed, 29 Apr 2020 16:52:46 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     Claudio Imbrenda <imbrenda@...ux.ibm.com>
Cc:     Christian Borntraeger <borntraeger@...ibm.com>,
        akpm@...ux-foundation.org, jack@...e.cz, kirill@...temov.name,
        david@...hat.com, aarcange@...hat.com, linux-mm@...ck.org,
        frankja@...ux.ibm.com, sfr@...b.auug.org.au, jhubbard@...dia.com,
        linux-kernel@...r.kernel.org, linux-s390@...r.kernel.org,
        peterz@...radead.org, sean.j.christopherson@...el.com,
        Ulrich.Weigand@...ibm.com
Subject: Re: [PATCH v1 1/1] fs/splice: add missing callback for inaccessible
 pages

On 4/29/20 3:53 PM, Claudio Imbrenda wrote:
>> Actually, that's the problem.  You've gone through all these careful
>> checks and made the page inaccessible.  *After* that process, how do
>> you keep the page from being hit by an I/O device before it's made
>> accessible again?  My patch just assumes that *all* pages have gone
>> through that process and passed those checks.
> I don't understand what you are saying here.
> 
> we start with all pages accessible, we mark pages as inaccessible when
> they are imported in the secure guest (we use the PG_arch_1 bit in
> struct page). we then try to catch all I/O on inaccessible pages and
> make them accessible so that I/O devices are happy. 

The catching mechanism is incomplete, that's all I'm saying.

Without looking too hard, and not even having the hardware, I've found
two paths where the "catching" was incomplete:

	1. sendfile(), which you've patched
	2. sendto(), which you haven't patched

> either your quick and dirty patch was too dirty (e.g. not accounting
> for possible races between make_accessible/make_inaccessible), or some
> of the functions in the trace you provided should do pin_user_page
> instead of get_user_page (or both)

I looked in the traces for any races.  For sendto(), at least, the
make_accessible() never happened before the process exited.  That's
entirely consistent with the theory that it entirely missed being
caught.  I can't find any evidence that there were races.

Go ahead and try it.  You have the patch!  I mean, I found a bug in
about 10 minutes in one tiny little VM.

And, yes, you need to get rid of the FOLL_PIN check unless you want to
go change a big swath of the remaining get_user_pages*() sites.