All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sean Christopherson <seanjc@google.com>
To: Peter Gonda <pgonda@google.com>
Cc: kvm@vger.kernel.org, Andy Nguyen <theflow@google.com>,
	Thomas Lendacky <thomas.lendacky@amd.com>,
	David Rientjes <rientjes@google.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	stable@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH V2] KVM: sev: Fix potential overflow send|recieve_update_data
Date: Tue, 7 Feb 2023 23:25:53 +0000	[thread overview]
Message-ID: <Y+LeAfc61yrYerhk@google.com> (raw)
In-Reply-To: <20230207171354.4012821-1-pgonda@google.com>

For now at least, I want to keep with "KVM: SVM:" instead of using "KVM: SEV:".
Many commits that touch SEV aren't strictly isolated to SEV, which means the "SEV"
tag is unreliable.  There's also the question of taggin SEV vs. SEV-ES vs. SEV-SNP.
It's usually easy enough to squeeze SEV (or SEV-ES or SNP) into the shortlog, e.g.

  KVM: SVM: Fix potential overflow in SEV's send|receive_update_data()

On Tue, Feb 07, 2023, Peter Gonda wrote:
> KVM_SEV_SEND_UPDATE_DATA and KVM_SEV_RECEIVE_UPDATE_DATA have an integer
> overflow issue. Params.guest_len and offset are both 32bite wide, with a

"32 bits"

> large params.guest_len the check to confirm a page boundary is not
> crossed can falsely pass:
> 
>     /* Check if we are crossing the page boundary *
>     offset = params.guest_uaddr & (PAGE_SIZE - 1);
>     if ((params.guest_len + offset > PAGE_SIZE))
> 
> Add an additional check to this conditional to confirm that

Eh, "to this conditional" is unnecessarily precise.

> params.guest_len itself is not greater than PAGE_SIZE.
> 
> The current code is can only overflow with a params.guest_len of greater

"is can", though I vote to omit the "current code" part entirely, it should be
obvious that this is talking about the pre-patched code.

> than 0xfffff000. And the FW spec says these commands fail with lengths
> greater than 16KB. So this issue should not be a security concern

Slightly reworded, how about this for the "not a security concern" disclaimer?

  Note, this isn't a security concern as overflow can happen if and only if
  params.guest_len is greater than 0xfffff000, and the FW spec says these
  commands fail with lengths greater than 16KB, i.e. the PSP will detect
  KVM's goof.

No need to send a v3, I'll fix up the changelog when applying.  Holler if you
disagree with anything though.

Thanks!

  parent reply	other threads:[~2023-02-07 23:26 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-02-07 17:13 [PATCH V2] KVM: sev: Fix potential overflow send|recieve_update_data Peter Gonda
2023-02-07 18:39 ` Tom Lendacky
2023-02-07 23:25 ` Sean Christopherson [this message]
2023-02-08  2:07 ` Sean Christopherson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Y+LeAfc61yrYerhk@google.com \
    --to=seanjc@google.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=pgonda@google.com \
    --cc=rientjes@google.com \
    --cc=stable@vger.kernel.org \
    --cc=theflow@google.com \
    --cc=thomas.lendacky@amd.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.