Messages in this thread | | | Date | Thu, 6 Sep 2018 22:27:46 +0200 | From | Peter Zijlstra <> | Subject | Re: [PATCH] x86: use WRITE_ONCE() when setting PTEs |
| |
On Thu, Sep 06, 2018 at 01:12:14PM -0700, Nadav Amit wrote: > at 12:57 PM, Peter Zijlstra <peterz@infradead.org> wrote: > > > On Sun, Sep 02, 2018 at 11:14:50AM -0700, Nadav Amit wrote: > >> When page-table entries are set, the compiler might optimize their > >> assignment by using multiple instructions to set the PTE. This might > >> turn into a security hazard if the user somehow manages to use the > >> interim PTE. L1TF does not make our lives easier, making even an interim > >> non-present PTE a security hazard. > >> > >> Using WRITE_ONCE() to set PTEs and friends should prevent this potential > >> security hazard. > >> > >> I skimmed the differences in the binary with and without this patch. The > >> differences are (obviously) greater when CONFIG_PARAVIRT=n as more > >> code optimizations are possible. For better and worse, the impact on the > >> binary with this patch is pretty small. Skimming the code did not cause > >> anything to jump out as a security hazard, but it seems that at least > >> move_soft_dirty_pte() caused set_pte_at() to use multiple writes. > > > > Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org> > > > > Also, its corollary would also make sense/be required, use READ_ONCE() > > when reading these. > > I don’t know. This would obviously be much more intrusive. I can add a > get_pte() and write a Coccinelle script to use it instead of reading the > PTE, but in most cases, I presume, it would be an overkill. > > The reason for that is that the PTEs are supposed to be accessed while > holding the page-table lock, and the hardware can only change dirty & access > bits. I think that any code that assumes that these bits do not change while > holding the lock is already broken in more ways.
There are lockless readers, but I just checked, mm/gup.c already uses READ_ONCE(), so that should be fine.
| |