[PATCH] KVM: SVM: Use kzalloc for sev ioctl interfaces to prevent kernel memory leak.

[PATCH] KVM: SVM: Use kzalloc for sev ioctl interfaces to prevent kernel memory leak.

[Ashish Kalra]

From: Ashish Kalra <ashish.kalra@amd.com>

For some sev ioctl interfaces, the length parameter that is passed maybe
less than or equal to SEV_FW_BLOB_MAX_SIZE, but larger than the data
that PSP firmware returns. In this case, kmalloc will allocate memory
that is the size of the input rather than the size of the data.
Since PSP firmware doesn't fully overwrite the allocated buffer, these
sev ioctl interface may return uninitialized kernel slab memory.

Reported-by: Andy Nguyen <theflow@google.com>
Suggested-by: David Rientjes <rientjes@google.com>
Suggested-by: Peter Gonda <pgonda@google.com>
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 arch/x86/kvm/svm/sev.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 7c392873626f..d93dab5c9ce1 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -688,7 +688,7 @@ static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp)
 		if (params.len > SEV_FW_BLOB_MAX_SIZE)
 			return -EINVAL;
 
-		blob = kmalloc(params.len, GFP_KERNEL_ACCOUNT);
+		blob = kzalloc(params.len, GFP_KERNEL_ACCOUNT);
 		if (!blob)
 			return -ENOMEM;
 
@@ -808,7 +808,7 @@ static int __sev_dbg_decrypt_user(struct kvm *kvm, unsigned long paddr,
 	if (!IS_ALIGNED(dst_paddr, 16) ||
 	    !IS_ALIGNED(paddr,     16) ||
 	    !IS_ALIGNED(size,      16)) {
-		tpage = (void *)alloc_page(GFP_KERNEL);
+		tpage = (void *)alloc_page(GFP_KERNEL | __GFP_ZERO);
 		if (!tpage)
 			return -ENOMEM;
 
@@ -1094,7 +1094,7 @@ static int sev_get_attestation_report(struct kvm *kvm, struct kvm_sev_cmd *argp)
 		if (params.len > SEV_FW_BLOB_MAX_SIZE)
 			return -EINVAL;
 
-		blob = kmalloc(params.len, GFP_KERNEL_ACCOUNT);
+		blob = kzalloc(params.len, GFP_KERNEL_ACCOUNT);
 		if (!blob)
 			return -ENOMEM;
 
-- 
2.25.1

Re: [PATCH] KVM: SVM: Use kzalloc for sev ioctl interfaces to prevent kernel memory leak.

[Borislav Petkov]

On Thu, May 12, 2022 at 08:23:28PM +0000, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.com>
> 
> For some sev ioctl interfaces, the length parameter that is passed maybe
> less than or equal to SEV_FW_BLOB_MAX_SIZE, but larger than the data
> that PSP firmware returns. In this case, kmalloc will allocate memory
> that is the size of the input rather than the size of the data.
> Since PSP firmware doesn't fully overwrite the allocated buffer, these
> sev ioctl interface may return uninitialized kernel slab memory.
> 
> Reported-by: Andy Nguyen <theflow@google.com>
> Suggested-by: David Rientjes <rientjes@google.com>
> Suggested-by: Peter Gonda <pgonda@google.com>
> Cc: kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>

This looks like it needs one (or more) Fixes: tags pointing to the
patch(es) adding those kmalloc calls...

And then Cc: <stable@vger.kernel.org> too.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] KVM: SVM: Use kzalloc for sev ioctl interfaces to prevent kernel memory leak.

[Peter Gonda]

On Thu, May 12, 2022 at 4:23 PM Ashish Kalra <Ashish.Kalra@amd.com> wrote:
>
> From: Ashish Kalra <ashish.kalra@amd.com>
>
> For some sev ioctl interfaces, the length parameter that is passed maybe
> less than or equal to SEV_FW_BLOB_MAX_SIZE, but larger than the data
> that PSP firmware returns. In this case, kmalloc will allocate memory
> that is the size of the input rather than the size of the data.
> Since PSP firmware doesn't fully overwrite the allocated buffer, these
> sev ioctl interface may return uninitialized kernel slab memory.
>
> Reported-by: Andy Nguyen <theflow@google.com>
> Suggested-by: David Rientjes <rientjes@google.com>
> Suggested-by: Peter Gonda <pgonda@google.com>
> Cc: kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
>  arch/x86/kvm/svm/sev.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>

Can we just update all the kmalloc()s that buffers get given to the
PSP? For instance doesn't sev_send_update_data() have an issue?
Reading the PSP spec it seems like a user can call this ioctl with a
large hdr_len and the PSP will only fill out what's actually required
like in these fixed up cases? This is assuming the PSP is written to
spec (and just the current version). I'd rather have all of these
instances updated.

Re: [PATCH] KVM: SVM: Use kzalloc for sev ioctl interfaces to prevent kernel memory leak.

[Sean Christopherson]

On Fri, May 13, 2022, Peter Gonda wrote:
> On Thu, May 12, 2022 at 4:23 PM Ashish Kalra <Ashish.Kalra@amd.com> wrote:
> >
> > From: Ashish Kalra <ashish.kalra@amd.com>
> >
> > For some sev ioctl interfaces, the length parameter that is passed maybe
> > less than or equal to SEV_FW_BLOB_MAX_SIZE, but larger than the data
> > that PSP firmware returns. In this case, kmalloc will allocate memory
> > that is the size of the input rather than the size of the data.
> > Since PSP firmware doesn't fully overwrite the allocated buffer, these
> > sev ioctl interface may return uninitialized kernel slab memory.
> >
> > Reported-by: Andy Nguyen <theflow@google.com>
> > Suggested-by: David Rientjes <rientjes@google.com>
> > Suggested-by: Peter Gonda <pgonda@google.com>
> > Cc: kvm@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> > ---
> >  arch/x86/kvm/svm/sev.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> 
> Can we just update all the kmalloc()s that buffers get given to the
> PSP? For instance doesn't sev_send_update_data() have an issue?
> Reading the PSP spec it seems like a user can call this ioctl with a
> large hdr_len and the PSP will only fill out what's actually required
> like in these fixed up cases? This is assuming the PSP is written to
> spec (and just the current version). I'd rather have all of these
> instances updated.

Agreed, the kernel should explicitly initialize any copy_to_user() to source and
never rely on the PSP to fill the entire blob unless there's an ironclad guarantee
the entire struct/blob will be written.  E.g. it's probably ok to skip zeroing
"data" in sev_ioctl_do_platform_status(), but even then it might be wortwhile as
defense-in-depth.

Looking through other copy_to_user() calls:

  - "blob" in sev_ioctl_do_pek_csr()
  - "id_blob" in sev_ioctl_do_get_id2()
  - "pdh_blob" and "cert_blob" in sev_ioctl_do_pdh_export()

The last one is probably fine since the copy length comes from the PSP, but it's
not like these ioctls are performance critical...

	/* If we query the length, FW responded with expected data. */
	input.cert_chain_len = data.cert_chain_len;
	input.pdh_cert_len = data.pdh_cert_len;

Re: [PATCH] KVM: SVM: Use kzalloc for sev ioctl interfaces to prevent kernel memory leak.

[Ashish Kalra]

Hello Sean & Peter,

On 5/13/22 14:49, Sean Christopherson wrote:
> On Fri, May 13, 2022, Peter Gonda wrote:
>> On Thu, May 12, 2022 at 4:23 PM Ashish Kalra <Ashish.Kalra@amd.com> wrote:
>>> From: Ashish Kalra <ashish.kalra@amd.com>
>>>
>>> For some sev ioctl interfaces, the length parameter that is passed maybe
>>> less than or equal to SEV_FW_BLOB_MAX_SIZE, but larger than the data
>>> that PSP firmware returns. In this case, kmalloc will allocate memory
>>> that is the size of the input rather than the size of the data.
>>> Since PSP firmware doesn't fully overwrite the allocated buffer, these
>>> sev ioctl interface may return uninitialized kernel slab memory.
>>>
>>> Reported-by: Andy Nguyen <theflow@google.com>
>>> Suggested-by: David Rientjes <rientjes@google.com>
>>> Suggested-by: Peter Gonda <pgonda@google.com>
>>> Cc: kvm@vger.kernel.org
>>> Cc: linux-kernel@vger.kernel.org
>>> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
>>> ---
>>>   arch/x86/kvm/svm/sev.c | 6 +++---
>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>> Can we just update all the kmalloc()s that buffers get given to the
>> PSP? For instance doesn't sev_send_update_data() have an issue?
>> Reading the PSP spec it seems like a user can call this ioctl with a
>> large hdr_len and the PSP will only fill out what's actually required
>> like in these fixed up cases? This is assuming the PSP is written to
>> spec (and just the current version). I'd rather have all of these
>> instances updated.

Yes, this function is also vulnerable as it allocates the return buffer 
using kmalloc() and copies back to user the buffer sized as per the user 
provided length (and not the FW returned length), so it surely needs fixup.

I will update all these instances to use kzalloc() instead of kmalloc().

> Agreed, the kernel should explicitly initialize any copy_to_user() to source and
> never rely on the PSP to fill the entire blob unless there's an ironclad guarantee
> the entire struct/blob will be written.  E.g. it's probably ok to skip zeroing
> "data" in sev_ioctl_do_platform_status(), but even then it might be wortwhile as
> defense-in-depth.
>
> Looking through other copy_to_user() calls:
>
>    - "blob" in sev_ioctl_do_pek_csr()
>    - "id_blob" in sev_ioctl_do_get_id2()
>    - "pdh_blob" and "cert_blob" in sev_ioctl_do_pdh_export()

These functions are part of the ccp driver and a fix for them has 
already been sent upstream to linux-crypto@vger.kernel.org and 
linux-kernel@vger.kernel.org:

[PATCH] crypto: ccp - Use kzalloc for sev ioctl interfaces to prevent 
kernel memory leak

Thanks,

Ashish

>
> The last one is probably fine since the copy length comes from the PSP, but it's
> not like these ioctls are performance critical...
>
> 	/* If we query the length, FW responded with expected data. */
> 	input.cert_chain_len = data.cert_chain_len;
> 	input.pdh_cert_len = data.pdh_cert_len;

Re: [PATCH] KVM: SVM: Use kzalloc for sev ioctl interfaces to prevent kernel memory leak.

[Sean Christopherson]

On Fri, May 13, 2022, Ashish Kalra wrote:
> Hello Sean & Peter,
> > Looking through other copy_to_user() calls:
> > 
> >    - "blob" in sev_ioctl_do_pek_csr()
> >    - "id_blob" in sev_ioctl_do_get_id2()
> >    - "pdh_blob" and "cert_blob" in sev_ioctl_do_pdh_export()
> 
> These functions are part of the ccp driver and a fix for them has already
> been sent upstream to linux-crypto@vger.kernel.org and
> linux-kernel@vger.kernel.org:
> 
> [PATCH] crypto: ccp - Use kzalloc for sev ioctl interfaces to prevent kernel
> memory leak

Ha, that's why I was getting a bit of deja vu.  I saw that fly by and then got it
confused with this patch.

Re: [PATCH] KVM: SVM: Use kzalloc for sev ioctl interfaces to prevent kernel memory leak.

[Peter Gonda]

On Fri, May 13, 2022 at 2:11 PM Ashish Kalra <ashkalra@amd.com> wrote:
>
> Hello Sean & Peter,
>
> On 5/13/22 14:49, Sean Christopherson wrote:
> > On Fri, May 13, 2022, Peter Gonda wrote:
> >> On Thu, May 12, 2022 at 4:23 PM Ashish Kalra <Ashish.Kalra@amd.com> wrote:
> >>> From: Ashish Kalra <ashish.kalra@amd.com>
> >>>
> >>> For some sev ioctl interfaces, the length parameter that is passed maybe
> >>> less than or equal to SEV_FW_BLOB_MAX_SIZE, but larger than the data
> >>> that PSP firmware returns. In this case, kmalloc will allocate memory
> >>> that is the size of the input rather than the size of the data.
> >>> Since PSP firmware doesn't fully overwrite the allocated buffer, these
> >>> sev ioctl interface may return uninitialized kernel slab memory.
> >>>
> >>> Reported-by: Andy Nguyen <theflow@google.com>
> >>> Suggested-by: David Rientjes <rientjes@google.com>
> >>> Suggested-by: Peter Gonda <pgonda@google.com>
> >>> Cc: kvm@vger.kernel.org
> >>> Cc: linux-kernel@vger.kernel.org
> >>> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> >>> ---
> >>>   arch/x86/kvm/svm/sev.c | 6 +++---
> >>>   1 file changed, 3 insertions(+), 3 deletions(-)
> >>>
> >> Can we just update all the kmalloc()s that buffers get given to the
> >> PSP? For instance doesn't sev_send_update_data() have an issue?
> >> Reading the PSP spec it seems like a user can call this ioctl with a
> >> large hdr_len and the PSP will only fill out what's actually required
> >> like in these fixed up cases? This is assuming the PSP is written to
> >> spec (and just the current version). I'd rather have all of these
> >> instances updated.
>
> Yes, this function is also vulnerable as it allocates the return buffer
> using kmalloc() and copies back to user the buffer sized as per the user
> provided length (and not the FW returned length), so it surely needs fixup.
>
> I will update all these instances to use kzalloc() instead of kmalloc().

Do we need the alloc_page() in __sev_dbg_encrypt_user() to have __GFP_ZERO too?


>
> > Agreed, the kernel should explicitly initialize any copy_to_user() to source and
> > never rely on the PSP to fill the entire blob unless there's an ironclad guarantee
> > the entire struct/blob will be written.  E.g. it's probably ok to skip zeroing
> > "data" in sev_ioctl_do_platform_status(), but even then it might be wortwhile as
> > defense-in-depth.
> >
> > Looking through other copy_to_user() calls:
> >
> >    - "blob" in sev_ioctl_do_pek_csr()
> >    - "id_blob" in sev_ioctl_do_get_id2()
> >    - "pdh_blob" and "cert_blob" in sev_ioctl_do_pdh_export()
>
> These functions are part of the ccp driver and a fix for them has
> already been sent upstream to linux-crypto@vger.kernel.org and
> linux-kernel@vger.kernel.org:
>
> [PATCH] crypto: ccp - Use kzalloc for sev ioctl interfaces to prevent
> kernel memory leak
>
> Thanks,
>
> Ashish
>
> >
> > The last one is probably fine since the copy length comes from the PSP, but it's
> > not like these ioctls are performance critical...
> >
> >       /* If we query the length, FW responded with expected data. */
> >       input.cert_chain_len = data.cert_chain_len;
> >       input.pdh_cert_len = data.pdh_cert_len;

Re: [PATCH] KVM: SVM: Use kzalloc for sev ioctl interfaces to prevent kernel memory leak.

[Ashish Kalra]

Hello Peter,

On 5/13/22 20:09, Peter Gonda wrote:
> On Fri, May 13, 2022 at 2:11 PM Ashish Kalra <ashkalra@amd.com> wrote:
>> Hello Sean & Peter,
>>
>> On 5/13/22 14:49, Sean Christopherson wrote:
>>> On Fri, May 13, 2022, Peter Gonda wrote:
>>>> On Thu, May 12, 2022 at 4:23 PM Ashish Kalra <Ashish.Kalra@amd.com> wrote:
>>>>> From: Ashish Kalra <ashish.kalra@amd.com>
>>>>>
>>>>> For some sev ioctl interfaces, the length parameter that is passed maybe
>>>>> less than or equal to SEV_FW_BLOB_MAX_SIZE, but larger than the data
>>>>> that PSP firmware returns. In this case, kmalloc will allocate memory
>>>>> that is the size of the input rather than the size of the data.
>>>>> Since PSP firmware doesn't fully overwrite the allocated buffer, these
>>>>> sev ioctl interface may return uninitialized kernel slab memory.
>>>>>
>>>>> Reported-by: Andy Nguyen <theflow@google.com>
>>>>> Suggested-by: David Rientjes <rientjes@google.com>
>>>>> Suggested-by: Peter Gonda <pgonda@google.com>
>>>>> Cc: kvm@vger.kernel.org
>>>>> Cc: linux-kernel@vger.kernel.org
>>>>> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
>>>>> ---
>>>>>    arch/x86/kvm/svm/sev.c | 6 +++---
>>>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>> Can we just update all the kmalloc()s that buffers get given to the
>>>> PSP? For instance doesn't sev_send_update_data() have an issue?
>>>> Reading the PSP spec it seems like a user can call this ioctl with a
>>>> large hdr_len and the PSP will only fill out what's actually required
>>>> like in these fixed up cases? This is assuming the PSP is written to
>>>> spec (and just the current version). I'd rather have all of these
>>>> instances updated.
>> Yes, this function is also vulnerable as it allocates the return buffer
>> using kmalloc() and copies back to user the buffer sized as per the user
>> provided length (and not the FW returned length), so it surely needs fixup.
>>
>> I will update all these instances to use kzalloc() instead of kmalloc().
> Do we need the alloc_page() in __sev_dbg_encrypt_user() to have __GFP_ZERO too?

Actually this is used to allocate intermediate buffers to do 
copy_from_user, so it should be safe as size here is used for actual 
guest memory size to be encrypted.

Thanks, Ashish
>
>
>>> Agreed, the kernel should explicitly initialize any copy_to_user() to source and
>>> never rely on the PSP to fill the entire blob unless there's an ironclad guarantee
>>> the entire struct/blob will be written.  E.g. it's probably ok to skip zeroing
>>> "data" in sev_ioctl_do_platform_status(), but even then it might be wortwhile as
>>> defense-in-depth.
>>>
>>> Looking through other copy_to_user() calls:
>>>
>>>     - "blob" in sev_ioctl_do_pek_csr()
>>>     - "id_blob" in sev_ioctl_do_get_id2()
>>>     - "pdh_blob" and "cert_blob" in sev_ioctl_do_pdh_export()
>> These functions are part of the ccp driver and a fix for them has
>> already been sent upstream to linux-crypto@vger.kernel.org and
>> linux-kernel@vger.kernel.org:
>>
>> [PATCH] crypto: ccp - Use kzalloc for sev ioctl interfaces to prevent
>> kernel memory leak
>>
>> Thanks,
>>
>> Ashish
>>
>>> The last one is probably fine since the copy length comes from the PSP, but it's
>>> not like these ioctls are performance critical...
>>>
>>>        /* If we query the length, FW responded with expected data. */
>>>        input.cert_chain_len = data.cert_chain_len;
>>>        input.pdh_cert_len = data.pdh_cert_len;