On 3/11/24 11:07 PM, Michael Kelley wrote:
> From: Kuppuswamy Sathyanarayanan <[email protected]>
>> On 3/11/24 9:15 AM, [email protected] wrote:
>>> From: Rick Edgecombe <[email protected]>
>>>
>>> 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 <[email protected]>
>>> Signed-off-by: Michael Kelley <[email protected]>
>>> ---
Reviewed-by: Kuppuswamy Sathyanarayanan <[email protected]>
>>> 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
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
From: Kuppuswamy Sathyanarayanan <[email protected]>
>
> >>> @@ -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