[v2,2/5] Drivers: hv: vmbus: Track decrypted status in vmbus_gpadl

Commit Message

Michael Kelley March 11, 2024, 4:15 p.m. UTC

From: Rick Edgecombe <[e-mail]>

In CoCo VMs it is possible for the untrusted host to cause
set_memory_encrypted() or set_memory_decrypted() to fail such that an
error is returned and the resulting memory is shared. Callers need to
take care to handle these errors to avoid returning decrypted (shared)
memory to the page allocator, which could lead to functional or security
issues.

In order to make sure callers of vmbus_establish_gpadl() and
vmbus_teardown_gpadl() don't return decrypted/shared pages to
allocators, add a field in struct vmbus_gpadl to keep track of the
decryption status of the buffers. This will allow the callers to
know if they should free or leak the pages.

Signed-off-by: Rick Edgecombe <[e-mail]>
Signed-off-by: Michael Kelley <[e-mail]>
---
 drivers/hv/channel.c   | 25 +++++++++++++++++++++----
 include/linux/hyperv.h |  1 +
 2 files changed, 22 insertions(+), 4 deletions(-)

Comments

Kuppuswamy Sathyanarayanan March 12, 2024, 5:02 a.m. UTC | #1

On 3/11/24 9:15 AM, [e-mail] wrote:
> From: Rick Edgecombe <[e-mail]>
>
> In CoCo VMs it is possible for the untrusted host to cause
> set_memory_encrypted() or set_memory_decrypted() to fail such that an
> error is returned and the resulting memory is shared. Callers need to
> take care to handle these errors to avoid returning decrypted (shared)
> memory to the page allocator, which could lead to functional or security
> issues.
>
> In order to make sure callers of vmbus_establish_gpadl() and
> vmbus_teardown_gpadl() don't return decrypted/shared pages to
> allocators, add a field in struct vmbus_gpadl to keep track of the
> decryption status of the buffers. This will allow the callers to
> know if they should free or leak the pages.
>
> Signed-off-by: Rick Edgecombe <[e-mail]>
> Signed-off-by: Michael Kelley <[e-mail]>
> ---
>  drivers/hv/channel.c   | 25 +++++++++++++++++++++----
>  include/linux/hyperv.h |  1 +
>  2 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index 56f7e06c673e..bb5abdcda18f 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -472,9 +472,18 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel,
>  		(atomic_inc_return(&vmbus_connection.next_gpadl_handle) - 1);
>  
>  	ret = create_gpadl_header(type, kbuffer, size, send_offset, &msginfo);
> -	if (ret)
> +	if (ret) {
> +		gpadl->decrypted = false;

Why not set it by default at the beginning of the function?

>  		return ret;
> +	}
>  
> +	/*
> +	 * Set the "decrypted" flag to true for the set_memory_decrypted()
> +	 * success case. In the failure case, the encryption state of the
> +	 * memory is unknown. Leave "decrypted" as true to ensure the
> +	 * memory will be leaked instead of going back on the free list.
> +	 */
> +	gpadl->decrypted = true;
>  	ret = set_memory_decrypted((unsigned long)kbuffer,
>  				   PFN_UP(size));
>  	if (ret) {
> @@ -563,9 +572,15 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel,
>  
>  	kfree(msginfo);
>  
> -	if (ret)
> -		set_memory_encrypted((unsigned long)kbuffer,
> -				     PFN_UP(size));
> +	if (ret) {
> +		/*
> +		 * If set_memory_encrypted() fails, the decrypted flag is
> +		 * left as true so the memory is leaked instead of being
> +		 * put back on the free list.
> +		 */
> +		if (!set_memory_encrypted((unsigned long)kbuffer, PFN_UP(size)))
> +			gpadl->decrypted = false;
> +	}
>  
>  	return ret;
>  }
> @@ -886,6 +901,8 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, struct vmbus_gpadl *gpad
>  	if (ret)
>  		pr_warn("Fail to set mem host visibility in GPADL teardown %d.\n", ret);

Will this be called only if vmbus_establish_gpad() is successful? If not, you might want to skip
set_memory_encrypted() call for decrypted = false case.

>  
> +	gpadl->decrypted = ret;
> +

IMO, you can set it to false by default. Any way with non zero return, user know about the
decryption failure.

>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(vmbus_teardown_gpadl);
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 2b00faf98017..5bac136c268c 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -812,6 +812,7 @@ struct vmbus_gpadl {
>  	u32 gpadl_handle;
>  	u32 size;
>  	void *buffer;
> +	bool decrypted;
>  };
>  
>  struct vmbus_channel {

Kuppuswamy Sathyanarayanan March 12, 2024, 5:45 a.m. UTC | #2

On Mon, Mar 11, 2024 at 10:02 PM Kuppuswamy Sathyanarayanan
<[e-mail]> wrote:
>
>
> On 3/11/24 9:15 AM, [e-mail] wrote:
> > From: Rick Edgecombe <[e-mail]>
> >
> > In CoCo VMs it is possible for the untrusted host to cause
> > set_memory_encrypted() or set_memory_decrypted() to fail such that an
> > error is returned and the resulting memory is shared. Callers need to
> > take care to handle these errors to avoid returning decrypted (shared)
> > memory to the page allocator, which could lead to functional or security
> > issues.
> >
> > In order to make sure callers of vmbus_establish_gpadl() and
> > vmbus_teardown_gpadl() don't return decrypted/shared pages to
> > allocators, add a field in struct vmbus_gpadl to keep track of the
> > decryption status of the buffers. This will allow the callers to
> > know if they should free or leak the pages.
> >
> > Signed-off-by: Rick Edgecombe <[e-mail]>
> > Signed-off-by: Michael Kelley <[e-mail]>
> > ---
> >  drivers/hv/channel.c   | 25 +++++++++++++++++++++----
> >  include/linux/hyperv.h |  1 +
> >  2 files changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> > index 56f7e06c673e..bb5abdcda18f 100644
> > --- a/drivers/hv/channel.c
> > +++ b/drivers/hv/channel.c
> > @@ -472,9 +472,18 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel,
> >               (atomic_inc_return(&vmbus_connection.next_gpadl_handle) - 1);
> >
> >       ret = create_gpadl_header(type, kbuffer, size, send_offset, &msginfo);
> > -     if (ret)
> > +     if (ret) {
> > +             gpadl->decrypted = false;
>
> Why not set it by default at the beginning of the function?
>
> >               return ret;
> > +     }
> >
> > +     /*
> > +      * Set the "decrypted" flag to true for the set_memory_decrypted()
> > +      * success case. In the failure case, the encryption state of the
> > +      * memory is unknown. Leave "decrypted" as true to ensure the
> > +      * memory will be leaked instead of going back on the free list.
> > +      */
> > +     gpadl->decrypted = true;
> >       ret = set_memory_decrypted((unsigned long)kbuffer,
> >                                  PFN_UP(size));
> >       if (ret) {
> > @@ -563,9 +572,15 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel,
> >
> >       kfree(msginfo);
> >
> > -     if (ret)
> > -             set_memory_encrypted((unsigned long)kbuffer,
> > -                                  PFN_UP(size));
> > +     if (ret) {
> > +             /*
> > +              * If set_memory_encrypted() fails, the decrypted flag is
> > +              * left as true so the memory is leaked instead of being
> > +              * put back on the free list.
> > +              */
> > +             if (!set_memory_encrypted((unsigned long)kbuffer, PFN_UP(size)))
> > +                     gpadl->decrypted = false;
> > +     }
> >
> >       return ret;
> >  }
> > @@ -886,6 +901,8 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, struct vmbus_gpadl *gpad
> >       if (ret)
> >               pr_warn("Fail to set mem host visibility in GPADL teardown %d.\n", ret);
>
> Will this be called only if vmbus_establish_gpad() is successful? If not, you might want to skip
> set_memory_encrypted() call for decrypted = false case.
>
> >
> > +     gpadl->decrypted = ret;
> > +
>
> IMO, you can set it to false by default. Any way with non zero return, user know about the
> decryption failure.

I understand this change after looking at the rest of the patches. So
please ignore the above
comment.

>
> >       return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(vmbus_teardown_gpadl);
> > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> > index 2b00faf98017..5bac136c268c 100644
> > --- a/include/linux/hyperv.h
> > +++ b/include/linux/hyperv.h
> > @@ -812,6 +812,7 @@ struct vmbus_gpadl {
> >       u32 gpadl_handle;
> >       u32 size;
> >       void *buffer;
> > +     bool decrypted;
> >  };
> >
> >  struct vmbus_channel {
>
> --
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer
>

Michael Kelley March 12, 2024, 6:07 a.m. UTC | #3

From: Kuppuswamy Sathyanarayanan <[e-mail]>
> 
> On 3/11/24 9:15 AM, [e-mail] wrote:
> > From: Rick Edgecombe <[e-mail]>
> >
> > In CoCo VMs it is possible for the untrusted host to cause
> > set_memory_encrypted() or set_memory_decrypted() to fail such that an
> > error is returned and the resulting memory is shared. Callers need to
> > take care to handle these errors to avoid returning decrypted (shared)
> > memory to the page allocator, which could lead to functional or security
> > issues.
> >
> > In order to make sure callers of vmbus_establish_gpadl() and
> > vmbus_teardown_gpadl() don't return decrypted/shared pages to
> > allocators, add a field in struct vmbus_gpadl to keep track of the
> > decryption status of the buffers. This will allow the callers to
> > know if they should free or leak the pages.
> >
> > Signed-off-by: Rick Edgecombe <[e-mail]>
> > Signed-off-by: Michael Kelley <[e-mail]>
> > ---
> >  drivers/hv/channel.c   | 25 +++++++++++++++++++++----
> >  include/linux/hyperv.h |  1 +
> >  2 files changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> > index 56f7e06c673e..bb5abdcda18f 100644
> > --- a/drivers/hv/channel.c
> > +++ b/drivers/hv/channel.c
> > @@ -472,9 +472,18 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel,
> >  		(atomic_inc_return(&vmbus_connection.next_gpadl_handle) - 1);
> >
> >  	ret = create_gpadl_header(type, kbuffer, size, send_offset, &msginfo);
> > -	if (ret)
> > +	if (ret) {
> > +		gpadl->decrypted = false;
> 
> Why not set it by default at the beginning of the function?

I considered doing that.  But it's an extra step to execute in the normal
path, because a couple of lines below it is always set to "true".  But
I don't have a strong preference either way.

> >  		return ret;
> > +	}
> >
> > +	/*
> > +	 * Set the "decrypted" flag to true for the set_memory_decrypted()
> > +	 * success case. In the failure case, the encryption state of the
> > +	 * memory is unknown. Leave "decrypted" as true to ensure the
> > +	 * memory will be leaked instead of going back on the free list.
> > +	 */
> > +	gpadl->decrypted = true;
> >  	ret = set_memory_decrypted((unsigned long)kbuffer,
> >  				   PFN_UP(size));
> >  	if (ret) {
> > @@ -563,9 +572,15 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel,
> >
> >  	kfree(msginfo);
> >
> > -	if (ret)
> > -		set_memory_encrypted((unsigned long)kbuffer,
> > -				     PFN_UP(size));
> > +	if (ret) {
> > +		/*
> > +		 * If set_memory_encrypted() fails, the decrypted flag is
> > +		 * left as true so the memory is leaked instead of being
> > +		 * put back on the free list.
> > +		 */
> > +		if (!set_memory_encrypted((unsigned long)kbuffer, PFN_UP(size)))
> > +			gpadl->decrypted = false;
> > +	}
> >
> >  	return ret;
> >  }
> > @@ -886,6 +901,8 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, struct vmbus_gpadl *gpad
> >  	if (ret)
> >  		pr_warn("Fail to set mem host visibility in GPADL teardown %d.\n", ret);
> 
> Will this be called only if vmbus_establish_gpad() is successful? If not, you
> might want to skip set_memory_encrypted() call for decrypted = false case.

It's only called if vmbus_establish_gpadl() is successful.  I agree
we don't want to call set_memory_encrypted() if the
set_memory_decrypted() wasn't executed or it failed.  But 
vmbus_teardown_gpadl() is never called with decrypted = false.

> 
> >
> > +	gpadl->decrypted = ret;
> > +
> 
> IMO, you can set it to false by default. Any way with non zero return, user
> know about the decryption failure.

I don’t agree, but feel free to explain further if my thinking is
flawed.

If set_memory_encrypted() fails, we want gpadl->decrypted = true.
Yes, the caller can see that vmbus_teardown_gpadl() failed,
but there's also a memory allocation failure, so the caller
would have to distinguish error codes.  And the caller isn't
necessarily where the memory is freed (or leaked).  We
want the decrypted flag to be correct so the code that
eventually frees the memory can decide to leak instead of
freeing.

Michael

> 
> >  	return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(vmbus_teardown_gpadl);
> > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> > index 2b00faf98017..5bac136c268c 100644
> > --- a/include/linux/hyperv.h
> > +++ b/include/linux/hyperv.h
> > @@ -812,6 +812,7 @@ struct vmbus_gpadl {
> >  	u32 gpadl_handle;
> >  	u32 size;
> >  	void *buffer;
> > +	bool decrypted;
> >  };
> >
> >  struct vmbus_channel {
> 
> --
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer

Kuppuswamy Sathyanarayanan March 12, 2024, 3:22 p.m. UTC | #4

On 3/11/24 11:07 PM, Michael Kelley wrote:
> From: Kuppuswamy Sathyanarayanan <[e-mail]>
>> On 3/11/24 9:15 AM, [e-mail] wrote:
>>> From: Rick Edgecombe <[e-mail]>
>>>
>>> In CoCo VMs it is possible for the untrusted host to cause
>>> set_memory_encrypted() or set_memory_decrypted() to fail such that an
>>> error is returned and the resulting memory is shared. Callers need to
>>> take care to handle these errors to avoid returning decrypted (shared)
>>> memory to the page allocator, which could lead to functional or security
>>> issues.
>>>
>>> In order to make sure callers of vmbus_establish_gpadl() and
>>> vmbus_teardown_gpadl() don't return decrypted/shared pages to
>>> allocators, add a field in struct vmbus_gpadl to keep track of the
>>> decryption status of the buffers. This will allow the callers to
>>> know if they should free or leak the pages.
>>>
>>> Signed-off-by: Rick Edgecombe <[e-mail]>
>>> Signed-off-by: Michael Kelley <[e-mail]>
>>> ---
Reviewed-by: Kuppuswamy Sathyanarayanan <[e-mail]>
>>>  drivers/hv/channel.c   | 25 +++++++++++++++++++++----
>>>  include/linux/hyperv.h |  1 +
>>>  2 files changed, 22 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
>>> index 56f7e06c673e..bb5abdcda18f 100644
>>> --- a/drivers/hv/channel.c
>>> +++ b/drivers/hv/channel.c
>>> @@ -472,9 +472,18 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel,
>>>  		(atomic_inc_return(&vmbus_connection.next_gpadl_handle) - 1);
>>>
>>>  	ret = create_gpadl_header(type, kbuffer, size, send_offset, &msginfo);
>>> -	if (ret)
>>> +	if (ret) {
>>> +		gpadl->decrypted = false;
>> Why not set it by default at the beginning of the function?
> I considered doing that.  But it's an extra step to execute in the normal
> path, because a couple of lines below it is always set to "true".  But
> I don't have a strong preference either way.
>

Got it. I am fine either way.

>>>  		return ret;
>>> +	}
>>>
>>> +	/*
>>> +	 * Set the "decrypted" flag to true for the set_memory_decrypted()
>>> +	 * success case. In the failure case, the encryption state of the
>>> +	 * memory is unknown. Leave "decrypted" as true to ensure the
>>> +	 * memory will be leaked instead of going back on the free list.
>>> +	 */
>>> +	gpadl->decrypted = true;
>>>  	ret = set_memory_decrypted((unsigned long)kbuffer,
>>>  				   PFN_UP(size));
>>>  	if (ret) {
>>> @@ -563,9 +572,15 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel,
>>>
>>>  	kfree(msginfo);
>>>
>>> -	if (ret)
>>> -		set_memory_encrypted((unsigned long)kbuffer,
>>> -				     PFN_UP(size));
>>> +	if (ret) {
>>> +		/*
>>> +		 * If set_memory_encrypted() fails, the decrypted flag is
>>> +		 * left as true so the memory is leaked instead of being
>>> +		 * put back on the free list.
>>> +		 */
>>> +		if (!set_memory_encrypted((unsigned long)kbuffer, PFN_UP(size)))
>>> +			gpadl->decrypted = false;
>>> +	}
>>>
>>>  	return ret;
>>>  }
>>> @@ -886,6 +901,8 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, struct vmbus_gpadl *gpad
>>>  	if (ret)
>>>  		pr_warn("Fail to set mem host visibility in GPADL teardown %d.\n", ret);
>> Will this be called only if vmbus_establish_gpad() is successful? If not, you
>> might want to skip set_memory_encrypted() call for decrypted = false case.
> It's only called if vmbus_establish_gpadl() is successful.  I agree
> we don't want to call set_memory_encrypted() if the
> set_memory_decrypted() wasn't executed or it failed.  But 
> vmbus_teardown_gpadl() is never called with decrypted = false.

Since you rely on  vmbus_teardown_gpadl() callers, personally I think it
is better to add that check. It is up to you.

>>> +	gpadl->decrypted = ret;
>>> +
>> IMO, you can set it to false by default. Any way with non zero return, user
>> know about the decryption failure.
> I don’t agree, but feel free to explain further if my thinking is
> flawed.
>
> If set_memory_encrypted() fails, we want gpadl->decrypted = true.
> Yes, the caller can see that vmbus_teardown_gpadl() failed,
> but there's also a memory allocation failure, so the caller
> would have to distinguish error codes.  And the caller isn't
> necessarily where the memory is freed (or leaked).  We
> want the decrypted flag to be correct so the code that
> eventually frees the memory can decide to leak instead of
> freeing.

I agree. I understood this part after looking at the rest of the series.

>
> Michael
>
>>>  	return ret;
>>>  }
>>>  EXPORT_SYMBOL_GPL(vmbus_teardown_gpadl);
>>> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
>>> index 2b00faf98017..5bac136c268c 100644
>>> --- a/include/linux/hyperv.h
>>> +++ b/include/linux/hyperv.h
>>> @@ -812,6 +812,7 @@ struct vmbus_gpadl {
>>>  	u32 gpadl_handle;
>>>  	u32 size;
>>>  	void *buffer;
>>> +	bool decrypted;
>>>  };
>>>
>>>  struct vmbus_channel {
>> --
>> Sathyanarayanan Kuppuswamy
>> Linux Kernel Developer

Michael Kelley March 14, 2024, 1:56 p.m. UTC | #5

From: Kuppuswamy Sathyanarayanan <[e-mail]>
> 
> >>> @@ -886,6 +901,8 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, struct vmbus_gpadl *gpad
> >>>  	if (ret)
> >>>  		pr_warn("Fail to set mem host visibility in GPADL teardown %d.\n", ret);
> >>
> >> Will this be called only if vmbus_establish_gpad() is successful? If not, you
> >> might want to skip set_memory_encrypted() call for decrypted = false case.
> >
> > It's only called if vmbus_establish_gpadl() is successful.  I agree
> > we don't want to call set_memory_encrypted() if the
> > set_memory_decrypted() wasn't executed or it failed.  But
> > vmbus_teardown_gpadl() is never called with decrypted = false.
> 
> Since you rely on  vmbus_teardown_gpadl() callers, personally I think it
> is better to add that check. It is up to you.
> 

In my judgment, a check isn't really necessary.  The structure of the GPADL
code has been stable for a long time, and I'm not aware of anything
pending that would motivate a change.  And if something did change
to call vmbus_teardown_gpadl() with the memory still encrypted,
the call to set_memory_encrypted() will cause an immediate error and
a WARN_ONCE from Rick's patch to __set_memory_enc_pgtable().
The problem won't go unnoticed.

Michael

Patch

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 56f7e06c673e..bb5abdcda18f 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -472,9 +472,18 @@  static int __vmbus_establish_gpadl(struct vmbus_channel *channel,
 		(atomic_inc_return(&vmbus_connection.next_gpadl_handle) - 1);
 
 	ret = create_gpadl_header(type, kbuffer, size, send_offset, &msginfo);
-	if (ret)
+	if (ret) {
+		gpadl->decrypted = false;
 		return ret;
+	}
 
+	/*
+	 * Set the "decrypted" flag to true for the set_memory_decrypted()
+	 * success case. In the failure case, the encryption state of the
+	 * memory is unknown. Leave "decrypted" as true to ensure the
+	 * memory will be leaked instead of going back on the free list.
+	 */
+	gpadl->decrypted = true;
 	ret = set_memory_decrypted((unsigned long)kbuffer,
 				   PFN_UP(size));
 	if (ret) {
@@ -563,9 +572,15 @@  static int __vmbus_establish_gpadl(struct vmbus_channel *channel,
 
 	kfree(msginfo);
 
-	if (ret)
-		set_memory_encrypted((unsigned long)kbuffer,
-				     PFN_UP(size));
+	if (ret) {
+		/*
+		 * If set_memory_encrypted() fails, the decrypted flag is
+		 * left as true so the memory is leaked instead of being
+		 * put back on the free list.
+		 */
+		if (!set_memory_encrypted((unsigned long)kbuffer, PFN_UP(size)))
+			gpadl->decrypted = false;
+	}
 
 	return ret;
 }
@@ -886,6 +901,8 @@  int vmbus_teardown_gpadl(struct vmbus_channel *channel, struct vmbus_gpadl *gpad
 	if (ret)
 		pr_warn("Fail to set mem host visibility in GPADL teardown %d.\n", ret);
 
+	gpadl->decrypted = ret;
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(vmbus_teardown_gpadl);
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 2b00faf98017..5bac136c268c 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -812,6 +812,7 @@  struct vmbus_gpadl {
 	u32 gpadl_handle;
 	u32 size;
 	void *buffer;
+	bool decrypted;
 };
 
 struct vmbus_channel {