On Fri, May 13, 2022, Peter Gonda wrote:
On Thu, May 12, 2022 at 4:23 PM Ashish Kalra <Ashish.Kalra@xxxxxxx> wrote:
From: Ashish Kalra <ashish.kalra@xxxxxxx>Can we just update all the kmalloc()s that buffers get given to the
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@xxxxxxxxxx>
Suggested-by: David Rientjes <rientjes@xxxxxxxxxx>
Suggested-by: Peter Gonda <pgonda@xxxxxxxxxx>
Cc: kvm@xxxxxxxxxxxxxxx
Cc: linux-kernel@xxxxxxxxxxxxxxx
Signed-off-by: Ashish Kalra <ashish.kalra@xxxxxxx>
---
arch/x86/kvm/svm/sev.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
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;