Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 2 Apr 2020 14:43:13 -0500
From:   Venu Busireddy <venu.busireddy@oracle.com>
To:     Brijesh Singh <brijesh.singh@amd.com>
Cc:     Ashish Kalra <Ashish.Kalra@amd.com>, pbonzini@redhat.com,
        tglx@linutronix.de, mingo@redhat.com, hpa@zytor.com,
        joro@8bytes.org, bp@suse.de, thomas.lendacky@amd.com,
        x86@kernel.org, kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        rientjes@google.com, srutherford@google.com, luto@kernel.org
Subject: Re: [PATCH v6 01/14] KVM: SVM: Add KVM_SEV SEND_START command
Message-ID: <20200402194313.GA656773@vbusired-dt>
References: <cover.1585548051.git.ashish.kalra@amd.com>
 <3f90333959fd49bed184d45a761cc338424bf614.1585548051.git.ashish.kalra@amd.com>
 <20200402062726.GA647295@vbusired-dt>
 <89a586e4-8074-0d32-f384-a4597975d129@amd.com>
 <20200402163717.GA653926@vbusired-dt>
 <8b1b4874-11a8-1422-5ea1-ed665f41ab5c@amd.com>
 <20200402185706.GA655878@vbusired-dt>
 <6ced22f7-cbe5-a698-e650-7716566d4d8a@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <6ced22f7-cbe5-a698-e650-7716566d4d8a@amd.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 2020-04-02 14:17:26 -0500, Brijesh Singh wrote:
> 
> On 4/2/20 1:57 PM, Venu Busireddy wrote:
> [snip]...
> 
> >> The question is, how does a userspace know the session length ? One
> >> method is you can precalculate a value based on your firmware version
> >> and have userspace pass that, or another approach is set
> >> params.session_len = 0 and query it from the FW. The FW spec allow to
> >> query the length, please see the spec. In the qemu patches I choose
> >> second approach. This is because session blob can change from one FW
> >> version to another and I tried to avoid calculating or hardcoding the
> >> length for a one version of the FW. You can certainly choose the first
> >> method. We want to ensure that kernel interface works on the both cases.
> > I like the fact that you have already implemented the functionality to
> > facilitate the user space to obtain the session length from the firmware
> > (by setting params.session_len to 0). However, I am trying to address
> > the case where the user space sets the params.session_len to a size
> > smaller than the size needed.
> >
> > Let me put it differently. Let us say that the session blob needs 128
> > bytes, but the user space sets params.session_len to 16. That results
> > in us allocating a buffer of 16 bytes, and set data->session_len to 16.
> >
> > What does the firmware do now?
> >
> > Does it copy 128 bytes into data->session_address, or, does it copy
> > 16 bytes?
> >
> > If it copies 128 bytes, we most certainly will end up with a kernel crash.
> >
> > If it copies 16 bytes, then what does it set in data->session_len? 16,
> > or 128? If 16, everything is good. If 128, we end up causing memory
> > access violation for the user space.
> 
> My interpretation of the spec is, if user provided length is smaller
> than the FW expected length then FW will reports an error with
> data->session_len set to the expected length. In other words, it should
> *not* copy anything into the session buffer in the event of failure.

That is good, and expected behavior.

> If FW is touching memory beyond what is specified in the session_len then
> its FW bug and we can't do much from kernel.

Agreed. But let us assume that the firmware is not touching memory that
it is not supposed to.

> Am I missing something ?

I believe you are agreeing that if the session blob needs 128 bytes and
user space sets params.session_len to 16, the firmware does not copy
any data to data->session_address, and sets data->session_len to 128.

Now, when we return, won't the user space try to access 128 bytes
(params.session_len) of data in params.session_uaddr, and crash? Because,
instead of returning an error that buffer is not large enough, we return
the call successfully!

That is why I was suggesting the following, which you seem to have
missed.

> > Perhaps, this can be dealt a little differently? Why not always call
> > sev_issue_cmd(kvm, SEV_CMD_SEND_START, ...) with zeroed out data? Then,
> > if the user space has set params.session_len to 0, we return with the
> > needed params.session_len. Otherwise, we check if params.session_len is
> > large enough, and if not, we return -EINVAL?

Doesn't the above approach address all scenarios?