| Message ID | 20180611151902.14383-8-nilal@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jun 11, 2018 at 11:19:02AM -0400, nilal@redhat.com wrote: > From: Nitesh Narayan Lal <nilal@redhat.com> > > 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. Looks like it's a bugfix for your previous patches. If true pls put it 1st in series, we don't want to break things then unbreak them back. > Signed-off-by: Nitesh Narayan Lal <nilal@redhat.com> I'd rather tell hypervisor what the poison value is. See virtio-balloon: VIRTIO_BALLOON_F_PAGE_POISON if that's not advertised, I think you should disable free page hinting, not poisoning. > --- > include/linux/page_hinting.h | 9 +++++++++ > mm/page_poison.c | 2 +- > virt/kvm/page_hinting.c | 1 + > 3 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/include/linux/page_hinting.h b/include/linux/page_hinting.h > index dd30644..b639078 100644 > --- a/include/linux/page_hinting.h > +++ b/include/linux/page_hinting.h > @@ -1,3 +1,4 @@ > +#include <linux/poison.h> > #define MAX_FGPT_ENTRIES 1000 > /* > * hypervisor_pages - It is a dummy structure passed with the hypercall. > @@ -14,6 +15,7 @@ struct hypervisor_pages { > extern struct hypervisor_pages hypervisor_pagelist[MAX_FGPT_ENTRIES]; > extern void (*request_hypercall)(void *, int); > extern void *balloon_ptr; > +extern bool want_page_poisoning; > > extern struct static_key_false guest_page_hinting_key; > int guest_page_hinting_sysctl(struct ctl_table *table, int write, > @@ -21,3 +23,10 @@ int guest_page_hinting_sysctl(struct ctl_table *table, int write, > extern int guest_page_hinting_flag; > void guest_alloc_page(struct page *page, int order); > 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 > +} What if value is read at the same time? Seems racy... > diff --git a/mm/page_poison.c b/mm/page_poison.c > index aa2b3d3..91ce8ad 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 078a3be..57eddd0 100644 > --- a/virt/kvm/page_hinting.c > +++ b/virt/kvm/page_hinting.c > @@ -302,6 +302,7 @@ void guest_free_page(struct page *page, int order) > * process context causing unwanted overwrites. This will be replaced > * with a better solution to prevent such race conditions. > */ > + disable_page_poisoning(); > local_irq_save(flags); > free_page_obj = &get_cpu_var(kvm_pt)[0]; > trace_guest_free_page(page, order); I think we would want to revert everything e.g. if balloon is removed. > -- > 2.9.5
On 06/15/2018 10:44 AM, Michael S. Tsirkin wrote: > On Mon, Jun 11, 2018 at 11:19:02AM -0400, nilal@redhat.com wrote: >> From: Nitesh Narayan Lal <nilal@redhat.com> >> >> 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. > Looks like it's a bugfix for your previous patches. > If true pls put it 1st in series, we don't want to > break things then unbreak them back. Sure, I will put it 1st in my next patch-series. > >> Signed-off-by: Nitesh Narayan Lal <nilal@redhat.com> > I'd rather tell hypervisor what the poison value is. > See > virtio-balloon: VIRTIO_BALLOON_F_PAGE_POISON > > if that's not advertised, I think you should disable free page hinting, > not poisoning. The approach to notify the hypervisor about poison value makes sense. I will make the required changes, thanks for pointing this out. > > >> --- >> include/linux/page_hinting.h | 9 +++++++++ >> mm/page_poison.c | 2 +- >> virt/kvm/page_hinting.c | 1 + >> 3 files changed, 11 insertions(+), 1 deletion(-) >> >> diff --git a/include/linux/page_hinting.h b/include/linux/page_hinting.h >> index dd30644..b639078 100644 >> --- a/include/linux/page_hinting.h >> +++ b/include/linux/page_hinting.h >> @@ -1,3 +1,4 @@ >> +#include <linux/poison.h> >> #define MAX_FGPT_ENTRIES 1000 >> /* >> * hypervisor_pages - It is a dummy structure passed with the hypercall. >> @@ -14,6 +15,7 @@ struct hypervisor_pages { >> extern struct hypervisor_pages hypervisor_pagelist[MAX_FGPT_ENTRIES]; >> extern void (*request_hypercall)(void *, int); >> extern void *balloon_ptr; >> +extern bool want_page_poisoning; >> >> extern struct static_key_false guest_page_hinting_key; >> int guest_page_hinting_sysctl(struct ctl_table *table, int write, >> @@ -21,3 +23,10 @@ int guest_page_hinting_sysctl(struct ctl_table *table, int write, >> extern int guest_page_hinting_flag; >> void guest_alloc_page(struct page *page, int order); >> 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 >> +} > What if value is read at the same time? Seems racy... Yeah, I agree. However, this will not be needed when I will notify the hypervisor about the page poisoning value. > >> diff --git a/mm/page_poison.c b/mm/page_poison.c >> index aa2b3d3..91ce8ad 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 078a3be..57eddd0 100644 >> --- a/virt/kvm/page_hinting.c >> +++ b/virt/kvm/page_hinting.c >> @@ -302,6 +302,7 @@ void guest_free_page(struct page *page, int order) >> * process context causing unwanted overwrites. This will be replaced >> * with a better solution to prevent such race conditions. >> */ >> + disable_page_poisoning(); >> local_irq_save(flags); >> free_page_obj = &get_cpu_var(kvm_pt)[0]; >> trace_guest_free_page(page, order); > I think we would want to revert everything e.g. if balloon > is removed. Yes, I missed this. I will make the required changes in my next patch-series. > >> -- >> 2.9.5
diff --git a/include/linux/page_hinting.h b/include/linux/page_hinting.h index dd30644..b639078 100644 --- a/include/linux/page_hinting.h +++ b/include/linux/page_hinting.h @@ -1,3 +1,4 @@ +#include <linux/poison.h> #define MAX_FGPT_ENTRIES 1000 /* * hypervisor_pages - It is a dummy structure passed with the hypercall. @@ -14,6 +15,7 @@ struct hypervisor_pages { extern struct hypervisor_pages hypervisor_pagelist[MAX_FGPT_ENTRIES]; extern void (*request_hypercall)(void *, int); extern void *balloon_ptr; +extern bool want_page_poisoning; extern struct static_key_false guest_page_hinting_key; int guest_page_hinting_sysctl(struct ctl_table *table, int write, @@ -21,3 +23,10 @@ int guest_page_hinting_sysctl(struct ctl_table *table, int write, extern int guest_page_hinting_flag; void guest_alloc_page(struct page *page, int order); 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 aa2b3d3..91ce8ad 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 078a3be..57eddd0 100644 --- a/virt/kvm/page_hinting.c +++ b/virt/kvm/page_hinting.c @@ -302,6 +302,7 @@ void guest_free_page(struct page *page, int order) * process context causing unwanted overwrites. This will be replaced * with a better solution to prevent such race conditions. */ + disable_page_poisoning(); local_irq_save(flags); free_page_obj = &get_cpu_var(kvm_pt)[0]; trace_guest_free_page(page, order);