[RFC,v8,4/7] KVM: Disabling page poisoning to prevent corruption
diff mbox series

Message ID 20190204201854.2328-5-nitesh@redhat.com
State New
Headers show
Series
  • KVM: Guest Free Page Hinting
Related show

Commit Message

Nitesh Narayan Lal Feb. 4, 2019, 8:18 p.m. UTC
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>
---
 include/linux/page_hinting.h | 8 ++++++++
 mm/page_poison.c             | 2 +-
 virt/kvm/page_hinting.c      | 1 +
 3 files changed, 10 insertions(+), 1 deletion(-)

Comments

Alexander Duyck Feb. 7, 2019, 5:23 p.m. UTC | #1
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.
Nitesh Narayan Lal Feb. 7, 2019, 5:56 p.m. UTC | #2
On 2/7/19 12:23 PM, Alexander Duyck wrote:
> 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.
As I have mentioned in the cover email, I intend to reuse Wei's already
merged work.

This will enable the guest to communicate the poison value which is in
use to the host.

>
>> ---
>>  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.
Alexander Duyck Feb. 7, 2019, 6:24 p.m. UTC | #3
On Thu, Feb 7, 2019 at 9:56 AM Nitesh Narayan Lal <nitesh@redhat.com> wrote:
>
>
> On 2/7/19 12:23 PM, Alexander Duyck wrote:
> > 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.
> As I have mentioned in the cover email, I intend to reuse Wei's already
> merged work.
>
> This will enable the guest to communicate the poison value which is in
> use to the host.

That is far from being reliable given that you are having to buffer
the pages for some period of time. I really think it would be better
to just allow page poisoning to function and when you can support
applying poison to a newly allocated page then you could look at
re-enabling it.

What I am getting at is that those that care about poisoning won't
likely care about performance and I would lump the memory hinting in
with other performance features.
Michael S. Tsirkin Feb. 7, 2019, 7:14 p.m. UTC | #4
On Thu, Feb 07, 2019 at 10:24:20AM -0800, Alexander Duyck wrote:
> On Thu, Feb 7, 2019 at 9:56 AM Nitesh Narayan Lal <nitesh@redhat.com> wrote:
> >
> >
> > On 2/7/19 12:23 PM, Alexander Duyck wrote:
> > > 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.
> > As I have mentioned in the cover email, I intend to reuse Wei's already
> > merged work.
> >
> > This will enable the guest to communicate the poison value which is in
> > use to the host.
> 
> That is far from being reliable given that you are having to buffer
> the pages for some period of time. I really think it would be better
> to just allow page poisoning to function and when you can support
> applying poison to a newly allocated page then you could look at
> re-enabling it.
> 
> What I am getting at is that those that care about poisoning won't
> likely care about performance and I would lump the memory hinting in
> with other performance features.

It's not just a performance issue.

There is an issue is with the host/guest API. Once host discards pages it
currently always gives you back zero-filled pages.
So guest that looks for the poison using e.g. unpoison_page
will crash unless the poison value is 0.

Idea behind current code is just to let host know:
it can either be more careful with these pages,
or skip them completely.
Michael S. Tsirkin Feb. 7, 2019, 9:08 p.m. UTC | #5
On Mon, Feb 04, 2019 at 03:18:51PM -0500, Nitesh Narayan Lal 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>

IMHO it's better to take the approach of the existing balloon code
and just send the poison value to host. Host can then avoid filling
memory with zeroes.


> ---
>  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 =
> -- 
> 2.17.2

Patch
diff mbox series

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 =