[4/9] drivers: hv: Mark shared pages unencrypted in SEV-SNP enlightened guest
Commit Message
From: Tianyu Lan <tiala@microsoft.com>
Hypervisor needs to access iput arg, VMBus synic event and
message pages. Mask these pages unencrypted in the sev-snp
guest and free them only if they have been marked encrypted
successfully.
Signed-off-by: Tianyu Lan <tiala@microsoft.com>
---
drivers/hv/hv.c | 57 +++++++++++++++++++++++++++++++++++++++---
drivers/hv/hv_common.c | 24 +++++++++++++++++-
2 files changed, 77 insertions(+), 4 deletions(-)
Comments
Tianyu Lan <ltykernel@gmail.com> writes:
> From: Tianyu Lan <tiala@microsoft.com>
>
> Hypervisor needs to access iput arg, VMBus synic event and
> message pages. Mask these pages unencrypted in the sev-snp
> guest and free them only if they have been marked encrypted
> successfully.
>
> Signed-off-by: Tianyu Lan <tiala@microsoft.com>
> ---
> drivers/hv/hv.c | 57 +++++++++++++++++++++++++++++++++++++++---
> drivers/hv/hv_common.c | 24 +++++++++++++++++-
> 2 files changed, 77 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
> index de6708dbe0df..94406dbe0df0 100644
> --- a/drivers/hv/hv.c
> +++ b/drivers/hv/hv.c
> @@ -20,6 +20,7 @@
> #include <linux/interrupt.h>
> #include <clocksource/hyperv_timer.h>
> #include <asm/mshyperv.h>
> +#include <linux/set_memory.h>
> #include "hyperv_vmbus.h"
>
> /* The one and only */
> @@ -78,7 +79,7 @@ int hv_post_message(union hv_connection_id connection_id,
>
> int hv_synic_alloc(void)
> {
> - int cpu;
> + int cpu, ret = -ENOMEM;
> struct hv_per_cpu_context *hv_cpu;
>
> /*
> @@ -123,26 +124,76 @@ int hv_synic_alloc(void)
> goto err;
> }
> }
> +
> + if (hv_isolation_type_en_snp()) {
> + ret = set_memory_decrypted((unsigned long)
> + hv_cpu->synic_message_page, 1);
> + if (ret) {
> + pr_err("Failed to decrypt SYNIC msg page: %d\n", ret);
> + hv_cpu->synic_message_page = NULL;
> +
> + /*
> + * Free the event page here and not encrypt
> + * the page in hv_synic_free().
> + */
> + free_page((unsigned long)hv_cpu->synic_event_page);
> + hv_cpu->synic_event_page = NULL;
> + goto err;
> + }
> +
> + ret = set_memory_decrypted((unsigned long)
> + hv_cpu->synic_event_page, 1);
> + if (ret) {
> + pr_err("Failed to decrypt SYNIC event page: %d\n", ret);
> + hv_cpu->synic_event_page = NULL;
> + goto err;
> + }
> +
> + memset(hv_cpu->synic_message_page, 0, PAGE_SIZE);
> + memset(hv_cpu->synic_event_page, 0, PAGE_SIZE);
> + }
> }
>
> return 0;
> +
> err:
> /*
> * Any memory allocations that succeeded will be freed when
> * the caller cleans up by calling hv_synic_free()
> */
> - return -ENOMEM;
> + return ret;
> }
>
>
> void hv_synic_free(void)
> {
> - int cpu;
> + int cpu, ret;
>
> for_each_present_cpu(cpu) {
> struct hv_per_cpu_context *hv_cpu
> = per_cpu_ptr(hv_context.cpu_context, cpu);
>
> + /* It's better to leak the page if the encryption fails. */
> + if (hv_isolation_type_en_snp()) {
> + if (hv_cpu->synic_message_page) {
> + ret = set_memory_encrypted((unsigned long)
> + hv_cpu->synic_message_page, 1);
> + if (ret) {
> + pr_err("Failed to encrypt SYNIC msg page: %d\n", ret);
> + hv_cpu->synic_message_page = NULL;
> + }
> + }
> +
> + if (hv_cpu->synic_event_page) {
> + ret = set_memory_encrypted((unsigned long)
> + hv_cpu->synic_event_page, 1);
> + if (ret) {
> + pr_err("Failed to encrypt SYNIC event page: %d\n", ret);
> + hv_cpu->synic_event_page = NULL;
> + }
> + }
> + }
> +
> free_page((unsigned long)hv_cpu->synic_event_page);
> free_page((unsigned long)hv_cpu->synic_message_page);
> }
> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> index 179bc5f5bf52..bed9aa6ac19a 100644
> --- a/drivers/hv/hv_common.c
> +++ b/drivers/hv/hv_common.c
> @@ -24,6 +24,7 @@
> #include <linux/kmsg_dump.h>
> #include <linux/slab.h>
> #include <linux/dma-map-ops.h>
> +#include <linux/set_memory.h>
> #include <asm/hyperv-tlfs.h>
> #include <asm/mshyperv.h>
>
> @@ -359,6 +360,7 @@ int hv_common_cpu_init(unsigned int cpu)
> u64 msr_vp_index;
> gfp_t flags;
> int pgcount = hv_root_partition ? 2 : 1;
> + int ret;
>
> /* hv_cpu_init() can be called with IRQs disabled from hv_resume() */
> flags = irqs_disabled() ? GFP_ATOMIC : GFP_KERNEL;
> @@ -368,6 +370,17 @@ int hv_common_cpu_init(unsigned int cpu)
> if (!(*inputarg))
> return -ENOMEM;
>
> + if (hv_isolation_type_en_snp()) {
> + ret = set_memory_decrypted((unsigned long)*inputarg, pgcount);
> + if (ret) {
> + kfree(*inputarg);
> + *inputarg = NULL;
> + return ret;
> + }
> +
> + memset(*inputarg, 0x00, pgcount * PAGE_SIZE);
> + }
> +
> if (hv_root_partition) {
> outputarg = (void **)this_cpu_ptr(hyperv_pcpu_output_arg);
> *outputarg = (char *)(*inputarg) + HV_HYP_PAGE_SIZE;
> @@ -387,7 +400,9 @@ int hv_common_cpu_die(unsigned int cpu)
> {
> unsigned long flags;
> void **inputarg, **outputarg;
> + int pgcount = hv_root_partition ? 2 : 1;
> void *mem;
> + int ret;
>
> local_irq_save(flags);
>
> @@ -402,7 +417,14 @@ int hv_common_cpu_die(unsigned int cpu)
>
> local_irq_restore(flags);
>
> - kfree(mem);
> + if (hv_isolation_type_en_snp()) {
> + ret = set_memory_encrypted((unsigned long)mem, pgcount);
> + if (ret)
> + pr_warn("Hyper-V: Failed to encrypt input arg on cpu%d: %d\n",
> + cpu, ret);
> + /* It's unsafe to free 'mem'. */
> + return 0;
Why is it unsafe to free 'mem' if ret == 0? Also, why don't we want to
proparate non-zero 'ret' from here to fail CPU offlining?
> + }
>
> return 0;
> }
On 6/5/2023 8:54 PM, Vitaly Kuznetsov wrote:
>> @@ -402,7 +417,14 @@ int hv_common_cpu_die(unsigned int cpu)
>>
>> local_irq_restore(flags);
>>
>> - kfree(mem);
>> + if (hv_isolation_type_en_snp()) {
>> + ret = set_memory_encrypted((unsigned long)mem, pgcount);
>> + if (ret)
>> + pr_warn("Hyper-V: Failed to encrypt input arg on cpu%d: %d\n",
>> + cpu, ret);
>> + /* It's unsafe to free 'mem'. */
>> + return 0;
> Why is it unsafe to free 'mem' if ret == 0? Also, why don't we want to
> proparate non-zero 'ret' from here to fail CPU offlining?
>
Based on Michael's patch the mem will not be freed during cpu offline.
https://lwn.net/ml/linux-kernel/87cz2j5zrc.fsf@redhat.com/
So I think it's unnessary to encrypt the mem again here.
Tianyu Lan <ltykernel@gmail.com> writes:
> On 6/5/2023 8:54 PM, Vitaly Kuznetsov wrote:
>>> @@ -402,7 +417,14 @@ int hv_common_cpu_die(unsigned int cpu)
>>>
>>> local_irq_restore(flags);
>>>
>>> - kfree(mem);
>>> + if (hv_isolation_type_en_snp()) {
>>> + ret = set_memory_encrypted((unsigned long)mem, pgcount);
>>> + if (ret)
>>> + pr_warn("Hyper-V: Failed to encrypt input arg on cpu%d: %d\n",
>>> + cpu, ret);
>>> + /* It's unsafe to free 'mem'. */
>>> + return 0;
>> Why is it unsafe to free 'mem' if ret == 0? Also, why don't we want to
>> proparate non-zero 'ret' from here to fail CPU offlining?
>>
>
> Based on Michael's patch the mem will not be freed during cpu offline.
> https://lwn.net/ml/linux-kernel/87cz2j5zrc.fsf@redhat.com/
> So I think it's unnessary to encrypt the mem again here.
Good, you can probably include Michael's patch in your next submission
then (unless it gets merged before that).
From: Tianyu Lan <ltykernel@gmail.com> Sent: Thursday, June 1, 2023 8:16 AM
>
> Hypervisor needs to access iput arg, VMBus synic event and
s/iput/input/
> message pages. Mask these pages unencrypted in the sev-snp
s/Mask/Mark/
> guest and free them only if they have been marked encrypted
> successfully.
>
> Signed-off-by: Tianyu Lan <tiala@microsoft.com>
> ---
> drivers/hv/hv.c | 57 +++++++++++++++++++++++++++++++++++++++---
> drivers/hv/hv_common.c | 24 +++++++++++++++++-
> 2 files changed, 77 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
> index de6708dbe0df..94406dbe0df0 100644
> --- a/drivers/hv/hv.c
> +++ b/drivers/hv/hv.c
> @@ -20,6 +20,7 @@
> #include <linux/interrupt.h>
> #include <clocksource/hyperv_timer.h>
> #include <asm/mshyperv.h>
> +#include <linux/set_memory.h>
> #include "hyperv_vmbus.h"
>
> /* The one and only */
> @@ -78,7 +79,7 @@ int hv_post_message(union hv_connection_id connection_id,
>
> int hv_synic_alloc(void)
> {
> - int cpu;
> + int cpu, ret = -ENOMEM;
> struct hv_per_cpu_context *hv_cpu;
>
> /*
> @@ -123,26 +124,76 @@ int hv_synic_alloc(void)
> goto err;
> }
> }
> +
> + if (hv_isolation_type_en_snp()) {
> + ret = set_memory_decrypted((unsigned long)
> + hv_cpu->synic_message_page, 1);
> + if (ret) {
> + pr_err("Failed to decrypt SYNIC msg page: %d\n", ret);
> + hv_cpu->synic_message_page = NULL;
> +
> + /*
> + * Free the event page here and not encrypt
> + * the page in hv_synic_free().
> + */
Let's tweak the wording of the comment:
/*
* Free the event page here so that hv_synic_free()
* won't later try to re-encrypt it.
*/
> + free_page((unsigned long)hv_cpu->synic_event_page);
> + hv_cpu->synic_event_page = NULL;
> + goto err;
> + }
> +
> + ret = set_memory_decrypted((unsigned long)
> + hv_cpu->synic_event_page, 1);
> + if (ret) {
> + pr_err("Failed to decrypt SYNIC event page: %d\n", ret);
> + hv_cpu->synic_event_page = NULL;
> + goto err;
> + }
> +
> + memset(hv_cpu->synic_message_page, 0, PAGE_SIZE);
> + memset(hv_cpu->synic_event_page, 0, PAGE_SIZE);
> + }
> }
>
> return 0;
> +
> err:
> /*
> * Any memory allocations that succeeded will be freed when
> * the caller cleans up by calling hv_synic_free()
> */
> - return -ENOMEM;
> + return ret;
> }
>
>
> void hv_synic_free(void)
> {
> - int cpu;
> + int cpu, ret;
>
> for_each_present_cpu(cpu) {
> struct hv_per_cpu_context *hv_cpu
> = per_cpu_ptr(hv_context.cpu_context, cpu);
>
> + /* It's better to leak the page if the encryption fails. */
> + if (hv_isolation_type_en_snp()) {
> + if (hv_cpu->synic_message_page) {
> + ret = set_memory_encrypted((unsigned long)
> + hv_cpu->synic_message_page, 1);
> + if (ret) {
> + pr_err("Failed to encrypt SYNIC msg page: %d\n", ret);
> + hv_cpu->synic_message_page = NULL;
> + }
> + }
> +
> + if (hv_cpu->synic_event_page) {
> + ret = set_memory_encrypted((unsigned long)
> + hv_cpu->synic_event_page, 1);
> + if (ret) {
> + pr_err("Failed to encrypt SYNIC event page: %d\n", ret);
> + hv_cpu->synic_event_page = NULL;
> + }
> + }
> + }
> +
> free_page((unsigned long)hv_cpu->synic_event_page);
> free_page((unsigned long)hv_cpu->synic_message_page);
> }
> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> index 179bc5f5bf52..bed9aa6ac19a 100644
> --- a/drivers/hv/hv_common.c
> +++ b/drivers/hv/hv_common.c
> @@ -24,6 +24,7 @@
> #include <linux/kmsg_dump.h>
> #include <linux/slab.h>
> #include <linux/dma-map-ops.h>
> +#include <linux/set_memory.h>
> #include <asm/hyperv-tlfs.h>
> #include <asm/mshyperv.h>
>
> @@ -359,6 +360,7 @@ int hv_common_cpu_init(unsigned int cpu)
> u64 msr_vp_index;
> gfp_t flags;
> int pgcount = hv_root_partition ? 2 : 1;
> + int ret;
>
> /* hv_cpu_init() can be called with IRQs disabled from hv_resume() */
> flags = irqs_disabled() ? GFP_ATOMIC : GFP_KERNEL;
> @@ -368,6 +370,17 @@ int hv_common_cpu_init(unsigned int cpu)
> if (!(*inputarg))
> return -ENOMEM;
>
> + if (hv_isolation_type_en_snp()) {
> + ret = set_memory_decrypted((unsigned long)*inputarg, pgcount);
> + if (ret) {
> + kfree(*inputarg);
> + *inputarg = NULL;
> + return ret;
> + }
> +
> + memset(*inputarg, 0x00, pgcount * PAGE_SIZE);
> + }
> +
> if (hv_root_partition) {
> outputarg = (void **)this_cpu_ptr(hyperv_pcpu_output_arg);
> *outputarg = (char *)(*inputarg) + HV_HYP_PAGE_SIZE;
> @@ -387,7 +400,9 @@ int hv_common_cpu_die(unsigned int cpu)
> {
> unsigned long flags;
> void **inputarg, **outputarg;
> + int pgcount = hv_root_partition ? 2 : 1;
> void *mem;
> + int ret;
>
> local_irq_save(flags);
>
> @@ -402,7 +417,14 @@ int hv_common_cpu_die(unsigned int cpu)
>
> local_irq_restore(flags);
>
> - kfree(mem);
> + if (hv_isolation_type_en_snp()) {
> + ret = set_memory_encrypted((unsigned long)mem, pgcount);
> + if (ret)
> + pr_warn("Hyper-V: Failed to encrypt input arg on cpu%d: %d\n",
> + cpu, ret);
> + /* It's unsafe to free 'mem'. */
> + return 0;
> + }
>
> return 0;
> }
> --
> 2.25.1
@@ -20,6 +20,7 @@
#include <linux/interrupt.h>
#include <clocksource/hyperv_timer.h>
#include <asm/mshyperv.h>
+#include <linux/set_memory.h>
#include "hyperv_vmbus.h"
/* The one and only */
@@ -78,7 +79,7 @@ int hv_post_message(union hv_connection_id connection_id,
int hv_synic_alloc(void)
{
- int cpu;
+ int cpu, ret = -ENOMEM;
struct hv_per_cpu_context *hv_cpu;
/*
@@ -123,26 +124,76 @@ int hv_synic_alloc(void)
goto err;
}
}
+
+ if (hv_isolation_type_en_snp()) {
+ ret = set_memory_decrypted((unsigned long)
+ hv_cpu->synic_message_page, 1);
+ if (ret) {
+ pr_err("Failed to decrypt SYNIC msg page: %d\n", ret);
+ hv_cpu->synic_message_page = NULL;
+
+ /*
+ * Free the event page here and not encrypt
+ * the page in hv_synic_free().
+ */
+ free_page((unsigned long)hv_cpu->synic_event_page);
+ hv_cpu->synic_event_page = NULL;
+ goto err;
+ }
+
+ ret = set_memory_decrypted((unsigned long)
+ hv_cpu->synic_event_page, 1);
+ if (ret) {
+ pr_err("Failed to decrypt SYNIC event page: %d\n", ret);
+ hv_cpu->synic_event_page = NULL;
+ goto err;
+ }
+
+ memset(hv_cpu->synic_message_page, 0, PAGE_SIZE);
+ memset(hv_cpu->synic_event_page, 0, PAGE_SIZE);
+ }
}
return 0;
+
err:
/*
* Any memory allocations that succeeded will be freed when
* the caller cleans up by calling hv_synic_free()
*/
- return -ENOMEM;
+ return ret;
}
void hv_synic_free(void)
{
- int cpu;
+ int cpu, ret;
for_each_present_cpu(cpu) {
struct hv_per_cpu_context *hv_cpu
= per_cpu_ptr(hv_context.cpu_context, cpu);
+ /* It's better to leak the page if the encryption fails. */
+ if (hv_isolation_type_en_snp()) {
+ if (hv_cpu->synic_message_page) {
+ ret = set_memory_encrypted((unsigned long)
+ hv_cpu->synic_message_page, 1);
+ if (ret) {
+ pr_err("Failed to encrypt SYNIC msg page: %d\n", ret);
+ hv_cpu->synic_message_page = NULL;
+ }
+ }
+
+ if (hv_cpu->synic_event_page) {
+ ret = set_memory_encrypted((unsigned long)
+ hv_cpu->synic_event_page, 1);
+ if (ret) {
+ pr_err("Failed to encrypt SYNIC event page: %d\n", ret);
+ hv_cpu->synic_event_page = NULL;
+ }
+ }
+ }
+
free_page((unsigned long)hv_cpu->synic_event_page);
free_page((unsigned long)hv_cpu->synic_message_page);
}
@@ -24,6 +24,7 @@
#include <linux/kmsg_dump.h>
#include <linux/slab.h>
#include <linux/dma-map-ops.h>
+#include <linux/set_memory.h>
#include <asm/hyperv-tlfs.h>
#include <asm/mshyperv.h>
@@ -359,6 +360,7 @@ int hv_common_cpu_init(unsigned int cpu)
u64 msr_vp_index;
gfp_t flags;
int pgcount = hv_root_partition ? 2 : 1;
+ int ret;
/* hv_cpu_init() can be called with IRQs disabled from hv_resume() */
flags = irqs_disabled() ? GFP_ATOMIC : GFP_KERNEL;
@@ -368,6 +370,17 @@ int hv_common_cpu_init(unsigned int cpu)
if (!(*inputarg))
return -ENOMEM;
+ if (hv_isolation_type_en_snp()) {
+ ret = set_memory_decrypted((unsigned long)*inputarg, pgcount);
+ if (ret) {
+ kfree(*inputarg);
+ *inputarg = NULL;
+ return ret;
+ }
+
+ memset(*inputarg, 0x00, pgcount * PAGE_SIZE);
+ }
+
if (hv_root_partition) {
outputarg = (void **)this_cpu_ptr(hyperv_pcpu_output_arg);
*outputarg = (char *)(*inputarg) + HV_HYP_PAGE_SIZE;
@@ -387,7 +400,9 @@ int hv_common_cpu_die(unsigned int cpu)
{
unsigned long flags;
void **inputarg, **outputarg;
+ int pgcount = hv_root_partition ? 2 : 1;
void *mem;
+ int ret;
local_irq_save(flags);
@@ -402,7 +417,14 @@ int hv_common_cpu_die(unsigned int cpu)
local_irq_restore(flags);
- kfree(mem);
+ if (hv_isolation_type_en_snp()) {
+ ret = set_memory_encrypted((unsigned long)mem, pgcount);
+ if (ret)
+ pr_warn("Hyper-V: Failed to encrypt input arg on cpu%d: %d\n",
+ cpu, ret);
+ /* It's unsafe to free 'mem'. */
+ return 0;
+ }
return 0;
}