Re: [RFC][Patch v8 4/7] KVM: Disabling page poisoning to prevent corruption

From: Alexander Duyck <alexander.duyck@gmail.com>
To: Nitesh Narayan Lal <nitesh@redhat.com>
Cc: kvm list <kvm@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Paolo Bonzini <pbonzini@redhat.com>,
	lcapitulino@redhat.com, pagupta@redhat.com, wei.w.wang@intel.com,
	Yang Zhang <yang.zhang.wz@gmail.com>,
	riel@surriel.com, david@redhat.com,
	"Michael S. Tsirkin" <mst@redhat.com>,
	dodgen@google.com, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	dhildenb@redhat.com, Andrea Arcangeli <aarcange@redhat.com>
Subject: Re: [RFC][Patch v8 4/7] KVM: Disabling page poisoning to prevent corruption
Date: Thu, 7 Feb 2019 09:23:35 -0800	[thread overview]
Message-ID: <CAKgT0Ud0-A8MmbSFQTd81X5-ezwtsi67qPLa0uC_MUs4r+bxqw@mail.gmail.com> (raw)
In-Reply-To: <20190204201854.2328-5-nitesh@redhat.com>

On Mon, Feb 4, 2019 at 2:11 PM Nitesh Narayan Lal <nitesh@redhat.com> wrote:
>
> This patch disables page poisoning if guest page hinting is enabled.
> It is required to avoid possible guest memory corruption errors.
> Page Poisoning is a feature in which the page is filled with a specific
> pattern of (0x00 or 0xaa) after arch_free_page and the same is verified
> before arch_alloc_page to prevent following issues:
>     *information leak from the freed data
>     *use after free bugs
>     *memory corruption
> Selection of the pattern depends on the CONFIG_PAGE_POISONING_ZERO
> Once the guest pages which are supposed to be freed are sent to the
> hypervisor it frees them. After freeing the pages in the global list
> following things may happen:
>     *Hypervisor reallocates the freed memory back to the guest
>     *Hypervisor frees the memory and maps a different physical memory
> In order to prevent any information leak hypervisor before allocating
> memory to the guest fills it with zeroes.
> The issue arises when the pattern used for Page Poisoning is 0xaa while
> the newly allocated page received from the hypervisor by the guest is
> filled with the pattern 0x00. This will result in memory corruption errors.
>
> Signed-off-by: Nitesh Narayan Lal <nitesh@redhat.com>

This seems kind of backwards to me. Why disable page poisoning instead
of just not hinting about the free pages? There shouldn't be that many
instances when page poisoning is enabled, and when it is it would make
more sense to leave it enabled rather than silently disable it.

> ---
>  include/linux/page_hinting.h | 8 ++++++++
>  mm/page_poison.c             | 2 +-
>  virt/kvm/page_hinting.c      | 1 +
>  3 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/page_hinting.h b/include/linux/page_hinting.h
> index 2d7ff59f3f6a..e800c6b07561 100644
> --- a/include/linux/page_hinting.h
> +++ b/include/linux/page_hinting.h
> @@ -19,7 +19,15 @@ struct hypervisor_pages {
>  extern int guest_page_hinting_flag;
>  extern struct static_key_false guest_page_hinting_key;
>  extern struct smp_hotplug_thread hinting_threads;
> +extern bool want_page_poisoning;
>
>  int guest_page_hinting_sysctl(struct ctl_table *table, int write,
>                               void __user *buffer, size_t *lenp, loff_t *ppos);
>  void guest_free_page(struct page *page, int order);
> +
> +static inline void disable_page_poisoning(void)
> +{
> +#ifdef CONFIG_PAGE_POISONING
> +       want_page_poisoning = 0;
> +#endif
> +}
> diff --git a/mm/page_poison.c b/mm/page_poison.c
> index f0c15e9017c0..9af96021133b 100644
> --- a/mm/page_poison.c
> +++ b/mm/page_poison.c
> @@ -7,7 +7,7 @@
>  #include <linux/poison.h>
>  #include <linux/ratelimit.h>
>
> -static bool want_page_poisoning __read_mostly;
> +bool want_page_poisoning __read_mostly;
>
>  static int __init early_page_poison_param(char *buf)
>  {
> diff --git a/virt/kvm/page_hinting.c b/virt/kvm/page_hinting.c
> index 636990e7fbb3..be529f6f2bc0 100644
> --- a/virt/kvm/page_hinting.c
> +++ b/virt/kvm/page_hinting.c
> @@ -103,6 +103,7 @@ void guest_free_page(struct page *page, int order)
>
>         local_irq_save(flags);
>         if (page_hinting_obj->kvm_pt_idx != MAX_FGPT_ENTRIES) {
> +               disable_page_poisoning();
>                 page_hinting_obj->kvm_pt[page_hinting_obj->kvm_pt_idx].pfn =
>                                                         page_to_pfn(page);
>                 page_hinting_obj->kvm_pt[page_hinting_obj->kvm_pt_idx].zonenum =

At a minimum it seems like you should have some sort of warning
message that you are disabling page poisoning rather than just
silently turning it off.